[REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[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 kworker 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 | 121 +++-------------------------------------
 3 files changed, 53 insertions(+), 180 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index ed7f3228e234..78cdc493bab5 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 c8b410c24cf0..976a6bc86d39 100644
--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -70,14 +70,6 @@ u64 iscsi_error_mask = 0x00;
 
 DEFINE_PER_CPU(struct bnx2i_percpu_s, bnx2i_percpu);
 
-static int bnx2i_cpu_callback(struct notifier_block *nfb,
-			      unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2i_cpu_notifier = {
-	.notifier_call = bnx2i_cpu_callback,
-};
-
-
 /**
  * bnx2i_identify_device - identifies NetXtreme II device type
  * @hba: 		Adapter structure pointer
@@ -410,93 +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);
-}
-
-
-/**
- * bnx2i_cpu_callback - Handler for CPU hotplug events
- *
- * @nfb:	The callback data block
- * @action:	The event triggering the callback
- * @hcpu:	The index of the CPU that the event is for
- *
- * This creates or destroys per-CPU data for iSCSI
- *
- * Returns NOTIFY_OK always.
- */
-static int bnx2i_cpu_callback(struct notifier_block *nfb,
-			      unsigned long action, void *hcpu)
-{
-	unsigned cpu = (unsigned long)hcpu;
-
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
-		printk(KERN_INFO "bnx2i: CPU %x online: Create Rx thread\n",
-			cpu);
-		bnx2i_percpu_thread_create(cpu);
-		break;
-	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
-		printk(KERN_INFO "CPU %x offline: Remove Rx thread\n", cpu);
-		bnx2i_percpu_thread_destroy(cpu);
-		break;
-	default:
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-
 /**
  * bnx2i_mod_init - module init entry point
  *
@@ -533,22 +438,12 @@ 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);
 	}
 
-	cpu_notifier_register_begin();
-
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_create(cpu);
-
-	/* Initialize per CPU interrupt thread */
-	__register_hotcpu_notifier(&bnx2i_cpu_notifier);
-
-	cpu_notifier_register_done();
-
 	return 0;
 
 unreg_xport:
@@ -587,14 +482,12 @@ static void __exit bnx2i_mod_exit(void)
 	}
 	mutex_unlock(&bnx2i_dev_lock);
 
-	cpu_notifier_register_begin();
+	for_each_possible_cpu(cpu) {
+		struct bnx2i_percpu_s *p;
 
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_destroy(cpu);
-
-	__unregister_hotcpu_notifier(&bnx2i_cpu_notifier);
-
-	cpu_notifier_register_done();
+		p = per_cpu_ptr(&bnx2i_percpu, cpu);
+		flush_work(&p->work);
+	}
 
 	iscsi_unregister_transport(&bnx2i_iscsi_transport);
 	cnic_unregister_driver(CNIC_ULP_ISCSI);
-- 
2.9.3

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

[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 kworker 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 | 157 ++++++--------------------------------
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  22 +++---
 3 files changed, 32 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd4eb4e41b2..fdd89935cac7 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 a5052dd8d7e6..c8673e3c4d35 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -127,13 +127,6 @@ 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.");
 
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-			     unsigned long action, void *hcpu);
-/* notification function for CPU hotplug events */
-static struct notifier_block bnx2fc_cpu_notifier = {
-	.notifier_call = bnx2fc_cpu_callback,
-};
-
 static inline struct net_device *bnx2fc_netdev(const struct fc_lport *lport)
 {
 	return ((struct bnx2fc_interface *)
@@ -621,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
  */
-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)
@@ -2570,91 +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);
-}
-
-/**
- * bnx2fc_cpu_callback - Handler for CPU hotplug events
- *
- * @nfb:    The callback data block
- * @action: The event triggering the callback
- * @hcpu:   The index of the CPU that the event is for
- *
- * This creates or destroys per-CPU data for fcoe
- *
- * Returns NOTIFY_OK always.
- */
-static int bnx2fc_cpu_callback(struct notifier_block *nfb,
-			     unsigned long action, void *hcpu)
-{
-	unsigned cpu = (unsigned long)hcpu;
-
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
-		printk(PFX "CPU %x online: Create Rx thread\n", cpu);
-		bnx2fc_percpu_thread_create(cpu);
-		break;
-	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
-		printk(PFX "CPU %x offline: Remove Rx thread\n", cpu);
-		bnx2fc_percpu_thread_destroy(cpu);
-		break;
-	default:
-		break;
-	}
-	return NOTIFY_OK;
-}
-
 static int bnx2fc_slave_configure(struct scsi_device *sdev)
 {
 	if (!bnx2fc_queue_depth)
@@ -2722,19 +2623,9 @@ 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);
 	}
 
