[REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

This is a repost to get the patches applied against v4.11-rc6. mkp's scsi
for-next tree can be merged with no conflicts.

The last repost [0] was not merged and stalled after Martin pinged Chad
[1]. He didn't even reply after tglx pinged him approx two weeks later.

Johannes Thumshirn was so kind to test the FCoE part of the old series
[2]. I did not add a tested-by tag due to the rebase.

If my memory is correct then my first attempt on this (with hotplug
threads back then) was around December 2015.

The whole series is also available at
 git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git scsi/bnx2_v2

[0] http://www.spinics.net/lists/linux-scsi/msg102143.html
[1] http://www.spinics.net/lists/linux-scsi/msg102295.html
[2] http://www.spinics.net/lists/linux-scsi/msg102716.html

Sebastian

[PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

The DECLARE_PER_CPU() definition is moved into the header file where it
belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets removed and
the remaining code is shifted to the left.
bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
decide if there is an active per-CPU thread. With the kworkers this is no
longer possible nor required.
The allocation of struct bnx2i_work does not happen with ->p_work_lock held
which is not required. I am unsure about the call-stack so I can't say
if this qualifies it for the allocation with GFP_KERNEL instead of
GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
The allocation case has been reversed so the inner if case is called on
!bnx2i_work and is just the invocation one function since the lock is not
held during allocation. The init of the new bnx2i_work struct is now
done also without the ->p_work_lock held: it is a new object, nobody
knows about it yet. It should be enough to hold the lock while adding
this item to the list. I am unsure about that atomic_inc() so I keep
things as they were.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the workqueue code.

This patch was only compile-tested due to -ENODEV.

Cc: QLogic-Storage-Upstream@qlogic.com
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i.h      |  11 ++---
 drivers/scsi/bnx2i/bnx2i_hwi.c  | 101 +++++++++++++++++---------------------
 drivers/scsi/bnx2i/bnx2i_init.c | 104 +++-------------------------------------
 3 files changed, 53 insertions(+), 163 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index 89ef1a1678d1..78f67542cbd3 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -31,7 +31,6 @@
 #include <linux/netdevice.h>
 #include <linux/completion.h>
 #include <linux/kthread.h>
-#include <linux/cpu.h>
 
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
@@ -775,12 +774,11 @@ struct bnx2i_work {
 };
 
 struct bnx2i_percpu_s {
-	struct task_struct *iothread;
+	struct work_struct work;
 	struct list_head work_list;
 	spinlock_t p_work_lock;
 };
 
-
 /* Global variables */
 extern unsigned int error_mask1, error_mask2;
 extern u64 iscsi_error_mask;
@@ -797,7 +795,7 @@ extern unsigned int rq_size;
 
 extern struct device_attribute *bnx2i_dev_attributes[];
 
-
+DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
 /*
  * Function Prototypes
@@ -875,8 +873,5 @@ extern void bnx2i_print_active_cmd_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_xmit_pdu_queue(struct bnx2i_conn *conn);
 extern void bnx2i_print_recv_state(struct bnx2i_conn *conn);
 
-extern int bnx2i_percpu_io_thread(void *arg);
-extern int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-				       struct bnx2i_conn *bnx2i_conn,
-				       struct cqe *cqe);
+extern void bnx2i_percpu_io_work(struct work_struct *work);
 #endif
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 42921dbba927..9be58f6523b3 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -19,8 +19,6 @@
 #include <scsi/libiscsi.h>
 #include "bnx2i.h"
 
-DECLARE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
-
 /**
  * bnx2i_get_cid_num - get cid from ep
  * @ep: 	endpoint pointer
@@ -1350,9 +1348,9 @@ int bnx2i_send_fw_iscsi_init_msg(struct bnx2i_hba *hba)
  *
  * process SCSI CMD Response CQE & complete the request to SCSI-ML
  */
-int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
-				struct bnx2i_conn *bnx2i_conn,
-				struct cqe *cqe)
+static int bnx2i_process_scsi_cmd_resp(struct iscsi_session *session,
+				       struct bnx2i_conn *bnx2i_conn,
+				       struct cqe *cqe)
 {
 	struct iscsi_conn *conn = bnx2i_conn->cls_conn->dd_data;
 	struct bnx2i_hba *hba = bnx2i_conn->hba;
@@ -1862,45 +1860,37 @@ static void bnx2i_process_cmd_cleanup_resp(struct iscsi_session *session,
 
 
 /**
- * bnx2i_percpu_io_thread - thread per cpu for ios
+ * bnx2i_percpu_io_work - thread per cpu for ios
  *
- * @arg:	ptr to bnx2i_percpu_info structure
+ * @work_s:	The work struct
  */
-int bnx2i_percpu_io_thread(void *arg)
+void bnx2i_percpu_io_work(struct work_struct *work_s)
 {
-	struct bnx2i_percpu_s *p = arg;
+	struct bnx2i_percpu_s *p;
 	struct bnx2i_work *work, *tmp;
 	LIST_HEAD(work_list);
 
-	set_user_nice(current, MIN_NICE);
+	p = container_of(work_s, struct bnx2i_percpu_s, work);
 
-	while (!kthread_should_stop()) {
-		spin_lock_bh(&p->p_work_lock);
-		while (!list_empty(&p->work_list)) {
-			list_splice_init(&p->work_list, &work_list);
-			spin_unlock_bh(&p->p_work_lock);
-
-			list_for_each_entry_safe(work, tmp, &work_list, list) {
-				list_del_init(&work->list);
-				/* work allocated in the bh, freed here */
-				bnx2i_process_scsi_cmd_resp(work->session,
-							    work->bnx2i_conn,
-							    &work->cqe);
-				atomic_dec(&work->bnx2i_conn->work_cnt);
-				kfree(work);
-			}
-			spin_lock_bh(&p->p_work_lock);
-		}
-		set_current_state(TASK_INTERRUPTIBLE);
+	spin_lock_bh(&p->p_work_lock);
+	while (!list_empty(&p->work_list)) {
+		list_splice_init(&p->work_list, &work_list);
 		spin_unlock_bh(&p->p_work_lock);
-		schedule();
+
+		list_for_each_entry_safe(work, tmp, &work_list, list) {
+			list_del_init(&work->list);
+			/* work allocated in the bh, freed here */
+			bnx2i_process_scsi_cmd_resp(work->session,
+						    work->bnx2i_conn,
+						    &work->cqe);
+			atomic_dec(&work->bnx2i_conn->work_cnt);
+			kfree(work);
+		}
+		spin_lock_bh(&p->p_work_lock);
 	}
-	__set_current_state(TASK_RUNNING);
-
-	return 0;
+	spin_unlock_bh(&p->p_work_lock);
 }
 
-
 /**
  * bnx2i_queue_scsi_cmd_resp - queue cmd completion to the percpu thread
  * @bnx2i_conn:		bnx2i connection
@@ -1920,7 +1910,6 @@ static int bnx2i_queue_scsi_cmd_resp(struct iscsi_session *session,
 	struct bnx2i_percpu_s *p = NULL;
 	struct iscsi_task *task;
 	struct scsi_cmnd *sc;
-	int rc = 0;
 	int cpu;
 
 	spin_lock(&session->back_lock);
@@ -1939,33 +1928,29 @@ static int bnx2i_queue_scsi_cmd_resp(struct iscsi_session *session,
 
 	spin_unlock(&session->back_lock);
 
-	p = &per_cpu(bnx2i_percpu, cpu);
-	spin_lock(&p->p_work_lock);
-	if (unlikely(!p->iothread)) {
-		rc = -EINVAL;
-		goto err;
-	}
 	/* Alloc and copy to the cqe */
 	bnx2i_work = kzalloc(sizeof(struct bnx2i_work), GFP_ATOMIC);
-	if (bnx2i_work) {
-		INIT_LIST_HEAD(&bnx2i_work->list);
-		bnx2i_work->session = session;
-		bnx2i_work->bnx2i_conn = bnx2i_conn;
-		memcpy(&bnx2i_work->cqe, cqe, sizeof(struct cqe));
-		list_add_tail(&bnx2i_work->list, &p->work_list);
-		atomic_inc(&bnx2i_conn->work_cnt);
-		wake_up_process(p->iothread);
-		spin_unlock(&p->p_work_lock);
-		goto done;
-	} else
-		rc = -ENOMEM;
-err:
-	spin_unlock(&p->p_work_lock);
-	bnx2i_process_scsi_cmd_resp(session, bnx2i_conn, (struct cqe *)cqe);
-done:
-	return rc;
-}
+	if (!bnx2i_work) {
+		bnx2i_process_scsi_cmd_resp(session, bnx2i_conn,
+					    (struct cqe *)cqe);
+		return -ENOMEM;
+	}
 
+	p = per_cpu_ptr(&bnx2i_percpu, cpu);
+
+	INIT_LIST_HEAD(&bnx2i_work->list);
+	bnx2i_work->session = session;
+	bnx2i_work->bnx2i_conn = bnx2i_conn;
+	memcpy(&bnx2i_work->cqe, cqe, sizeof(struct cqe));
+
+	spin_lock(&p->p_work_lock);
+	list_add_tail(&bnx2i_work->list, &p->work_list);
+	atomic_inc(&bnx2i_conn->work_cnt);
+	spin_unlock(&p->p_work_lock);
+
+	schedule_work_on(cpu, &p->work);
+	return 0;
+}
 
 /**
  * bnx2i_process_new_cqes - process newly DMA'ed CQE's
diff --git a/drivers/scsi/bnx2i/bnx2i_init.c b/drivers/scsi/bnx2i/bnx2i_init.c
index 86afc002814c..551a17f9c841 100644
--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -402,73 +402,6 @@ int bnx2i_get_stats(void *handle)
 	return 0;
 }
 
-
-/**
- * bnx2i_percpu_thread_create - Create a receive thread for an
- *				online CPU
- *
- * @cpu:	cpu index for the online cpu
- */
-static void bnx2i_percpu_thread_create(unsigned int cpu)
-{
-	struct bnx2i_percpu_s *p;
-	struct task_struct *thread;
-
-	p = &per_cpu(bnx2i_percpu, cpu);
-
-	thread = kthread_create_on_node(bnx2i_percpu_io_thread, (void *)p,
-					cpu_to_node(cpu),
-					"bnx2i_thread/%d", cpu);
-	/* bind thread to the cpu */
-	if (likely(!IS_ERR(thread))) {
-		kthread_bind(thread, cpu);
-		p->iothread = thread;
-		wake_up_process(thread);
-	}
-}
-
-
-static void bnx2i_percpu_thread_destroy(unsigned int cpu)
-{
-	struct bnx2i_percpu_s *p;
-	struct task_struct *thread;
-	struct bnx2i_work *work, *tmp;
-
-	/* Prevent any new work from being queued for this CPU */
-	p = &per_cpu(bnx2i_percpu, cpu);
-	spin_lock_bh(&p->p_work_lock);
-	thread = p->iothread;
-	p->iothread = NULL;
-
-	/* Free all work in the list */
-	list_for_each_entry_safe(work, tmp, &p->work_list, list) {
-		list_del_init(&work->list);
-		bnx2i_process_scsi_cmd_resp(work->session,
-					    work->bnx2i_conn, &work->cqe);
-		kfree(work);
-	}
-
-	spin_unlock_bh(&p->p_work_lock);
-	if (thread)
-		kthread_stop(thread);
-}
-
-static int bnx2i_cpu_online(unsigned int cpu)
-{
-	pr_info("bnx2i: CPU %x online: Create Rx thread\n", cpu);
-	bnx2i_percpu_thread_create(cpu);
-	return 0;
-}
-
-static int bnx2i_cpu_dead(unsigned int cpu)
-{
-	pr_info("CPU %x offline: Remove Rx thread\n", cpu);
-	bnx2i_percpu_thread_destroy(cpu);
-	return 0;
-}
-
-static enum cpuhp_state bnx2i_online_state;
-
 /**
  * bnx2i_mod_init - module init entry point
  *
@@ -505,41 +438,20 @@ static int __init bnx2i_mod_init(void)
 
 	/* Create percpu kernel threads to handle iSCSI I/O completions */
 	for_each_possible_cpu(cpu) {
-		p = &per_cpu(bnx2i_percpu, cpu);
+		p = per_cpu_ptr(&bnx2i_percpu, cpu);
 		INIT_LIST_HEAD(&p->work_list);
 		spin_lock_init(&p->p_work_lock);
-		p->iothread = NULL;
+		INIT_WORK(&p->work, bnx2i_percpu_io_work);
 	}
 
-	get_online_cpus();
-
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_create(cpu);
-
-	err = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-				       "scsi/bnx2i:online",
-				       bnx2i_cpu_online, NULL);
-	if (err < 0)
-		goto remove_threads;
-	bnx2i_online_state = err;
-
-	cpuhp_setup_state_nocalls(CPUHP_SCSI_BNX2I_DEAD, "scsi/bnx2i:dead",
-				  NULL, bnx2i_cpu_dead);
-	put_online_cpus();
 	return 0;
 
-remove_threads:
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_destroy(cpu);
-	put_online_cpus();
-	cnic_unregister_driver(CNIC_ULP_ISCSI);
 unreg_xport:
 	iscsi_unregister_transport(&bnx2i_iscsi_transport);
 out:
 	return err;
 }
 
