Skip to contentSkip to conversation

nvmet_fc: Reduce work_q count

PATCH New James Smart 4 participants 9 messages Sep 27, 2017 – Oct 3, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Instead of a work_q per controller queue, use a global workqueue.

Signed-off-by James Smart <james.smart at broadcom.com>
---
 drivers/nvme/target/fc.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)
drivers/nvme/target/fc.c +22 -18
index 58e010bdda3e..75b1752e8375 100644
@@ -139,7 +139,6 @@ struct nvmet_fc_tgt_queue {
 	struct list_head		fod_list;
 	struct list_head		pending_cmd_list;
 	struct list_head		avail_defer_list;
-	struct workqueue_struct		*work_q;
 	struct kref			ref;
 } __aligned(sizeof(unsigned long long));
 
@@ -220,6 +219,7 @@ static DEFINE_SPINLOCK(nvmet_fc_tgtlock);
 static LIST_HEAD(nvmet_fc_target_list);
 static DEFINE_IDA(nvmet_fc_tgtport_cnt);
 
+static struct workqueue_struct *nvmet_fc_io_wq;
 
 static void nvmet_fc_handle_ls_rqst_work(struct work_struct *work);
 static void nvmet_fc_handle_fcp_rqst_work(struct work_struct *work);
@@ -496,6 +496,7 @@ nvmet_fc_queue_fcp_req(struct nvmet_fc_tgtport *tgtport,
 		       struct nvmefc_tgt_fcp_req *fcpreq)
 {
 	struct nvmet_fc_fcp_iod *fod = fcpreq->nvmet_fc_private;
+	int cpu;
 
 	/*
 	 * put all admin cmds on hw queue id 0. All io commands go to
@@ -504,9 +505,11 @@ nvmet_fc_queue_fcp_req(struct nvmet_fc_tgtport *tgtport,
 	fcpreq->hwqid = queue->qid ?
 			((queue->qid - 1) % tgtport->ops->max_hw_queues) : 0;
 
-	if (tgtport->ops->target_features & NVMET_FCTGTFEAT_CMD_IN_ISR)
-		queue_work_on(queue->cpu, queue->work_q, &fod->work);
-	else
+	if (tgtport->ops->target_features & NVMET_FCTGTFEAT_CMD_IN_ISR) {
+		cpu = (queue->cpu == WORK_CPU_UNBOUND) ?
+					get_cpu() : queue->cpu;
+		queue_work_on(cpu, nvmet_fc_io_wq, &fod->work);
+	} else
 		nvmet_fc_handle_fcp_rqst(tgtport, fod);
 }
 
@@ -620,12 +623,6 @@ nvmet_fc_alloc_target_queue(struct nvmet_fc_tgt_assoc *assoc,
 	if (!nvmet_fc_tgt_a_get(assoc))
 		goto out_free_queue;
 
-	queue->work_q = alloc_workqueue("ntfc%d.%d.%d", 0, 0,
-				assoc->tgtport->fc_target_port.port_num,
-				assoc->a_id, qid);
-	if (!queue->work_q)
-		goto out_a_put;
-
 	queue->fod = (struct nvmet_fc_fcp_iod *)&queue[1];
 	queue->qid = qid;
 	queue->sqsize = sqsize;
@@ -657,8 +654,6 @@ nvmet_fc_alloc_target_queue(struct nvmet_fc_tgt_assoc *assoc,
 
 out_fail_iodlist:
 	nvmet_fc_destroy_fcp_iodlist(assoc->tgtport, queue);
-	destroy_workqueue(queue->work_q);
-out_a_put:
 	nvmet_fc_tgt_a_put(assoc);
 out_free_queue:
 	kfree(queue);
@@ -681,8 +676,6 @@ nvmet_fc_tgt_queue_free(struct kref *ref)
 
 	nvmet_fc_tgt_a_put(queue->assoc);
 
-	destroy_workqueue(queue->work_q);
-
 	kfree(queue);
 }
 
@@ -765,7 +758,7 @@ nvmet_fc_delete_target_queue(struct nvmet_fc_tgt_queue *queue)
 	}
 	spin_unlock_irqrestore(&queue->qlock, flags);
 
-	flush_workqueue(queue->work_q);
+	flush_workqueue(nvmet_fc_io_wq);
 
 	if (disconnect)
 		nvmet_sq_destroy(&queue->nvme_sq);
@@ -2052,11 +2045,15 @@ nvmet_fc_xmt_fcp_op_done(struct nvmefc_tgt_fcp_req *fcpreq)
 {
 	struct nvmet_fc_fcp_iod *fod = fcpreq->nvmet_fc_private;
 	struct nvmet_fc_tgt_queue *queue = fod->queue;
+	int cpu;
 
-	if (fod->tgtport->ops->target_features & NVMET_FCTGTFEAT_OPDONE_IN_ISR)
+	if (fod->tgtport->ops->target_features &
+				NVMET_FCTGTFEAT_OPDONE_IN_ISR) {
+		cpu = (queue->cpu == WORK_CPU_UNBOUND) ?
+					get_cpu() : queue->cpu;
 		/* context switch so completion is not in ISR context */
-		queue_work_on(queue->cpu, queue->work_q, &fod->done_work);
-	else
+		queue_work_on(cpu, nvmet_fc_io_wq, &fod->done_work);
+	} else
 		nvmet_fc_fod_op_done(fod);
 }
 