-	cpu_notifier_register_begin();
-
-	for_each_online_cpu(cpu) {
-		bnx2fc_percpu_thread_create(cpu);
-	}
-
-	/* Initialize per CPU interrupt thread */
-	__register_hotcpu_notifier(&bnx2fc_cpu_notifier);
-
-	cpu_notifier_register_done();
-
 	cnic_register_driver(CNIC_ULP_FCOE, &bnx2fc_cnic_cb);
 
 	return 0;
@@ -2798,17 +2689,13 @@ static void __exit bnx2fc_mod_exit(void)
 	if (l2_thread)
 		kthread_stop(l2_thread);
 
-	cpu_notifier_register_begin();
+	for_each_possible_cpu(cpu) {
+		struct bnx2fc_percpu_s *p;
 
-	/* Destroy per cpu threads */
-	for_each_online_cpu(cpu) {
-		bnx2fc_percpu_thread_destroy(cpu);
+		p = per_cpu_ptr(&bnx2fc_percpu, cpu);
+		flush_work(&p->work);
 	}
 
-	__unregister_hotcpu_notifier(&bnx2fc_cpu_notifier);
-
-	cpu_notifier_register_done();
-
 	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 28c671b609b2..acad76d3a81b 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.9.3

[REPOST 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_els.c  |  4 ++--
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 14 +++++++-------
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |  8 +++-----
 drivers/scsi/bnx2fc/bnx2fc_io.c   |  2 +-
 5 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index fdd89935cac7..64308584405d 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_els.c b/drivers/scsi/bnx2fc/bnx2fc_els.c
index 5beea776b9f5..68ca518d34b0 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_els.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_els.c
@@ -254,7 +254,7 @@ int bnx2fc_send_rls(struct bnx2fc_rport *tgt, struct fc_frame *fp)
 	return rc;
 }
 
-void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
+static void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
 {
 	struct bnx2fc_mp_req *mp_req;
 	struct fc_frame_header *fc_hdr, *fh;
@@ -364,7 +364,7 @@ void bnx2fc_srr_compl(struct bnx2fc_els_cb_arg *cb_arg)
 	kref_put(&orig_io_req->refcount, bnx2fc_cmd_release);
 }
 
-void bnx2fc_rec_compl(struct bnx2fc_els_cb_arg *cb_arg)
+static void bnx2fc_rec_compl(struct bnx2fc_els_cb_arg *cb_arg)
 {
 	struct bnx2fc_cmd *orig_io_req, *new_io_req;
 	struct bnx2fc_cmd *rec_req;
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index c8673e3c4d35..f2be89984990 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);
@@ -1396,7 +1396,7 @@ static struct bnx2fc_hba *bnx2fc_hba_create(struct cnic_dev *cnic)
 	return NULL;
 }
 
-struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
+static struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
 				      struct net_device *netdev,
 				      enum fip_state fip_mode)
 {
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index acad76d3a81b..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;
@@ -994,7 +992,7 @@ void bnx2fc_arm_cq(struct bnx2fc_rport *tgt)
 
 }
 
-struct bnx2fc_work *bnx2fc_alloc_work(struct bnx2fc_rport *tgt, u16 wqe)
+static struct bnx2fc_work *bnx2fc_alloc_work(struct bnx2fc_rport *tgt, u16 wqe)
 {
 	struct bnx2fc_work *work;
 	work = kzalloc(sizeof(struct bnx2fc_work), GFP_ATOMIC);
@@ -1007,7 +1005,7 @@ 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;
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 8f24d60f09d7..f501095f91ac 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1079,7 +1079,7 @@ int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
 	return bnx2fc_initiate_tmf(sc_cmd, FCP_TMF_LUN_RESET);
 }
 