-
 /**
  * bnx2i_mod_exit - module cleanup/exit entry point
  *
@@ -569,14 +481,12 @@ static void __exit bnx2i_mod_exit(void)
 	}
 	mutex_unlock(&bnx2i_dev_lock);
 
-	get_online_cpus();
+	for_each_possible_cpu(cpu) {
+		struct bnx2i_percpu_s *p;
 
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_destroy(cpu);
-
-	cpuhp_remove_state_nocalls(bnx2i_online_state);
-	cpuhp_remove_state_nocalls(CPUHP_SCSI_BNX2I_DEAD);
-	put_online_cpus();
+		p = per_cpu_ptr(&bnx2i_percpu, cpu);
+		flush_work(&p->work);
+	}
 
 	iscsi_unregister_transport(&bnx2i_iscsi_transport);
 	cnic_unregister_driver(CNIC_ULP_ISCSI);
-- 
2.11.0

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Johannes Thumshirn]

On 04/10/2017 07:12 PM, Sebastian Andrzej Siewior wrote:
> The driver creates its own per-CPU threads which are updated based on CPU
> hotplug events. It is also possible to use kworkers and remove some of the
> infrastructure get the same job done while saving a few lines of code.
>
> The DECLARE_PER_CPU() definition is moved into the header file where it
> belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
> mostly the same code. The outer loop (kthread_should_stop()) gets removed and
> the remaining code is shifted to the left.
> bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked ->iothread to
> decide if there is an active per-CPU thread. With the kworkers this is no
> longer possible nor required.
> The allocation of struct bnx2i_work does not happen with ->p_work_lock held
> which is not required. I am unsure about the call-stack so I can't say
> if this qualifies it for the allocation with GFP_KERNEL instead of
> GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
> The allocation case has been reversed so the inner if case is called on
> !bnx2i_work and is just the invocation one function since the lock is not
> held during allocation. The init of the new bnx2i_work struct is now
> done also without the ->p_work_lock held: it is a new object, nobody
> knows about it yet. It should be enough to hold the lock while adding
> this item to the list. I am unsure about that atomic_inc() so I keep
> things as they were.
>
> The remaining part is the removal CPU hotplug notifier since it is taken
> care by the workqueue code.
>
> This patch was only compile-tested due to -ENODEV.
>
> Cc: QLogic-Storage-Upstream@qlogic.com
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Rangankar, Manish]

On 10/04/17 10:42 PM, "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>
wrote:

>The driver creates its own per-CPU threads which are updated based on CPU
>hotplug events. It is also possible to use kworkers and remove some of the
>infrastructure get the same job done while saving a few lines of code.
>
>The DECLARE_PER_CPU() definition is moved into the header file where it
>belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
>mostly the same code. The outer loop (kthread_should_stop()) gets removed
>and
>the remaining code is shifted to the left.
>bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked
>->iothread to
>decide if there is an active per-CPU thread. With the kworkers this is no
>longer possible nor required.
>The allocation of struct bnx2i_work does not happen with ->p_work_lock
>held
>which is not required. I am unsure about the call-stack so I can't say
>if this qualifies it for the allocation with GFP_KERNEL instead of
>GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
>The allocation case has been reversed so the inner if case is called on
>!bnx2i_work and is just the invocation one function since the lock is not
>held during allocation. The init of the new bnx2i_work struct is now
>done also without the ->p_work_lock held: it is a new object, nobody
>knows about it yet. It should be enough to hold the lock while adding
>this item to the list. I am unsure about that atomic_inc() so I keep
>things as they were.
>
>The remaining part is the removal CPU hotplug notifier since it is taken
>care by the workqueue code.
>
>This patch was only compile-tested due to -ENODEV.
>
>Cc: QLogic-Storage-Upstream@qlogic.com
>Cc: Christoph Hellwig <hch@lst.de>
>Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>---

Didn't seen any issue with regression testing. Thanks Sebastian.

Acked-by: Manish Rangankar <Manish.Rangankar@cavium.com>

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Johannes Thumshirn]

So here we are again,
Tested-by: Johannes Thumshirn <jthumshirn@suse.de>

FCoE will follow as soon as my setup can speak FCoE again.
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

On 2017-06-29 15:57:56 [+0200], Johannes Thumshirn wrote:
> So here we are again,
> Tested-by: Johannes Thumshirn <jthumshirn@suse.de>
> 
> FCoE will follow as soon as my setup can speak FCoE again.

So it all looks good, doesn't it? Chad never responded to my question
on his patch. I still doubt that it fixes the problem he observed.

Sebastian

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Chad Dupuis]

On Fri, 7 Jul 2017, 9:14am, Sebastian Andrzej Siewior wrote:

> On 2017-06-29 15:57:56 [+0200], Johannes Thumshirn wrote:
> > So here we are again,
> > Tested-by: Johannes Thumshirn <jthumshirn@suse.de>
> > 
> > FCoE will follow as soon as my setup can speak FCoE again.
> 
> So it all looks good, doesn't it? Chad never responded to my question
> on his patch. I still doubt that it fixes the problem he observed.
> 
> Sebastian
> 

What was the question?  My observation is that the patch I proposed fixed 
the issue we saw on testing the patch set.  With that small change 
(essentially modulo by the number of active CPUs vs. the total number) 
your patch set worked ok.

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

On 2017-07-07 09:20:02 [-0400], Chad Dupuis wrote:
> What was the question?  My observation is that the patch I proposed fixed 
> the issue we saw on testing the patch set.  With that small change 
> (essentially modulo by the number of active CPUs vs. the total number) 
> your patch set worked ok.

That mail at the bottom of this mail where I said why I think your patch
is a nop in this context.

Sebastian

On 2017-05-17 17:07:34 [+0200], To Chad Dupuis wrote:
> > > Sebastian, can you add this change to your patch set?
> >
> > Are sure that you can reliably reproduce the issue and fix it with the
> > patch above? Because this patch:
>
> oh. Okay. Now it clicked. It can fix the issue but it is still possible,
> that CPU0 goes down between your check for it and schedule_work_on()
> returning. Let my think of something…

Oh wait. I already thought about this: it may take bnx2fc_percpu from
CPU7 and run the worker on CPU3. The job isn't lost, because the worker
does:
                                                    
| static void bnx2fc_percpu_io_work(struct work_struct *work_s)
| {
|         struct bnx2fc_percpu_s *p;
 …
|         p = container_of(work_s, struct bnx2fc_percpu_s, work);
|
|         spin_lock_bh(&p->fp_work_lock);

and so will access bnx2fc_percpu of CPU7 running on CPU3. So I *think*
that your patch should make no difference and there should be no leak if
schedule_work_on() is invoked on an offline CPU.

[PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to workqueue

[Sebastian Andrzej Siewior]

The driver creates its own per-CPU threads which are updated based on CPU
hotplug events. It is also possible to use kworkers and remove some of the
infrastructure get the same job done while saving a few lines of code.

bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
mostly the same code. The outer loop (kthread_should_stop()) gets
removed and the remaining code is shifted to the left.
In bnx2fc_process_new_cqes() the code checked for ->iothread to
decide if there is an active per-CPU thread. With the kworkers this
is no longer possible nor required. The allocation of a new work item
(via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
lock held. It performs only a memory allocation + initialization which
does not require any kind of serialization. The lock is held while
adding the new member to fps->work_list list.

The remaining part is the removal CPU hotplug notifier since it is taken
care by the workqueue code.

This patch was only compile-tested due to -ENODEV.

Cc: QLogic-Storage-Upstream@qlogic.com
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h      |   2 +-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 144 ++++++--------------------------------
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 33 insertions(+), 135 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index 4fc8ed5fe067..0279cc8de7a0 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -168,7 +168,7 @@ extern struct fcoe_percpu_s bnx2fc_global;
 extern struct workqueue_struct *bnx2fc_wq;
 
 struct bnx2fc_percpu_s {
-	struct task_struct *iothread;
+	struct work_struct work;
 	struct list_head work_list;
 	spinlock_t fp_work_lock;
 };
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 93b5a0012417..329922d51f8a 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -614,39 +614,32 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 }
 
 /**
- * bnx2fc_percpu_io_thread - thread per cpu for ios
+ * bnx2fc_percpu_io_work - work per cpu for ios
  *
- * @arg:	ptr to bnx2fc_percpu_info structure
+ * @work_s:	The work struct
  */
