Patchwork [Open-FCoE,01/11] scsi/fcoe: lock online CPUs in fcoe_percpu_clean()

login
register
mail settings
Submitter Sebastian Andrzej Siewior
Date March 11, 2016, 3:28 p.m.
Message ID <1457710143-29182-2-git-send-email-bigeasy@linutronix.de>
Download mbox | patch
Permalink /patch/311/
State Awaiting Upstream
Headers show

Comments

Sebastian Andrzej Siewior - March 11, 2016, 3:28 p.m.
for_each_possible_cpu() with a cpu_online() + `thread' check possibly does
the job. But there is a tiny race: Say CPU5 is reported online but is
going down. And after fcoe_percpu_clean() saw that CPU5 is online it
decided to enqueue a packet. After dev_alloc_skb() returned a skb
that CPU is offline (or say the notifier destroyed the kthread). So we
would OOps because `thread' is NULL.
An alternative would be to lock the CPUs during our loop (so no CPU is
going away) and then we iterate over the online mask.

Cc: Vasu Dev <vasu.dev@intel.com>
Cc: "James E.J. Bottomley" <JBottomley@odin.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: fcoe-devel@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/fcoe/fcoe.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Sebastian Andrzej Siewior - March 11, 2016, 4:32 p.m.
On 03/11/2016 05:17 PM, Christoph Hellwig wrote:
> On Fri, Mar 11, 2016 at 04:28:53PM +0100, Sebastian Andrzej Siewior wrote:
>> for_each_possible_cpu() with a cpu_online() + `thread' check possibly does
>> the job. But there is a tiny race: Say CPU5 is reported online but is
>> going down. And after fcoe_percpu_clean() saw that CPU5 is online it
>> decided to enqueue a packet. After dev_alloc_skb() returned a skb
>> that CPU is offline (or say the notifier destroyed the kthread). So we
>> would OOps because `thread' is NULL.
>> An alternative would be to lock the CPUs during our loop (so no CPU is
>> going away) and then we iterate over the online mask.
> 
> I've looked over this and the following patches, and I suspect
> the right thing to do for fcoe and bnx2 is to convert them to use the
> generic workqueue code instead of reinventing it poorly.

alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
should be struct work_struct but all I have is a skb. Is there an easy
way to get this attached?

Sebastian
Christoph Hellwig - March 15, 2016, 8:19 a.m.
On Fri, Mar 11, 2016 at 05:32:15PM +0100, Sebastian Andrzej Siewior wrote:
> alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
> should be struct work_struct but all I have is a skb. Is there an easy
> way to get this attached?

Good question.  There is skb->cb, but it looks like it doesn't have
space for an additional work_item in the fcoe case.  Maybe have
a per-cpu work_struct and keep all the list handling as-is for now?
Sebastian Andrzej Siewior - April 8, 2016, 1:30 p.m.
On 03/15/2016 09:19 AM, Christoph Hellwig wrote:
> On Fri, Mar 11, 2016 at 05:32:15PM +0100, Sebastian Andrzej Siewior wrote:
>> alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
>> should be struct work_struct but all I have is a skb. Is there an easy
>> way to get this attached?
> 
> Good question.  There is skb->cb, but it looks like it doesn't have
> space for an additional work_item in the fcoe case.  Maybe have
> a per-cpu work_struct and keep all the list handling as-is for now?

Okay. Let me try this. What about the few fixes from the series (which
apply before the rework to smbboot theads)?

Sebastian
Sebastian Andrzej Siewior - April 8, 2016, 6:14 p.m.
On 04/08/2016 03:30 PM, Sebastian Andrzej Siewior wrote:
> On 03/15/2016 09:19 AM, Christoph Hellwig wrote:
>> On Fri, Mar 11, 2016 at 05:32:15PM +0100, Sebastian Andrzej Siewior wrote:
>>> alloc_workqueue() in setup and then queue_work_on(cpu, , item)? item
>>> should be struct work_struct but all I have is a skb. Is there an easy
>>> way to get this attached?
>>
>> Good question.  There is skb->cb, but it looks like it doesn't have
>> space for an additional work_item in the fcoe case.  Maybe have
>> a per-cpu work_struct and keep all the list handling as-is for now?
> 
> Okay. Let me try this. What about the few fixes from the series (which
> apply before the rework to smbboot theads)?

okay kworker. This does not look good. I have it converted what I miss
flushing work when CPU goes down and ensuring not to queue work while
the CPU is down.

- cpu_online(x) is racy. In DOWN_PREPARE the worker is deactivated /
  stopped. However slightly later the bit from the CPU mask is removed.

- Whatever is queued and did not make it before the CPU went down seems
  to be delayed until the CPU comes back online.

- if the worker keeps running while the CPU is going down the worker
  continues running on a different CPU.

So I don't see how the former two points can be solved without keeping
track of CPUs in a CPU notifier. Getting pushed to a different CPU be
probably less of an issue if we would have a work-item and would not
need to rely on the per-CPU list.

Sebastian

Patch

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 0efe7112fc1f..2b0d207f4b2b 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -2461,12 +2461,10 @@  static void fcoe_percpu_clean(struct fc_lport *lport)
 	struct sk_buff *skb;
 	unsigned int cpu;
 
-	for_each_possible_cpu(cpu) {
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
 		pp = &per_cpu(fcoe_percpu, cpu);
 
-		if (!pp->thread || !cpu_online(cpu))
-			continue;
-
 		skb = dev_alloc_skb(0);
 		if (!skb)
 			continue;
@@ -2481,6 +2479,7 @@  static void fcoe_percpu_clean(struct fc_lport *lport)
 
 		wait_for_completion(&fcoe_flush_completion);
 	}
+	put_online_cpus();
 }
 
 /**