-int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
+static int bnx2fc_abts_cleanup(struct bnx2fc_cmd *io_req)
 {
 	struct bnx2fc_rport *tgt = io_req->tgt;
 	int rc = SUCCESS;
-- 
2.9.3

[REPOST 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 f501095f91ac..634e1e539850 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.9.3

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

[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 kworker 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 kworker 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 | 57 +++++++++------------------------------
 include/scsi/libfcoe.h            |  1 -
 2 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index f2be89984990..64d1ed0e7c5f 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);
 }
 
 
@@ -2574,7 +2568,6 @@ static int bnx2fc_slave_configure(struct scsi_device *sdev)
 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;
@@ -2607,17 +2600,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);
@@ -2630,8 +2613,6 @@ static int __init bnx2fc_mod_init(void)
 
 	return 0;
 
-free_wq:
-	destroy_workqueue(bnx2fc_wq);
 release_bt:
 	bnx2fc_release_transport();
 detach_ft:
@@ -2644,9 +2625,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 +2654,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.9.3

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

[Johannes Thumshirn]

On Wed, Aug 17, 2016 at 12:09:59PM +0200, 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 kworker 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>

I'm sorry it took so long but for the whole series:
Tested-by: Johannes Thumshirn <jth@kernel.org>
Acked-by: Johannes Thumshirn <jth@kernel.org>

Testing was conducted by booting from a QLogic BCM57840 CNA.

	Johannes
-- 
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: [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Martin K. Petersen]

>>>>> "Johannes" == Johannes Thumshirn <jthumshirn@suse.de> writes:

>> On Wed, Aug 17, 2016 at 12:09:59PM +0200, 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.

Johannes> I'm sorry it took so long but for the whole series: Tested-by:
Johannes> Johannes Thumshirn <jth@kernel.org> Acked-by: Johannes
Johannes> Thumshirn <jth@kernel.org>

Johannes> Testing was conducted by booting from a QLogic BCM57840 CNA.

Chad?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Chad Dupuis]

On Fri, 9 Sep 2016, 11:28am -0000, Martin K. Petersen wrote:

> >>>>> "Johannes" == Johannes Thumshirn <jthumshirn@suse.de> writes:
> 
> >> On Wed, Aug 17, 2016 at 12:09:59PM +0200, 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.
> 
> Johannes> I'm sorry it took so long but for the whole series: Tested-by:
> Johannes> Johannes Thumshirn <jth@kernel.org> Acked-by: Johannes
> Johannes> Thumshirn <jth@kernel.org>
> 
> Johannes> Testing was conducted by booting from a QLogic BCM57840 CNA.
> 
> Chad?
> 

We're regression testing the patches now.  Please hold off on applying 
them. 

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

[Martin K. Petersen]

>>>>> "Chad" == Chad Dupuis <chad.dupuis@cavium.com> writes:

Johannes> Testing was conducted by booting from a QLogic BCM57840 CNA.

Chad> We're regression testing the patches now.  Please hold off on
Chad> applying them.

OK. I will wait.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Sebastian Andrzej Siewior]

On 2016-09-14 13:25:12 [-0400], Martin K. Petersen wrote:
> >>>>> "Chad" == Chad Dupuis <chad.dupuis@cavium.com> writes:
> 
> Chad> We're regression testing the patches now.  Please hold off on
> Chad> applying them.
> 
> OK. I will wait.

Chad, any updates on this tests?

Sebastian

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

[Christoph Hellwig]

It seems like the whole damn driver should just use threaded interrupts.
Of course it's a giant beast and not just the iSCSI one.  But even
if we don't go all the way I'd much prefer workqueues.  kthread work
is simply the worst API ever and I'd prefer to not have it proliferate.

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Thomas Gleixner]

On Mon, 7 Nov 2016, Christoph Hellwig wrote:

> It seems like the whole damn driver should just use threaded interrupts.
> Of course it's a giant beast and not just the iSCSI one.  But even
> if we don't go all the way I'd much prefer workqueues.  kthread work
> is simply the worst API ever and I'd prefer to not have it proliferate.

