flow_cache_flush soft lockup with heavy ipsec traffic

PATCH In review Maris Paupe 3 participants 4 messages Nov 9, 2011

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Maris Paupe

author

2011-11-09 12:39 UTC #0 lore raw

PATCH 1/1 flow_cache_flush soft lockup with heavy ipsec traffic

During ipsec packet processing flow_cache_flush() may get called which creates flow_cache_gc_taklet(), this function is guarded by mutex and waits until all tasklets are finished before releasing it, another softirq may happen during flow_cache_gc_taklet(), in case when this irq is packet reading from a device, it can happen that flow_cache_flush() gets called again and a deadlock occurs. Here i purpose a simple fix to this problem by disabling softirqs during tasklet process. It could also be fixed in ipsec processing code, but I am too unfamiliar with it to touch it.

Signed-off-by Maris Paupe <[email protected]>

net/core/flow.c +2 -0

index 8ae42de..19ff283 100644
@@ -105,6 +105,7 @@ static void flow_cache_gc_task(struct work_struct *work)
 	struct list_head gc_list;
 	struct flow_cache_entry *fce, *n;
 
+	local_bh_disable();
 	INIT_LIST_HEAD(&gc_list);
 	spin_lock_bh(&flow_cache_gc_lock);
 	list_splice_tail_init(&flow_cache_gc_list, &gc_list);

@@ -112,6 +113,7 @@ static void flow_cache_gc_task(struct work_struct *work)
 
 	list_for_each_entry_safe(fce, n, &gc_list, u.gc_list)
 		flow_entry_kill(fce);
+	local_bh_enable();
 }
 static DECLARE_WORK(flow_cache_gc_work, flow_cache_gc_task);

Eric Dumazet

2011-11-09 13:16 UTC #1 lore raw

Quoted 25 lines · Maris Paupe

During ipsec packet processing flow_cache_flush() may get called which creates flow_cache_gc_taklet(), this function is guarded by mutex and waits until all tasklets are finished before releasing it, another softirq may happen during flow_cache_gc_taklet(), in case when this irq is packet reading from a device, it can happen that flow_cache_flush() gets called again and a deadlock occurs.
Here i purpose a simple fix to this problem by disabling softirqs during tasklet process. It could also be fixed in ipsec processing code, but I am too unfamiliar with it to touch it.
Signed-off-by Maris Paupe <[email protected]>
net/core/flow.c +1 -0
index 8ae42de..19ff283 100644
@@ -105,6 +105,7 @@ static void flow_cache_gc_task(struct work_struct *work)
  	struct list_head gc_list;
  	struct flow_cache_entry *fce, *n;
 
+	local_bh_disable();
  	INIT_LIST_HEAD(&gc_list);
  	spin_lock_bh(&flow_cache_gc_lock);
  	list_splice_tail_init(&flow_cache_gc_list, &gc_list);

net/core/flow.c static void flow_cache_gc_task(struct work_struct *work) · line 117

Quoted 5 lines

net/core/flow.c +1 -0

@@ -112,6 +113,7 @@ static void flow_cache_gc_task(struct work_struct *work)
 
  	list_for_each_entry_safe(fce, n, &gc_list, u.gc_list)
  		flow_entry_kill(fce);
+	local_bh_enable();
  }

Sorry, I dont understand your patch.

BH are disabled by the spin_lock_bh() call.

Once flow_cache_entry are in garbage list, nothing but garbage collector can access them. I see no possible deadlock. Or there is a bug somewhere and your patch avoid it.

Whole point of using a work queue to perform garbage collect was to not hold BH too long (allowing sotirq to process incoming packets), so you basically remove what was done in commit 8e4795605d.

Could you explain the problem you have ? Any stack trace or something ?

Steffen Klassert

2011-11-09 13:43 UTC #2 lore raw

Quoted 5 lines · Eric Dumazet

Sorry, I dont understand your patch.
BH are disabled by the spin_lock_bh() call.

Quoted 8 lines

Once flow_cache_entry are in garbage list, nothing but garbage collector can access them. I see no possible deadlock. Or there is a bug somewhere and your patch avoid it.
Whole point of using a work queue to perform garbage collect was to not hold BH too long (allowing sotirq to process incoming packets), so you basically remove what was done in commit 8e4795605d.

I guess this tries to address a problem that was already discussed here:

http://patchwork.ozlabs.org/patch/116457/

Maris Paupe

author

2011-11-09 14:41 UTC #3 lore raw

I think i found the correct fix for my problem here http://patchwork.ozlabs.org/patch/114330/
The scenario sounds the same, i will test it, thanks.

Quoted 19 lines · Steffen Klassert

Quoted 5 lines · Eric Dumazet
Sorry, I dont understand your patch.
BH are disabled by the spin_lock_bh() call.
Quoted 8 lines
Once flow_cache_entry are in garbage list, nothing but garbage collector can access them. I see no possible deadlock. Or there is a bug somewhere and your patch avoid it.
Whole point of using a work queue to perform garbage collect was to not hold BH too long (allowing sotirq to process incoming packets), so you basically remove what was done in commit 8e4795605d.
I guess this tries to address a problem that was already discussed here:
http://patchwork.ozlabs.org/patch/116457/