-static int bnx2fc_percpu_io_thread(void *arg)
+static void bnx2fc_percpu_io_work(struct work_struct *work_s)
 {
-	struct bnx2fc_percpu_s *p = arg;
+	struct bnx2fc_percpu_s *p;
 	struct bnx2fc_work *work, *tmp;
 	LIST_HEAD(work_list);
 
-	set_user_nice(current, MIN_NICE);
-	set_current_state(TASK_INTERRUPTIBLE);
-	while (!kthread_should_stop()) {
-		schedule();
-		spin_lock_bh(&p->fp_work_lock);
-		while (!list_empty(&p->work_list)) {
-			list_splice_init(&p->work_list, &work_list);
-			spin_unlock_bh(&p->fp_work_lock);
+	p = container_of(work_s, struct bnx2fc_percpu_s, work);
 
-			list_for_each_entry_safe(work, tmp, &work_list, list) {
-				list_del_init(&work->list);
-				bnx2fc_process_cq_compl(work->tgt, work->wqe);
-				kfree(work);
-			}
-
-			spin_lock_bh(&p->fp_work_lock);
-		}
-		__set_current_state(TASK_INTERRUPTIBLE);
+	spin_lock_bh(&p->fp_work_lock);
+	while (!list_empty(&p->work_list)) {
+		list_splice_init(&p->work_list, &work_list);
 		spin_unlock_bh(&p->fp_work_lock);
-	}
-	__set_current_state(TASK_RUNNING);
 
-	return 0;
+		list_for_each_entry_safe(work, tmp, &work_list, list) {
+			list_del_init(&work->list);
+			bnx2fc_process_cq_compl(work->tgt, work->wqe);
+			kfree(work);
+		}
+
+		spin_lock_bh(&p->fp_work_lock);
+	}
+	spin_unlock_bh(&p->fp_work_lock);
 }
 
 static struct fc_host_statistics *bnx2fc_get_host_stats(struct Scsi_Host *shost)
@@ -2563,73 +2556,6 @@ static struct fcoe_transport bnx2fc_transport = {
 	.disable = bnx2fc_disable,
 };
 
-/**
- * bnx2fc_percpu_thread_create - Create a receive thread for an
- *				 online CPU
- *
- * @cpu: cpu index for the online cpu
- */
-static void bnx2fc_percpu_thread_create(unsigned int cpu)
-{
-	struct bnx2fc_percpu_s *p;
-	struct task_struct *thread;
-
-	p = &per_cpu(bnx2fc_percpu, cpu);
-
-	thread = kthread_create_on_node(bnx2fc_percpu_io_thread,
-					(void *)p, cpu_to_node(cpu),
-					"bnx2fc_thread/%d", cpu);
-	/* bind thread to the cpu */
-	if (likely(!IS_ERR(thread))) {
-		kthread_bind(thread, cpu);
-		p->iothread = thread;
-		wake_up_process(thread);
-	}
-}
-
-static void bnx2fc_percpu_thread_destroy(unsigned int cpu)
-{
-	struct bnx2fc_percpu_s *p;
-	struct task_struct *thread;
-	struct bnx2fc_work *work, *tmp;
-
-	BNX2FC_MISC_DBG("destroying io thread for CPU %d\n", cpu);
-
-	/* Prevent any new work from being queued for this CPU */
-	p = &per_cpu(bnx2fc_percpu, cpu);
-	spin_lock_bh(&p->fp_work_lock);
-	thread = p->iothread;
-	p->iothread = NULL;
-
-
-	/* Free all work in the list */
-	list_for_each_entry_safe(work, tmp, &p->work_list, list) {
-		list_del_init(&work->list);
-		bnx2fc_process_cq_compl(work->tgt, work->wqe);
-		kfree(work);
-	}
-
-	spin_unlock_bh(&p->fp_work_lock);
-
-	if (thread)
-		kthread_stop(thread);
-}
-
-
-static int bnx2fc_cpu_online(unsigned int cpu)
-{
-	printk(PFX "CPU %x online: Create Rx thread\n", cpu);
-	bnx2fc_percpu_thread_create(cpu);
-	return 0;
-}
-
-static int bnx2fc_cpu_dead(unsigned int cpu)
-{
-	printk(PFX "CPU %x offline: Remove Rx thread\n", cpu);
-	bnx2fc_percpu_thread_destroy(cpu);
-	return 0;
-}
-
 static int bnx2fc_slave_configure(struct scsi_device *sdev)
 {
 	if (!bnx2fc_queue_depth)
@@ -2699,33 +2625,13 @@ static int __init bnx2fc_mod_init(void)
 		p = &per_cpu(bnx2fc_percpu, cpu);
 		INIT_LIST_HEAD(&p->work_list);
 		spin_lock_init(&p->fp_work_lock);
+		INIT_WORK(&p->work, bnx2fc_percpu_io_work);
 	}
 