That's what the patch is doing. It uses INIT_WORK() and
schedule_work[_on](). I can't find any reference to kthread work.

Thanks,

	tglx

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Christoph Hellwig]

On Mon, Nov 07, 2016 at 05:31:03PM +0100, Thomas Gleixner wrote:
> On Mon, 7 Nov 2016, Christoph Hellwig wrote:
> 
> > It seems like the whole damn driver should just use threaded interrupts.
> > Of course it's a giant beast and not just the iSCSI one.  But even
> > if we don't go all the way I'd much prefer workqueues.  kthread work
> > is simply the worst API ever and I'd prefer to not have it proliferate.
> 
> That's what the patch is doing. It uses INIT_WORK() and
> schedule_work[_on](). I can't find any reference to kthread work.

You're right it does - between the incorrect subject and the fact
that it still keeps the linked list of items arounds instead of fully
using the workqueue infrastructure I was a bit confused before my
first coffee this morning.  Same applies to patch 2.

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-11-07 17:38:48 [+0100], Christoph Hellwig wrote:
> You're right it does - between the incorrect subject and the fact
> that it still keeps the linked list of items arounds instead of fully
> using the workqueue infrastructure I was a bit confused before my
> first coffee this morning.  Same applies to patch 2.

sorry for the confusion in the subject. If I remember correctly you said
that we may not have enough room for a larger / work_struct struct and I
should keep the list for now.

Sebastian

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Christoph Hellwig]

On Mon, Nov 07, 2016 at 05:46:29PM +0100, Sebastian Andrzej Siewior wrote:
> sorry for the confusion in the subject. If I remember correctly you said
> that we may not have enough room for a larger / work_struct struct and I
> should keep the list for now.

But that was for libfc where it only has a tiny bit of private data
i nthe skb - this is for cnix which has plenty of space, and in fact
probably should do the workqueue offload in the core before invoking
the protocol drivers.

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Sebastian Andrzej Siewior]

On 2016-11-07 17:48:46 [+0100], Christoph Hellwig wrote:
> On Mon, Nov 07, 2016 at 05:46:29PM +0100, Sebastian Andrzej Siewior wrote:
> > sorry for the confusion in the subject. If I remember correctly you said
> > that we may not have enough room for a larger / work_struct struct and I
> > should keep the list for now.
> 
> But that was for libfc where it only has a tiny bit of private data
> i nthe skb - this is for cnix which has plenty of space, and in fact

Right. now I remember.

> probably should do the workqueue offload in the core before invoking
> the protocol drivers.

So we keep things as they are right now or are we getting also rid of
the internal list? This was tested by Johannes and Chad (claimed to do
testing) and the removal the internal list afterwards shouldn't be that
big deal. Not sure what the last point is about.

Sebastian

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Christoph Hellwig]

On Mon, Nov 07, 2016 at 06:04:21PM +0100, Sebastian Andrzej Siewior wrote:
> So we keep things as they are right now or are we getting also rid of
> the internal list? This was tested by Johannes and Chad (claimed to do
> testing)

IFF the patches actually are tested as-is let's get them in.  I don't
think they are how we want the code to like in the long run, though.

> 
> Not sure what the last point is about.

It's about bnx2i and bnx2fc being clients of the main cnix driver,
which does the actual interrupt handling.  It seems we should actually
offloading things to a workqueue or threaded interrupts in that core
module instead of duplicating it in the clients.

Re: [PREEMPT-RT] [REPOST PATCH 1/5] scsi: bnx2i: convert to kworker

[Johannes Thumshirn]

On Mon, Nov 07, 2016 at 08:10:25PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 07, 2016 at 06:04:21PM +0100, Sebastian Andrzej Siewior wrote:
> > So we keep things as they are right now or are we getting also rid of
> > the internal list? This was tested by Johannes and Chad (claimed to do
> > testing)
> 
> IFF the patches actually are tested as-is let's get them in.  I don't
> think they are how we want the code to like in the long run, though.

I'm definitively sure I did test the bnx2fc patches. I'm not 100% sure I did
test the bnx2i patches as well (althoug I wrote "for the whole series").
Testing was IIRC booting from the FCoE LUN and the doing a series of fio tests
for about an hour.

Byte,
	Johannes

-- 
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