Re: [PATCH v3] scsi:tcmu: switch tcmu completion path to work queue context

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bodo,

在 2022/5/15 02:32, Bodo Stroesser 写道:
Hi Liu,

thank you for the patch.

To be honest I'm not sure this is the right way to boost performance.
Since tcmu still uses the cmdr_lock to serialize all work in kernel,
the only effect AFAICS is that userspace returns faster from the
uio_write(). This probably just speeds up userspace if it runs as one
thread only.
Do you also have measurement results for small IOs?

Well, you are right, there`s no obvious performance change for small IO.


I'd prefer to first finalize and send my patch series that makes tcmu
release cmdr_lock while copying data - in READ direction as well as in
WRITE direction. Using LIO core's completion wq allows to do multiple
data copies in both direction in parallel, if the fabric allows to
handle multiple incoming WRITES in parallel.

With that series in place it would be good to have another try with
your change. I)f it still can boost throughput, I'll be happy to
accept it.

Your idea is better, with your patches, this patch wont have much impact,

but I still do some tests.

thanks,

Guixin Liu


Bodo


On 11.05.22 04:08, Guixin Liu wrote:
Currently, tcmu do completion in uio_write context, the userspace
will wait tcmu complete synchronously, switch completion path to
work queue context for better performance.

Use tcmu + file to evaluate performance,
fio job: fio -filename=/dev/sdb  -direct=1 -size=2G -name=1 -thread
-runtime=60 -time_based  -rw=randwrite -numjobs=16 -iodepth=16 -bs=128k

Without this patch:
    READ: bw=2775MiB/s (2910MB/s), 173MiB/s-174MiB/s
(181MB/s-183MB/s), io=163GiB (175GB), run=60001-60001msec

With this patch:
    READ: bw=3333MiB/s (3495MB/s), 208MiB/s-209MiB/s
(218MB/s-219MB/s), io=195GiB (210GB), run=60001-60001msec

Signed-off-by: Guixin Liu <kanie@xxxxxxxxxxxxxxxxx>
---
  drivers/target/target_core_user.c | 155 +++++++++++++++++++++-----------------
  1 file changed, 87 insertions(+), 68 deletions(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index fd7267b..6fd514d99 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -168,6 +168,8 @@ struct tcmu_dev {
      char dev_config[TCMU_CONFIG_LEN];
        int nl_reply_supported;
+
+    struct work_struct complete_work;
  };
    #define TCMU_DEV(_se_dev) container_of(_se_dev, struct tcmu_dev, se_dev)
@@ -223,6 +225,7 @@ struct tcmu_tmr {
    static atomic_t global_page_count = ATOMIC_INIT(0);
  static struct delayed_work tcmu_unmap_work;
+static struct workqueue_struct *tcmu_comp_wq;
  static int tcmu_global_max_pages = TCMU_GLOBAL_MAX_PAGES_DEF;
    static int tcmu_set_global_max_data_area(const char *str,
@@ -1596,6 +1599,76 @@ static void tcmu_detach_hba(struct se_hba *hba)
      hba->hba_ptr = NULL;
  }
  +static void run_qfull_queue(struct tcmu_dev *udev, bool fail)
+{
+    struct tcmu_cmd *tcmu_cmd, *tmp_cmd;
+    LIST_HEAD(cmds);
+    sense_reason_t scsi_ret;
+    int ret;
+
+    if (list_empty(&udev->qfull_queue))
+        return;
+
+    pr_debug("running %s's cmdr queue forcefail %d\n", udev->name, fail);
+
+    list_splice_init(&udev->qfull_queue, &cmds);
+
+    list_for_each_entry_safe(tcmu_cmd, tmp_cmd, &cmds, queue_entry) {
+        list_del_init(&tcmu_cmd->queue_entry);
+
+        pr_debug("removing cmd %p on dev %s from queue\n",
+             tcmu_cmd, udev->name);
+
+        if (fail) {
+            /*
+             * We were not able to even start the command, so
+             * fail with busy to allow a retry in case runner
+             * was only temporarily down. If the device is being
+             * removed then LIO core will do the right thing and
+             * fail the retry.
+             */
+            tcmu_cmd->se_cmd->priv = NULL;
+            target_complete_cmd(tcmu_cmd->se_cmd, SAM_STAT_BUSY);
+            tcmu_free_cmd(tcmu_cmd);
+            continue;
+        }
+
+        ret = queue_cmd_ring(tcmu_cmd, &scsi_ret);
+        if (ret < 0) {
+            pr_debug("cmd %p on dev %s failed with %u\n",
+                 tcmu_cmd, udev->name, scsi_ret);
+            /*
+             * Ignore scsi_ret for now. target_complete_cmd
+             * drops it.
+             */
+            tcmu_cmd->se_cmd->priv = NULL;
+            target_complete_cmd(tcmu_cmd->se_cmd,
+                        SAM_STAT_CHECK_CONDITION);
+            tcmu_free_cmd(tcmu_cmd);
+        } else if (ret > 0) {
+            pr_debug("ran out of space during cmdr queue run\n");
+            /*
+             * cmd was requeued, so just put all cmds back in
+             * the queue
+             */
+            list_splice_tail(&cmds, &udev->qfull_queue);
+            break;
+        }
+    }
+
+    tcmu_set_next_deadline(&udev->qfull_queue, &udev->qfull_timer);
+}
+
+static void tcmu_complete_work(struct work_struct *work)
+{
+    struct tcmu_dev *udev = container_of(work, struct tcmu_dev, complete_work);
+
+    mutex_lock(&udev->cmdr_lock);
+    if (tcmu_handle_completions(udev))
+        run_qfull_queue(udev, false);
+    mutex_unlock(&udev->cmdr_lock);
+}
+
  static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name)
  {
      struct tcmu_dev *udev;
@@ -1634,6 +1707,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name)
        xa_init(&udev->data_pages);
  +    INIT_WORK(&udev->complete_work, tcmu_complete_work);
      return &udev->se_dev;
  }
  @@ -1725,75 +1799,10 @@ static void tcmu_dev_kref_release(struct kref *kref)
      call_rcu(&dev->rcu_head, tcmu_dev_call_rcu);
  }
  -static void run_qfull_queue(struct tcmu_dev *udev, bool fail)
-{
-    struct tcmu_cmd *tcmu_cmd, *tmp_cmd;
-    LIST_HEAD(cmds);
-    sense_reason_t scsi_ret;
-    int ret;
-
-    if (list_empty(&udev->qfull_queue))
-        return;
-
-    pr_debug("running %s's cmdr queue forcefail %d\n", udev->name, fail);
-
-    list_splice_init(&udev->qfull_queue, &cmds);
-
-    list_for_each_entry_safe(tcmu_cmd, tmp_cmd, &cmds, queue_entry) {
-        list_del_init(&tcmu_cmd->queue_entry);
-
-        pr_debug("removing cmd %p on dev %s from queue\n",
-             tcmu_cmd, udev->name);
-
-        if (fail) {
-            /*
-             * We were not able to even start the command, so
-             * fail with busy to allow a retry in case runner
-             * was only temporarily down. If the device is being
-             * removed then LIO core will do the right thing and
-             * fail the retry.
-             */
-            tcmu_cmd->se_cmd->priv = NULL;
-            target_complete_cmd(tcmu_cmd->se_cmd, SAM_STAT_BUSY);
-            tcmu_free_cmd(tcmu_cmd);
-            continue;
-        }
-
-        ret = queue_cmd_ring(tcmu_cmd, &scsi_ret);
-        if (ret < 0) {
-            pr_debug("cmd %p on dev %s failed with %u\n",
-                 tcmu_cmd, udev->name, scsi_ret);
-            /*
-             * Ignore scsi_ret for now. target_complete_cmd
-             * drops it.
-             */
-            tcmu_cmd->se_cmd->priv = NULL;
-            target_complete_cmd(tcmu_cmd->se_cmd,
-                        SAM_STAT_CHECK_CONDITION);
-            tcmu_free_cmd(tcmu_cmd);
-        } else if (ret > 0) {
-            pr_debug("ran out of space during cmdr queue run\n");
-            /*
-             * cmd was requeued, so just put all cmds back in
-             * the queue
-             */
-            list_splice_tail(&cmds, &udev->qfull_queue);
-            break;
-        }
-    }
-
-    tcmu_set_next_deadline(&udev->qfull_queue, &udev->qfull_timer);
-}
-
  static int tcmu_irqcontrol(struct uio_info *info, s32 irq_on)
  {
      struct tcmu_dev *udev = container_of(info, struct tcmu_dev, uio_info);
-
-    mutex_lock(&udev->cmdr_lock);
-    if (tcmu_handle_completions(udev))
-        run_qfull_queue(udev, false);
-    mutex_unlock(&udev->cmdr_lock);
-
+    queue_work(tcmu_comp_wq, &udev->complete_work);
      return 0;
  }
  @@ -3281,12 +3290,19 @@ static int __init tcmu_module_init(void)
        INIT_DELAYED_WORK(&tcmu_unmap_work, tcmu_unmap_work_fn);
  +    tcmu_comp_wq = alloc_workqueue("tcmu-comp-wq",
+            WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
+    if (!tcmu_comp_wq)
+        return -ENOMEM;
+
      tcmu_cmd_cache = kmem_cache_create("tcmu_cmd_cache",
                  sizeof(struct tcmu_cmd),
                  __alignof__(struct tcmu_cmd),
                  0, NULL);
-    if (!tcmu_cmd_cache)
-        return -ENOMEM;
+    if (!tcmu_cmd_cache) {
+        ret = -ENOMEM;
+        goto out_free_comp_wq;
+    }
        tcmu_root_device = root_device_register("tcm_user");
      if (IS_ERR(tcmu_root_device)) {
@@ -3335,6 +3351,8 @@ static int __init tcmu_module_init(void)
      root_device_unregister(tcmu_root_device);
  out_free_cache:
      kmem_cache_destroy(tcmu_cmd_cache);
+out_free_comp_wq:
+    destroy_workqueue(tcmu_comp_wq);
        return ret;
  }
@@ -3347,6 +3365,7 @@ static void __exit tcmu_module_exit(void)
      genl_unregister_family(&tcmu_genl_family);
      root_device_unregister(tcmu_root_device);
      kmem_cache_destroy(tcmu_cmd_cache);
+    destroy_workqueue(tcmu_comp_wq);
  }
    MODULE_DESCRIPTION("TCM USER subsystem plugin");



[Index of Archives]     [Linux SCSI]     [Kernel Newbies]     [Linux SCSI Target Infrastructure]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Device Mapper]

  Powered by Linux