-	get_online_cpus();
-
-	for_each_online_cpu(cpu)
-		bnx2fc_percpu_thread_create(cpu);
-
-	rc = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-				       "scsi/bnx2fc:online",
-				       bnx2fc_cpu_online, NULL);
-	if (rc < 0)
-		goto stop_threads;
-	bnx2fc_online_state = rc;
-
-	cpuhp_setup_state_nocalls(CPUHP_SCSI_BNX2FC_DEAD, "scsi/bnx2fc:dead",
-				  NULL, bnx2fc_cpu_dead);
-	put_online_cpus();
-
 	cnic_register_driver(CNIC_ULP_FCOE, &bnx2fc_cnic_cb);
 
 	return 0;
 
-stop_threads:
-	for_each_online_cpu(cpu)
-		bnx2fc_percpu_thread_destroy(cpu);
-	put_online_cpus();
-	kthread_stop(l2_thread);
 free_wq:
 	destroy_workqueue(bnx2fc_wq);
 release_bt:
@@ -2784,17 +2690,13 @@ static void __exit bnx2fc_mod_exit(void)
 	if (l2_thread)
 		kthread_stop(l2_thread);
 
-	get_online_cpus();
-	/* Destroy per cpu threads */
-	for_each_online_cpu(cpu) {
-		bnx2fc_percpu_thread_destroy(cpu);
+	for_each_possible_cpu(cpu) {
+		struct bnx2fc_percpu_s *p;
+
+		p = per_cpu_ptr(&bnx2fc_percpu, cpu);
+		flush_work(&p->work);
 	}
 
-	cpuhp_remove_state_nocalls(bnx2fc_online_state);
-	cpuhp_remove_state_nocalls(CPUHP_SCSI_BNX2FC_DEAD);
-
-	put_online_cpus();
-
 	destroy_workqueue(bnx2fc_wq);
 	/*
 	 * detach from scsi transport
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 5ff9f89c17c7..1ed7a1784e15 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1046,23 +1046,19 @@ int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt)
 			struct bnx2fc_percpu_s *fps = NULL;
 			unsigned int cpu = wqe % num_possible_cpus();
 
-			fps = &per_cpu(bnx2fc_percpu, cpu);
-			spin_lock_bh(&fps->fp_work_lock);
-			if (unlikely(!fps->iothread))
-				goto unlock;
-
 			work = bnx2fc_alloc_work(tgt, wqe);
-			if (work)
+			if (work) {
+				fps = &per_cpu(bnx2fc_percpu, cpu);
+
+				spin_lock_bh(&fps->fp_work_lock);
 				list_add_tail(&work->list,
 					      &fps->work_list);
-unlock:
-			spin_unlock_bh(&fps->fp_work_lock);
-
-			/* Pending work request completion */
-			if (fps->iothread && work)
-				wake_up_process(fps->iothread);
-			else
+				spin_unlock_bh(&fps->fp_work_lock);
+				schedule_work_on(cpu, &fps->work);
+			} else {
 				bnx2fc_process_cq_compl(tgt, wqe);
+			}
+
 			num_free_sqes++;
 		}
 		cqe++;
-- 
2.11.0

Re: [PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to workqueue

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/5] scsi: bnx2fc: convert per-CPU thread to workqueue

[Johannes Thumshirn]

On 04/10/2017 07:12 PM, Sebastian Andrzej Siewior wrote:
> The driver creates its own per-CPU threads which are updated based on CPU
> hotplug events. It is also possible to use kworkers and remove some of the
> infrastructure get the same job done while saving a few lines of code.
>
> bnx2fc_percpu_io_thread() becomes bnx2fc_percpu_io_work() which is
> mostly the same code. The outer loop (kthread_should_stop()) gets
> removed and the remaining code is shifted to the left.
> In bnx2fc_process_new_cqes() the code checked for ->iothread to
> decide if there is an active per-CPU thread. With the kworkers this
> is no longer possible nor required. The allocation of a new work item
> (via bnx2fc_alloc_work()) does no longer happen with the ->fp_work_lock
> lock held. It performs only a memory allocation + initialization which
> does not require any kind of serialization. The lock is held while
> adding the new member to fps->work_list list.
>
> The remaining part is the removal CPU hotplug notifier since it is taken
> care by the workqueue code.
>
> This patch was only compile-tested due to -ENODEV.
>
> Cc: QLogic-Storage-Upstream@qlogic.com
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

[PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Sebastian Andrzej Siewior]

- All symbols which are only used within one .c file are marked static
  and removed from the bnx2fc.h file if possible.

- the declarion of bnx2fc_percpu is moved into the header file

This patch was only compile-tested due to -ENODEV.

Cc: QLogic-Storage-Upstream@qlogic.com
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc.h      |  5 +----
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 19 +++++++++----------
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  6 ++----
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index 0279cc8de7a0..5b2151e7d894 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -172,6 +172,7 @@ struct bnx2fc_percpu_s {
 	struct list_head work_list;
 	spinlock_t fp_work_lock;
 };
+DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
 
 struct bnx2fc_fw_stats {
 	u64	fc_crc_cnt;
@@ -513,7 +514,6 @@ void bnx2fc_cmd_mgr_free(struct bnx2fc_cmd_mgr *cmgr);
 void bnx2fc_get_link_state(struct bnx2fc_hba *hba);
 char *bnx2fc_get_next_rqe(struct bnx2fc_rport *tgt, u8 num_items);
 void bnx2fc_return_rqe(struct bnx2fc_rport *tgt, u8 num_items);
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen);
 int bnx2fc_send_rrq(struct bnx2fc_cmd *aborted_io_req);
 int bnx2fc_send_adisc(struct bnx2fc_rport *tgt, struct fc_frame *fp);
 int bnx2fc_send_logo(struct bnx2fc_rport *tgt, struct fc_frame *fp);
@@ -537,7 +537,6 @@ void bnx2fc_init_task(struct bnx2fc_cmd *io_req,
 void bnx2fc_add_2_sq(struct bnx2fc_rport *tgt, u16 xid);
 void bnx2fc_ring_doorbell(struct bnx2fc_rport *tgt);
 int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd);
-int bnx2fc_eh_host_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd);
 int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd);
 void bnx2fc_rport_event_handler(struct fc_lport *lport,
@@ -570,8 +569,6 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, u32 did,
 						   struct fc_frame *,
 						   void *),
 				      void *arg, u32 timeout);
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt);
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt);
 void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe);
 struct bnx2fc_rport *bnx2fc_tgt_lookup(struct fcoe_port *port,
 					     u32 port_id);
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 329922d51f8a..2f66c2ea093c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -49,7 +49,7 @@ struct workqueue_struct *bnx2fc_wq;
  * Here the io threads are per cpu but the l2 thread is just one
  */
 struct fcoe_percpu_s bnx2fc_global;
-DEFINE_SPINLOCK(bnx2fc_global_lock);
+static DEFINE_SPINLOCK(bnx2fc_global_lock);
 
 static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
@@ -107,22 +107,22 @@ MODULE_PARM_DESC(debug_logging,
 		"\t\t0x10 - fcoe L2 fame related logs.\n"
 		"\t\t0xff - LOG all messages.");
 
-uint bnx2fc_devloss_tmo;
+static uint bnx2fc_devloss_tmo;
 module_param_named(devloss_tmo, bnx2fc_devloss_tmo, uint, S_IRUGO);
 MODULE_PARM_DESC(devloss_tmo, " Change devloss_tmo for the remote ports "
 	"attached via bnx2fc.");
 
-uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
+static uint bnx2fc_max_luns = BNX2FC_MAX_LUN;
 module_param_named(max_luns, bnx2fc_max_luns, uint, S_IRUGO);
 MODULE_PARM_DESC(max_luns, " Change the default max_lun per SCSI host. Default "
 	"0xffff.");
 
-uint bnx2fc_queue_depth;
+static uint bnx2fc_queue_depth;
 module_param_named(queue_depth, bnx2fc_queue_depth, uint, S_IRUGO);
 MODULE_PARM_DESC(queue_depth, " Change the default queue depth of SCSI devices "
 	"attached via bnx2fc.");
 
-uint bnx2fc_log_fka;
+static uint bnx2fc_log_fka;
 module_param_named(log_fka, bnx2fc_log_fka, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(log_fka, " Print message to kernel log when fcoe is "
 	"initiating a FIP keep alive when debug logging is enabled.");
@@ -167,7 +167,7 @@ static void bnx2fc_clean_rx_queue(struct fc_lport *lp)
 	spin_unlock_bh(&bg->fcoe_rx_list.lock);
 }
 
-int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
+static int bnx2fc_get_paged_crc_eof(struct sk_buff *skb, int tlen)
 {
 	int rc;
 	spin_lock(&bnx2fc_global_lock);
@@ -1395,10 +1395,9 @@ static struct bnx2fc_hba *bnx2fc_hba_create(struct cnic_dev *cnic)
 	return NULL;
 }
 
