nvmet_fc: Reduce work_q count
Subsystems (3)
PATCH 1/1 nvmet_fc: Reduce work_q count
Instead of a work_q per controller queue, use a global workqueue.
--- drivers/nvme/target/fc.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)
drivers/nvme/target/fc.c +22 -18
@@ -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
PATCH 1/1 nvmet_fc: Reduce work_q count
Quoted 1 line · James Smart
Instead of a
work_qper 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
PATCH 1/1 nvmet_fc: Reduce work_q count
Quoted 9 lines · Johannes Thumshirn
Quoted 1 line · James Smart
Instead of a
work_qper 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
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.htmlThis 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
Sagi Grimberg
maintainerPATCH 1/1 nvmet_fc: Reduce work_q count
In reply to James Smartdrivers/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...
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_UNBOUNDif 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
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.
Sagi Grimberg
maintainerPATCH 1/1 nvmet_fc: Reduce work_q count
Quoted 4 lines
And that's exactly what plain
queue_workwill 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().
PATCH 1/1 nvmet_fc: Reduce work_q count
In reply to Christoph HellwigQuoted 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_workwill 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