@@ -2558,6 +2555,11 @@ static struct nvmet_fabrics_ops nvmet_fc_tgt_fcp_ops = {
 
 static int __init nvmet_fc_init_module(void)
 {
+	nvmet_fc_io_wq = alloc_workqueue("nvmet-fc",
+				WQ_MEM_RECLAIM | WQ_CPU_INTENSIVE, 0);
+	if (!nvmet_fc_io_wq)
+		return -ENOMEM;
+
 	return nvmet_register_transport(&nvmet_fc_tgt_fcp_ops);
 }
 
@@ -2569,6 +2571,8 @@ static void __exit nvmet_fc_exit_module(void)
 
 	nvmet_unregister_transport(&nvmet_fc_tgt_fcp_ops);
 
+	destroy_workqueue(nvmet_fc_io_wq);
+
 	ida_destroy(&nvmet_fc_tgtport_cnt);
 }
 
Signature · 1 line
2.13.1
#1 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Quoted 1 line · James Smart

Instead of a work_q per controller queue, use a global workqueue.

Could you be a bit more specific on why you want to use one global nvme-fc workqueue?

Thanks,
Johannes

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at 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
#2 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Quoted 9 lines · Johannes Thumshirn
Quoted 1 line · James Smart

Instead of a work_q per controller queue, use a global workqueue.

Could you be a bit more specific on why you want to use one global nvme-fc workqueue?

Thanks,
Johannes

Based on the recommendation from Christoph:
http://lists.infradead.org/pipermail/linux-nvme/2017-May/010388.html

This patch has gone through several itterations with the last recommendation being the single work_q.

-- james

#3 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Quoted 5 lines · James Smart

Based on the recommendation from Christoph:
http://lists.infradead.org/pipermail/linux-nvme/2017-May/010388.html

This patch has gone through several itterations with the last recommendation being the single work_q.

Ah OK thanks

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at 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
maintainer
#4 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

In reply to James Smart

drivers/nvme/target/fc.c nvmet_fc_queue_fcp_req(struct nvmet_fc_tgtport *tgtport, · line 510

Quoted 11 lines
drivers/nvme/target/fc.c +3 -3
@@ -504,9 +505,11 @@ nvmet_fc_queue_fcp_req(struct nvmet_fc_tgtport *tgtport,
  	fcpreq->hwqid = queue->qid ?
  			((queue->qid - 1) % tgtport->ops->max_hw_queues) : 0;
 
-	if (tgtport->ops->target_features & NVMET_FCTGTFEAT_CMD_IN_ISR)
-		queue_work_on(queue->cpu, queue->work_q, &fod->work);
-	else
+	if (tgtport->ops->target_features & NVMET_FCTGTFEAT_CMD_IN_ISR) {
+		cpu = (queue->cpu == WORK_CPU_UNBOUND) ?
+					get_cpu() : queue->cpu;

Why doing this? Why not let workqueue choose it for you? it will attempt your local cpu, but if its busy it will get someone else...

You can simply queue it with WORK_CPU_UNBOUND if it happened to be set this way...

#5 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Quoted 15 lines · Sagi Grimberg
Quoted 11 lines
(quoted diff) +0 -0
@@ -504,9 +505,11 @@ nvmet_fc_queue_fcp_req(struct nvmet_fc_tgtport 
 *tgtport,

????? fcpreq->hwqid = queue->qid ?
????????????? ((queue->qid - 1) % tgtport->ops->max_hw_queues) : 0; -??? if (tgtport->ops->target_features & NVMET_FCTGTFEAT_CMD_IN_ISR) -??????? queue_work_on(queue->cpu, queue->work_q, &fod->work); -??? else
+??? if (tgtport->ops->target_features & NVMET_FCTGTFEAT_CMD_IN_ISR) { +??????? cpu = (queue->cpu == WORK_CPU_UNBOUND) ? +??????????????????? get_cpu() : queue->cpu;

Why doing this? Why not let workqueue choose it for you? it will attempt your local cpu, but if its busy it will get someone else...

Quoted 2 lines

You can simply queue it with WORK_CPU_UNBOUND if it happened to be set this way...

Because we're still trying to get cpu affinity on the connection resources that will be shared by the io on the same queue.

And we're trying to spread out the cpu utilization rather than ganging up on a few cpus (e.g. the lldd may have (quite a few) fewer msix than there are cpus).

But... as I now got lpfc to move to a softirq model (still needs to be merged), and cavium, as the other FC adapter was already there. I'm going to come around and delete these "IN_ISR" cases. I was going to delay that but perhaps nows the time to do so.

-- james

#6 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Quoted 2 lines · James Smart

Because we're still trying to get cpu affinity on the connection resources that will be shared by the io on the same queue.

And that's exactly what plain queue_work will give you. If it doesn't do exactly what your open code logic does it has a bug that needs to be fixed.

maintainer
#7 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

Quoted 4 lines

And that's exactly what plain queue_work will give you. If it doesn't do exactly what your open code logic does it has a bug that needs to be fixed.

I also think you are missing a put_cpu().

#8 lore raw

PATCH 1/1 nvmet_fc: Reduce work_q count

In reply to Christoph Hellwig
Quoted 8 lines · Christoph Hellwig
Quoted 2 lines · James Smart

Because we're still trying to get cpu affinity on the connection resources that will be shared by the io on the same queue.

And that's exactly what plain queue_work will give you. If it doesn't do exactly what your open code logic does it has a bug that needs to be fixed.

I don't understand how queuing work alone, which can go to any cpu (current preferred) helps with avoiding cross-cpu access to the same transport/lldd hw queue data structures.

But, as stated. A moot point now. I'll remove this whole IN_ISR, thus work_q item, shortly.

-- james