-static struct bnx2fc_interface *
-bnx2fc_interface_create(struct bnx2fc_hba *hba,
-			struct net_device *netdev,
-			enum fip_state fip_mode)
+static struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
+				      struct net_device *netdev,
+				      enum fip_state fip_mode)
 {
 	struct fcoe_ctlr_device *ctlr_dev;
 	struct bnx2fc_interface *interface;
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 1ed7a1784e15..c2288d6cd217 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -14,8 +14,6 @@
 
 #include "bnx2fc.h"
 
-DECLARE_PER_CPU(struct bnx2fc_percpu_s, bnx2fc_percpu);
-
 static void bnx2fc_fastpath_notification(struct bnx2fc_hba *hba,
 					struct fcoe_kcqe *new_cqe_kcqe);
 static void bnx2fc_process_ofld_cmpl(struct bnx2fc_hba *hba,
@@ -980,7 +978,7 @@ void bnx2fc_process_cq_compl(struct bnx2fc_rport *tgt, u16 wqe)
 	spin_unlock_bh(&tgt->tgt_lock);
 }
 
-void bnx2fc_arm_cq(struct bnx2fc_rport *tgt)
+static void bnx2fc_arm_cq(struct bnx2fc_rport *tgt)
 {
 	struct b577xx_fcoe_rx_doorbell *rx_db = &tgt->rx_db;
 	u32 msg;
@@ -1007,7 +1005,7 @@ static struct bnx2fc_work *bnx2fc_alloc_work(struct bnx2fc_rport *tgt, u16 wqe)
 	return work;
 }
 
-int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt)
+static int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt)
 {
 	struct fcoe_cqe *cq;
 	u32 cq_cons;
-- 
2.11.0

Re: [PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/5] scsi: bnx2fc: clean up header definitions

[Johannes Thumshirn]

On 04/10/2017 07:12 PM, Sebastian Andrzej Siewior wrote:
> - All symbols which are only used within one .c file are marked static
>   and removed from the bnx2fc.h file if possible.
>
> - the declarion of bnx2fc_percpu is moved into the header file
>
> This patch was only compile-tested due to -ENODEV.
>
> Cc: QLogic-Storage-Upstream@qlogic.com
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

[PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Sebastian Andrzej Siewior]

The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
is expected to release the lock during wait_for_completion() and acquire
the lock afterwards.

This patch was only compile-tested due to -ENODEV.

Cc: QLogic-Storage-Upstream@qlogic.com
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 898461b146cc..3eed2453648c 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1080,6 +1080,8 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 }
 
 static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+__releases(tgt->tgt_lock)
+__acquires(tgt->tgt_lock)
 {
 	struct bnx2fc_rport *tgt = io_req->tgt;
 	int rc = SUCCESS;
-- 
2.11.0

Re: [PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/5] scsi: bnx2fc: annoate unlock + release for sparse

[Johannes Thumshirn]

On 04/10/2017 07:12 PM, Sebastian Andrzej Siewior wrote:
> The caller of bnx2fc_abts_cleanup() holds the tgt->tgt_lock lock and it
> is expected to release the lock during wait_for_completion() and acquire
> the lock afterwards.
>
> This patch was only compile-tested due to -ENODEV.
>
> Cc: QLogic-Storage-Upstream@qlogic.com
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

[PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to workqueue

[Sebastian Andrzej Siewior]

This is not driven by the hotplug conversation but while I am at it
looks like a good candidate. Converting the thread to a workqueue user
removes also the kthread member from struct fcoe_percpu_s.

This driver uses the struct fcoe_percpu_s but it does not need the
crc_eof_page member, only the work item and fcoe_rx_list. So it is
removed there.

We had one thread so we only use the workqueue on the current CPU. If
someone knows how spread this nicely, it would only require the usage of
schedule_work_on() instead schedule_work() :)

This patch was only compile-tested due to -ENODEV.

Cc: QLogic-Storage-Upstream@qlogic.com
Cc: Christoph Hellwig <hch@lst.de>
Cc: fcoe-devel@open-fcoe.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 59 ++++++++-------------------------------
 include/scsi/libfcoe.h            |  1 -
 2 files changed, 12 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 2f66c2ea093c..ca1b5908114d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -479,7 +479,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	__skb_queue_tail(&bg->fcoe_rx_list, skb);
 	if (bg->fcoe_rx_list.qlen == 1)
-		wake_up_process(bg->kthread);
+		schedule_work(&bg->work);
 
 	spin_unlock(&bg->fcoe_rx_list.lock);
 
@@ -489,26 +489,20 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
 	return -1;
 }
 
-static int bnx2fc_l2_rcv_thread(void *arg)
+static void bnx2fc_l2_rcv_work(struct work_struct *work_s)
 {
-	struct fcoe_percpu_s *bg = arg;
+	struct fcoe_percpu_s *bg;
 	struct sk_buff *skb;
 
-	set_user_nice(current, MIN_NICE);
-	set_current_state(TASK_INTERRUPTIBLE);
-	while (!kthread_should_stop()) {
-		schedule();
-		spin_lock_bh(&bg->fcoe_rx_list.lock);
-		while ((skb = __skb_dequeue(&bg->fcoe_rx_list)) != NULL) {
-			spin_unlock_bh(&bg->fcoe_rx_list.lock);
-			bnx2fc_recv_frame(skb);
-			spin_lock_bh(&bg->fcoe_rx_list.lock);
-		}
-		__set_current_state(TASK_INTERRUPTIBLE);
+	bg = container_of(work_s, struct fcoe_percpu_s, work);
+
+	spin_lock_bh(&bg->fcoe_rx_list.lock);
+	while ((skb = __skb_dequeue(&bg->fcoe_rx_list)) != NULL) {
 		spin_unlock_bh(&bg->fcoe_rx_list.lock);
+		bnx2fc_recv_frame(skb);
+		spin_lock_bh(&bg->fcoe_rx_list.lock);
 	}
-	__set_current_state(TASK_RUNNING);
-	return 0;
+	spin_unlock_bh(&bg->fcoe_rx_list.lock);
 }
 
 
@@ -2564,8 +2558,6 @@ static int bnx2fc_slave_configure(struct scsi_device *sdev)
 	return 0;
 }
 
-static enum cpuhp_state bnx2fc_online_state;
-
 /**
  * bnx2fc_mod_init - module init entry point
  *
@@ -2575,7 +2567,6 @@ static enum cpuhp_state bnx2fc_online_state;
 static int __init bnx2fc_mod_init(void)
 {
 	struct fcoe_percpu_s *bg;
-	struct task_struct *l2_thread;
 	int rc = 0;
 	unsigned int cpu = 0;
 	struct bnx2fc_percpu_s *p;
@@ -2608,17 +2599,7 @@ static int __init bnx2fc_mod_init(void)
 
 	bg = &bnx2fc_global;
 	skb_queue_head_init(&bg->fcoe_rx_list);
-	l2_thread = kthread_create(bnx2fc_l2_rcv_thread,
-				   (void *)bg,
-				   "bnx2fc_l2_thread");
-	if (IS_ERR(l2_thread)) {
-		rc = PTR_ERR(l2_thread);
-		goto free_wq;
-	}
-	wake_up_process(l2_thread);
-	spin_lock_bh(&bg->fcoe_rx_list.lock);
-	bg->kthread = l2_thread;
-	spin_unlock_bh(&bg->fcoe_rx_list.lock);
+	INIT_WORK(&bg->work, bnx2fc_l2_rcv_work);
 
 	for_each_possible_cpu(cpu) {
 		p = &per_cpu(bnx2fc_percpu, cpu);
@@ -2631,8 +2612,6 @@ static int __init bnx2fc_mod_init(void)
 
 	return 0;
 
-free_wq:
-	destroy_workqueue(bnx2fc_wq);
 release_bt:
 	bnx2fc_release_transport();
 detach_ft:
@@ -2645,9 +2624,6 @@ static void __exit bnx2fc_mod_exit(void)
 {
 	LIST_HEAD(to_be_deleted);
 	struct bnx2fc_hba *hba, *next;
-	struct fcoe_percpu_s *bg;
-	struct task_struct *l2_thread;
-	struct sk_buff *skb;
 	unsigned int cpu = 0;
 
 	/*
@@ -2676,18 +2652,7 @@ static void __exit bnx2fc_mod_exit(void)
 	}
 	cnic_unregister_driver(CNIC_ULP_FCOE);
 
-	/* Destroy global thread */
-	bg = &bnx2fc_global;
-	spin_lock_bh(&bg->fcoe_rx_list.lock);
-	l2_thread = bg->kthread;
-	bg->kthread = NULL;
-	while ((skb = __skb_dequeue(&bg->fcoe_rx_list)) != NULL)
-		kfree_skb(skb);
-
-	spin_unlock_bh(&bg->fcoe_rx_list.lock);
-
-	if (l2_thread)
-		kthread_stop(l2_thread);
+	flush_work(&bnx2fc_global.work);
 
 	for_each_possible_cpu(cpu) {
 		struct bnx2fc_percpu_s *p;
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 722d3264d3bf..ca6fb3c0f172 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -332,7 +332,6 @@ struct fcoe_transport {
  *		    memory for a new trailer
  */
 struct fcoe_percpu_s {
-	struct task_struct *kthread;
 	struct work_struct work;
 	struct sk_buff_head fcoe_rx_list;
 	struct page *crc_eof_page;
-- 
2.11.0

Re: [PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to workqueue

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/5] scsi: bnx2fc: convert bnx2fc_l2_rcv_thread() to workqueue

[Johannes Thumshirn]

On 04/10/2017 07:12 PM, Sebastian Andrzej Siewior wrote:
> This is not driven by the hotplug conversation but while I am at it
> looks like a good candidate. Converting the thread to a workqueue user
> removes also the kthread member from struct fcoe_percpu_s.
>
> This driver uses the struct fcoe_percpu_s but it does not need the
> crc_eof_page member, only the work item and fcoe_rx_list. So it is
> removed there.
>
> We had one thread so we only use the workqueue on the current CPU. If
> someone knows how spread this nicely, it would only require the usage of
> schedule_work_on() instead schedule_work() :)
>
> This patch was only compile-tested due to -ENODEV.
>
> Cc: QLogic-Storage-Upstream@qlogic.com
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: fcoe-devel@open-fcoe.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Chad Dupuis]

On Mon, 10 Apr 2017, 5:12pm -0000, Sebastian Andrzej Siewior wrote:

> This is a repost to get the patches applied against v4.11-rc6. mkp's scsi
> for-next tree can be merged with no conflicts.
> 
> The last repost [0] was not merged and stalled after Martin pinged Chad
> [1]. He didn't even reply after tglx pinged him approx two weeks later.
> 
> Johannes Thumshirn was so kind to test the FCoE part of the old series
> [2]. I did not add a tested-by tag due to the rebase.
> 
> If my memory is correct then my first attempt on this (with hotplug
> threads back then) was around December 2015.
> 
> The whole series is also available at
>  git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git scsi/bnx2_v2
> 
> [0] http://www.spinics.net/lists/linux-scsi/msg102143.html
> [1] http://www.spinics.net/lists/linux-scsi/msg102295.html
> [2] http://www.spinics.net/lists/linux-scsi/msg102716.html
> 
> Sebastian
> 

Sebastian, will take a look. 

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-04-10 14:20:41 [-0400], Chad Dupuis wrote:
> Sebastian, will take a look. 

This [0] commit in tip's smp/hotplug branch is staged for the next merge
window. It will trigger a lockdep warning on the recursive
get_online_cpus() invocation in the two drivers. It is fixed/avoided by
the series.

[0] https://git.kernel.org/tip/tip/c/d215aab82d81974f438bfbc80aa437132f3c37c3

Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-04-10 19:12:49 [+0200], To Martin K . Petersen wrote:
> This is a repost to get the patches applied against v4.11-rc6. mkp's scsi
> for-next tree can be merged with no conflicts.
…

Martin, do you see any chance to get this merged? Chad replied to the
list that he is going to test it on 2017-04-10, didn't respond to the
ping 10 days later. The series stalled last time in the same way.

> The whole series is also available at
>  git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/hotplug-staging.git scsi/bnx2_v2
 
Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Martin K. Petersen]

Sebastian,

> Martin, do you see any chance to get this merged? Chad replied to the
> list that he is going to test it on 2017-04-10, didn't respond to the
> ping 10 days later. The series stalled last time in the same way.

I am very reluctant to merge something when a driver has an active
maintainer and that person has not acked the change.

That said, Chad: You have been sitting on this for quite a while. Please
make it a priority. In exchange for veto rights you do have to provide
timely feedback on anything that touches your driver.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Chad Dupuis]

On Mon, 8 May 2017, 10:04pm, Martin K. Petersen wrote:

> 
> Sebastian,
> 
> > Martin, do you see any chance to get this merged? Chad replied to the
> > list that he is going to test it on 2017-04-10, didn't respond to the
> > ping 10 days later. The series stalled last time in the same way.
> 
> I am very reluctant to merge something when a driver has an active
> maintainer and that person has not acked the change.
> 
> That said, Chad: You have been sitting on this for quite a while. Please
> make it a priority. In exchange for veto rights you do have to provide
> timely feedback on anything that touches your driver.
> 
> Thanks!
> 

We did do some testing and hit a calltrace during device discovery:

[ 1332.551799] INFO: task scsi_eh_15:1970 blocked for more than 120 
seconds.
[ 1332.551804] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables 
this message.
[ 1332.551807] scsi_eh_15      D ffff880823488c14     0  1970      2 
0x00000080
[ 1332.551813]  ffff881053a17cb0 0000000000000046 ffff88084d693ec0 
ffff881053a17fd8
[ 1332.551817]  ffff881053a17fd8 ffff881053a17fd8 ffff88084d693ec0 
ffff880823488d48
[ 1332.551821]  ffff880823488d50 7fffffffffffffff ffff88084d693ec0 
ffff880823488c14
[ 1332.551825] Call Trace:
[ 1332.551838]  [<ffffffff8168b579>] schedule+0x29/0x70
[ 1332.551844]  [<ffffffff81688fc9>] schedule_timeout+0x239/0x2d0
[ 1332.551850]  [<ffffffff810882f8>] ? console_unlock+0x208/0x400
[ 1332.551855]  [<ffffffff810888b4>] ? vprintk_emit+0x3c4/0x510
[ 1332.551861]  [<ffffffff81096acb>] ? lock_timer_base.isra.33+0x2b/0x50
[ 1332.551866]  [<ffffffff8168b956>] wait_for_completion+0x116/0x170
[ 1332.551874]  [<ffffffff810c4ec0>] ? wake_up_state+0x20/0x20
[ 1332.551885]  [<ffffffffa06ae234>] bnx2fc_abts_cleanup+0x3d/0x62 
[bnx2fc]
[ 1332.551892]  [<ffffffffa06a5a80>] bnx2fc_eh_abort+0x470/0x580 [bnx2fc]
[ 1332.551900]  [<ffffffff814570af>] scsi_error_handler+0x59f/0x8b0
[ 1332.551904]  [<ffffffff81456b10>] ? scsi_eh_get_sense+0x250/0x250
[ 1332.551911]  [<ffffffff810b052f>] kthread+0xcf/0xe0
[ 1332.551916]  [<ffffffff810b0460>] ? kthread_create_on_node+0x140/0x140
[ 1332.551923]  [<ffffffff81696418>] ret_from_fork+0x58/0x90
[ 1332.551928]  [<ffffffff810b0460>] ? kthread_create_on_node+0x140/0x140 

To be honest, I'm reluctant to merge these patches on bnx2fc as the I/O 
path on this driver has been stable for quite some time and given that 
it's an older driver I'm not looking to make changes there.

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[James Bottomley]

On Tue, 2017-05-09 at 10:17 -0400, Chad Dupuis wrote:
> On Mon, 8 May 2017, 10:04pm, Martin K. Petersen wrote:
> 
> > 
> > Sebastian,
> > 
> > > Martin, do you see any chance to get this merged? Chad replied to
> the
> > > list that he is going to test it on 2017-04-10, didn't respond to
> the
> > > ping 10 days later. The series stalled last time in the same way.
> > 
> > I am very reluctant to merge something when a driver has an active
> > maintainer and that person has not acked the change.
> > 
> > That said, Chad: You have been sitting on this for quite a while.
> Please
> > make it a priority. In exchange for veto rights you do have to
> provide
> > timely feedback on anything that touches your driver.
> > 
> > Thanks!
> > 
> 
> We did do some testing and hit a calltrace during device discovery:
> 
> [ 1332.551799] INFO: task scsi_eh_15:1970 blocked for more than 120 
> seconds.
> [ 1332.551804] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables 
> this message.
> [ 1332.551807] scsi_eh_15      D ffff880823488c14     0  1970      2 
> 0x00000080
> [ 1332.551813]  ffff881053a17cb0 0000000000000046 ffff88084d693ec0 
> ffff881053a17fd8
> [ 1332.551817]  ffff881053a17fd8 ffff881053a17fd8 ffff88084d693ec0 
> ffff880823488d48
> [ 1332.551821]  ffff880823488d50 7fffffffffffffff ffff88084d693ec0 
> ffff880823488c14
> [ 1332.551825] Call Trace:
> [ 1332.551838]  [<ffffffff8168b579>] schedule+0x29/0x70
> [ 1332.551844]  [<ffffffff81688fc9>] schedule_timeout+0x239/0x2d0
> [ 1332.551850]  [<ffffffff810882f8>] ? console_unlock+0x208/0x400
> [ 1332.551855]  [<ffffffff810888b4>] ? vprintk_emit+0x3c4/0x510
> [ 1332.551861]  [<ffffffff81096acb>] ?
> lock_timer_base.isra.33+0x2b/0x50
> [ 1332.551866]  [<ffffffff8168b956>] wait_for_completion+0x116/0x170
> [ 1332.551874]  [<ffffffff810c4ec0>] ? wake_up_state+0x20/0x20
> [ 1332.551885]  [<ffffffffa06ae234>] bnx2fc_abts_cleanup+0x3d/0x62 
> [bnx2fc]
> [ 1332.551892]  [<ffffffffa06a5a80>] bnx2fc_eh_abort+0x470/0x580
> [bnx2fc]
> [ 1332.551900]  [<ffffffff814570af>] scsi_error_handler+0x59f/0x8b0
> [ 1332.551904]  [<ffffffff81456b10>] ? scsi_eh_get_sense+0x250/0x250
> [ 1332.551911]  [<ffffffff810b052f>] kthread+0xcf/0xe0
> [ 1332.551916]  [<ffffffff810b0460>] ?
> kthread_create_on_node+0x140/0x140
> [ 1332.551923]  [<ffffffff81696418>] ret_from_fork+0x58/0x90
> [ 1332.551928]  [<ffffffff810b0460>] ?
> kthread_create_on_node+0x140/0x140 

Reporting this when you found it would have been helpful ...

That said, it does look like a genuine hang in the workqueues, so it
rather invalidates the current patch set.

> To be honest, I'm reluctant to merge these patches on bnx2fc as the
> I/O path on this driver has been stable for quite some time and given
> that it's an older driver I'm not looking to make changes there.

OK, so find a way to achieve both sets of goals because there's a limit
to how long we allow "stable" drivers to hold up infrastructure changes
within the kernel.  The main goal of the current patch set is to remove
the cpu hotplug calls from the drivers because they want to remove them
from the kernel.  This is rather complex because you're using per cpu
work queues so you currently have to manage starting and stopping them
as the CPUs come up or go down ... getting rid of that for standard
kernel infrastructure will make the driver easier to keep in
maintenance mode for longer.

James

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Chad Dupuis]

On Tue, 9 May 2017, 11:18am, James Bottomley wrote:

> On Tue, 2017-05-09 at 10:17 -0400, Chad Dupuis wrote:
> > On Mon, 8 May 2017, 10:04pm, Martin K. Petersen wrote:
> > 
> > > 
> > > Sebastian,
> > > 
> > > > Martin, do you see any chance to get this merged? Chad replied to
> > the
> > > > list that he is going to test it on 2017-04-10, didn't respond to
> > the
> > > > ping 10 days later. The series stalled last time in the same way.
> > > 
> > > I am very reluctant to merge something when a driver has an active
> > > maintainer and that person has not acked the change.
> > > 
> > > That said, Chad: You have been sitting on this for quite a while.
> > Please
> > > make it a priority. In exchange for veto rights you do have to
> > provide
> > > timely feedback on anything that touches your driver.
> > > 
> > > Thanks!
> > > 
> > 
> > We did do some testing and hit a calltrace during device discovery:
> > 
> > [ 1332.551799] INFO: task scsi_eh_15:1970 blocked for more than 120 
> > seconds.
> > [ 1332.551804] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> > disables 
> > this message.
> > [ 1332.551807] scsi_eh_15      D ffff880823488c14     0  1970      2 
> > 0x00000080
> > [ 1332.551813]  ffff881053a17cb0 0000000000000046 ffff88084d693ec0 
> > ffff881053a17fd8
> > [ 1332.551817]  ffff881053a17fd8 ffff881053a17fd8 ffff88084d693ec0 
> > ffff880823488d48
> > [ 1332.551821]  ffff880823488d50 7fffffffffffffff ffff88084d693ec0 
> > ffff880823488c14
> > [ 1332.551825] Call Trace:
> > [ 1332.551838]  [<ffffffff8168b579>] schedule+0x29/0x70
> > [ 1332.551844]  [<ffffffff81688fc9>] schedule_timeout+0x239/0x2d0
> > [ 1332.551850]  [<ffffffff810882f8>] ? console_unlock+0x208/0x400
> > [ 1332.551855]  [<ffffffff810888b4>] ? vprintk_emit+0x3c4/0x510
> > [ 1332.551861]  [<ffffffff81096acb>] ?
> > lock_timer_base.isra.33+0x2b/0x50
> > [ 1332.551866]  [<ffffffff8168b956>] wait_for_completion+0x116/0x170
> > [ 1332.551874]  [<ffffffff810c4ec0>] ? wake_up_state+0x20/0x20
> > [ 1332.551885]  [<ffffffffa06ae234>] bnx2fc_abts_cleanup+0x3d/0x62 
> > [bnx2fc]
> > [ 1332.551892]  [<ffffffffa06a5a80>] bnx2fc_eh_abort+0x470/0x580
> > [bnx2fc]
> > [ 1332.551900]  [<ffffffff814570af>] scsi_error_handler+0x59f/0x8b0
> > [ 1332.551904]  [<ffffffff81456b10>] ? scsi_eh_get_sense+0x250/0x250
> > [ 1332.551911]  [<ffffffff810b052f>] kthread+0xcf/0xe0
> > [ 1332.551916]  [<ffffffff810b0460>] ?
> > kthread_create_on_node+0x140/0x140
> > [ 1332.551923]  [<ffffffff81696418>] ret_from_fork+0x58/0x90
> > [ 1332.551928]  [<ffffffff810b0460>] ?
> > kthread_create_on_node+0x140/0x140 
> 
> Reporting this when you found it would have been helpful ...
> 
> That said, it does look like a genuine hang in the workqueues, so it
> rather invalidates the current patch set.
> 
> > To be honest, I'm reluctant to merge these patches on bnx2fc as the
> > I/O path on this driver has been stable for quite some time and given
> > that it's an older driver I'm not looking to make changes there.
> 
> OK, so find a way to achieve both sets of goals because there's a limit
> to how long we allow "stable" drivers to hold up infrastructure changes
> within the kernel.  The main goal of the current patch set is to remove
> the cpu hotplug calls from the drivers because they want to remove them
> from the kernel.  This is rather complex because you're using per cpu
> work queues so you currently have to manage starting and stopping them
> as the CPUs come up or go down ... getting rid of that for standard
> kernel infrastructure will make the driver easier to keep in
> maintenance mode for longer.
> 
> James
> 

Ok, I believe I've found the issue here.  The machine that the test has 
performed on had many more possible CPUs than active CPUs.  We calculate 
which CPU to the work time on in bnx2fc_process_new_cqes() like this:

unsigned int cpu = wqe % num_possible_cpus();

Since not all CPUs are active, we were trying to schedule work on 
non-active CPUs which meant that the upper layers were never notified of 
the completion.  With this change:

diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c 
b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index c2288d6..6f08e43 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1042,7 +1042,12 @@ static int bnx2fc_process_new_cqes(struct 
bnx2fc_rport *tgt)
                        /* Pending work request completion */
                        struct bnx2fc_work *work = NULL;
                        struct bnx2fc_percpu_s *fps = NULL;
-                       unsigned int cpu = wqe % num_possible_cpus();
+                       unsigned int cpu = wqe % num_active_cpus();
+
+                       /* Sanity check cpu to make sure it's online */
+                       if (!cpu_active(cpu))
+                               /* Default to CPU 0 */
+                               cpu = 0;
 
                        work = bnx2fc_alloc_work(tgt, wqe);
                        if (work) {

The issue is fixed.

Sebastian, can you add this change to your patch set?

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-05-12 11:55:52 [-0400], Chad Dupuis wrote:
> Ok, I believe I've found the issue here.  The machine that the test has 
> performed on had many more possible CPUs than active CPUs.  We calculate 
> which CPU to the work time on in bnx2fc_process_new_cqes() like this:
> 
> unsigned int cpu = wqe % num_possible_cpus();
> 
> Since not all CPUs are active, we were trying to schedule work on 
> non-active CPUs which meant that the upper layers were never notified of 
> the completion.  With this change:
> 
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c 
> b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> index c2288d6..6f08e43 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> @@ -1042,7 +1042,12 @@ static int bnx2fc_process_new_cqes(struct 
> bnx2fc_rport *tgt)
>                         /* Pending work request completion */
>                         struct bnx2fc_work *work = NULL;
>                         struct bnx2fc_percpu_s *fps = NULL;
> -                       unsigned int cpu = wqe % num_possible_cpus();
> +                       unsigned int cpu = wqe % num_active_cpus();
> +
> +                       /* Sanity check cpu to make sure it's online */
> +                       if (!cpu_active(cpu))
> +                               /* Default to CPU 0 */
> +                               cpu = 0;
>  
>                         work = bnx2fc_alloc_work(tgt, wqe);
>                         if (work) {
> 
> The issue is fixed.
> 
> Sebastian, can you add this change to your patch set?

Are sure that you can reliably reproduce the issue and fix it with the
patch above? Because this patch:

diff --git a/init/main.c b/init/main.c
index b0c11cbf5ddf..483a971b1fd2 100644
--- a/init/main.c
+++ b/init/main.c
@@ -997,6 +997,12 @@ static int __ref kernel_init(void *unused)
              "See Linux Documentation/admin-guide/init.rst for guidance.");
 }
 
+static void workfn(struct work_struct *work)
+{
+       pr_err("%s() %d\n", __func__, raw_smp_processor_id());
+}
+static DECLARE_WORK(work, workfn);
+
 static noinline void __init kernel_init_freeable(void)
 {
        /*
@@ -1040,6 +1046,15 @@ static noinline void __init kernel_init_freeable(void)
 
        (void) sys_dup(0);
        (void) sys_dup(0);
+       {
+
+               cpu_down(3);
+               pr_err("%s() num possible: %d\n", __func__, num_possible_cpus());
+               pr_err("%s() num online  : %d\n", __func__, num_online_cpus());
+               pr_err("%s() 3 active    : %d\n", __func__, cpu_active(3));
+               schedule_work_on(3, &work);
+               ssleep(5);
+       }
        /*
         * check if there is an early userspace init.  If yes, let it do all
         * the work

produces this output:
[    1.960313] Unregister pv shared memory for cpu 3
[    1.997000] kernel_init_freeable() num possible: 8
[    1.998073] kernel_init_freeable() num online  : 7
[    1.999125] kernel_init_freeable() 3 active    : 0
[    2.000337] workfn() 1

which means, CPU3 is offline and work runs on CPU1 instead. So it does
already what you suggest except that chances are, that it is not run on
CPU0 in this case (but on another CPU).

So it either takes some time for wait_for_completion(&io_req->tm_done);
to come back _or_ there is a leak somewhere where a complete() is
somehow missing / racing against something.

Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Chad Dupuis]

On Wed, 17 May 2017, 11:01am, Sebastian Andrzej Siewior wrote:

> On 2017-05-12 11:55:52 [-0400], Chad Dupuis wrote:
> > Ok, I believe I've found the issue here.  The machine that the test has 
> > performed on had many more possible CPUs than active CPUs.  We calculate 
> > which CPU to the work time on in bnx2fc_process_new_cqes() like this:
> > 
> > unsigned int cpu = wqe % num_possible_cpus();
> > 
> > Since not all CPUs are active, we were trying to schedule work on 
> > non-active CPUs which meant that the upper layers were never notified of 
> > the completion.  With this change:
> > 
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c 
> > b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > index c2288d6..6f08e43 100644
> > --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > @@ -1042,7 +1042,12 @@ static int bnx2fc_process_new_cqes(struct 
> > bnx2fc_rport *tgt)
> >                         /* Pending work request completion */
> >                         struct bnx2fc_work *work = NULL;
> >                         struct bnx2fc_percpu_s *fps = NULL;
> > -                       unsigned int cpu = wqe % num_possible_cpus();
> > +                       unsigned int cpu = wqe % num_active_cpus();
> > +
> > +                       /* Sanity check cpu to make sure it's online */
> > +                       if (!cpu_active(cpu))
> > +                               /* Default to CPU 0 */
> > +                               cpu = 0;
> >  
> >                         work = bnx2fc_alloc_work(tgt, wqe);
> >                         if (work) {
> > 
> > The issue is fixed.
> > 
> > Sebastian, can you add this change to your patch set?
> 
> Are sure that you can reliably reproduce the issue and fix it with the
> patch above? Because this patch:

Yes it was reproducible each time we would start the FCoE interface.  With 
the above patch, our sanity test passed with no issues seen.

> 
> diff --git a/init/main.c b/init/main.c
> index b0c11cbf5ddf..483a971b1fd2 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -997,6 +997,12 @@ static int __ref kernel_init(void *unused)
>               "See Linux Documentation/admin-guide/init.rst for guidance.");
>  }
>  
> +static void workfn(struct work_struct *work)
> +{
> +       pr_err("%s() %d\n", __func__, raw_smp_processor_id());
> +}
> +static DECLARE_WORK(work, workfn);
> +
>  static noinline void __init kernel_init_freeable(void)
>  {
>         /*
> @@ -1040,6 +1046,15 @@ static noinline void __init kernel_init_freeable(void)
>  
>         (void) sys_dup(0);
>         (void) sys_dup(0);
> +       {
> +
> +               cpu_down(3);
> +               pr_err("%s() num possible: %d\n", __func__, num_possible_cpus());
> +               pr_err("%s() num online  : %d\n", __func__, num_online_cpus());
> +               pr_err("%s() 3 active    : %d\n", __func__, cpu_active(3));
> +               schedule_work_on(3, &work);
> +               ssleep(5);
> +       }
>         /*
>          * check if there is an early userspace init.  If yes, let it do all
>          * the work
> 
> produces this output:
> [    1.960313] Unregister pv shared memory for cpu 3
> [    1.997000] kernel_init_freeable() num possible: 8
> [    1.998073] kernel_init_freeable() num online  : 7
> [    1.999125] kernel_init_freeable() 3 active    : 0
> [    2.000337] workfn() 1
> 
> which means, CPU3 is offline and work runs on CPU1 instead. So it does
> already what you suggest except that chances are, that it is not run on
> CPU0 in this case (but on another CPU).
> 
> So it either takes some time for wait_for_completion(&io_req->tm_done);
> to come back _or_ there is a leak somewhere where a complete() is
> somehow missing / racing against something.
> 
> Sebastian
> 

Re: [PREEMPT-RT] [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-05-17 17:01:53 [+0200], To Chad Dupuis wrote:
> On 2017-05-12 11:55:52 [-0400], Chad Dupuis wrote:
> > Ok, I believe I've found the issue here.  The machine that the test has 
> > performed on had many more possible CPUs than active CPUs.  We calculate 
> > which CPU to the work time on in bnx2fc_process_new_cqes() like this:
> > 
> > unsigned int cpu = wqe % num_possible_cpus();
> > 
> > Since not all CPUs are active, we were trying to schedule work on 
> > non-active CPUs which meant that the upper layers were never notified of 
> > the completion.  With this change:
> > 
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c 
> > b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > index c2288d6..6f08e43 100644
> > --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > @@ -1042,7 +1042,12 @@ static int bnx2fc_process_new_cqes(struct 
> > bnx2fc_rport *tgt)
> >                         /* Pending work request completion */
> >                         struct bnx2fc_work *work = NULL;
> >                         struct bnx2fc_percpu_s *fps = NULL;
> > -                       unsigned int cpu = wqe % num_possible_cpus();
> > +                       unsigned int cpu = wqe % num_active_cpus();
> > +
> > +                       /* Sanity check cpu to make sure it's online */
> > +                       if (!cpu_active(cpu))
> > +                               /* Default to CPU 0 */
> > +                               cpu = 0;
> >  
> >                         work = bnx2fc_alloc_work(tgt, wqe);
> >                         if (work) {
> > 
> > The issue is fixed.
> > 
> > Sebastian, can you add this change to your patch set?
> 
> Are sure that you can reliably reproduce the issue and fix it with the
> patch above? Because this patch:

oh. Okay. Now it clicked. It can fix the issue but it is still possible,
that CPU0 goes down between your check for it and schedule_work_on()
returning. Let my think of something…

Sebastian

Re: [PREEMPT-RT] [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Sebastian Andrzej Siewior]

On 2017-05-17 17:07:34 [+0200], To Chad Dupuis wrote:
> > > Sebastian, can you add this change to your patch set?
> > 
> > Are sure that you can reliably reproduce the issue and fix it with the
> > patch above? Because this patch:
> 
> oh. Okay. Now it clicked. It can fix the issue but it is still possible,
> that CPU0 goes down between your check for it and schedule_work_on()
> returning. Let my think of something…

Oh wait. I already thought about this: it may take bnx2fc_percpu from
CPU7 and run the worker on CPU3. The job isn't lost, because the worker
does:

| static void bnx2fc_percpu_io_work(struct work_struct *work_s)
| {
|         struct bnx2fc_percpu_s *p;
 …
|         p = container_of(work_s, struct bnx2fc_percpu_s, work);
| 
|         spin_lock_bh(&p->fp_work_lock);

and so will access bnx2fc_percpu of CPU7 running on CPU3. So I *think*
that your patch should make no difference and there should be no leak if
schedule_work_on() is invoked on an offline CPU.
 
Sebastian

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

[Martin K. Petersen]

Chad,

> To be honest, I'm reluctant to merge these patches on bnx2fc as the
> I/O path on this driver has been stable for quite some time and given
> that it's an older driver I'm not looking to make changes there.

I understand that the driver is in maintenance mode. However, the Linux
kernel and its interfaces are not. We do not offer any guarantees in
that department. So even if you do not intend to add new features to
bnx2fc, you will still need to keep it current wrt. the ongoing changes
in kernel interfaces (for better and for worse).

-- 
Martin K. Petersen	Oracle Linux Engineering