| Message ID | 20210209123845.4856-11-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: fix cmd plugging and completion | expand |
On 09.02.21 13:38, Mike Christie wrote: > This patch adds plug/unplug callouts for tcmu, so we can avoid the > number of times we switch to userspace. Using this driver with tcm > loop is a common config, and dependng on the nr_hw_queues > (nr_hw_queues=1 performs much better) and fio jobs (lower num jobs > around 4) this patch can increase IOPs by only around 5-10% because > we hit other issues like the big per tcmu device mutex.> > Signed-off-by: Mike Christie <michael.christie@oracle.com> > --- > drivers/target/target_core_user.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c > index a5991df23581..a030ca6f0f4c 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -111,6 +111,7 @@ struct tcmu_dev { > struct kref kref; > > struct se_device se_dev; > + struct se_dev_plug se_plug; > > char *name; > struct se_hba *hba; > @@ -119,6 +120,7 @@ struct tcmu_dev { > #define TCMU_DEV_BIT_BROKEN 1 > #define TCMU_DEV_BIT_BLOCKED 2 > #define TCMU_DEV_BIT_TMR_NOTIFY 3 > +#define TCM_DEV_BIT_PLUGGED 4 > unsigned long flags; > > struct uio_info uio_info; > @@ -959,6 +961,25 @@ static uint32_t ring_insert_padding(struct tcmu_dev *udev, size_t cmd_size) > return cmd_head; > } > > +static void tcmu_unplug_device(struct se_dev_plug *se_plug) > +{ > + struct se_device *se_dev = se_plug->se_dev; > + struct tcmu_dev *udev = TCMU_DEV(se_dev); > + > + uio_event_notify(&udev->uio_info); Don't we have a race here? Let's assume that - just here the thread is interrupted - userspace starts,empties the ring and sleeps again - another cpu queues a new CDB in the ring In that - of course very rare condition - userspace will not wake up for the freshly queued CDB. I think, first clearing the bit, then doing the uio_event_notify would work (without need to take the big tcmu mutex). > + clear_bit(TCM_DEV_BIT_PLUGGED, &udev->flags); > +} > + > +static struct se_dev_plug *tcmu_plug_device(struct se_device *se_dev) > +{ > + struct tcmu_dev *udev = TCMU_DEV(se_dev); > + > + if (!test_and_set_bit(TCM_DEV_BIT_PLUGGED, &udev->flags)) > + return &udev->se_plug; > + > + return NULL; > +} > + > /** > * queue_cmd_ring - queue cmd to ring or internally > * @tcmu_cmd: cmd to queue > @@ -1086,8 +1107,8 @@ static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err) > > list_add_tail(&tcmu_cmd->queue_entry, &udev->inflight_queue); > > - /* TODO: only if FLUSH and FUA? */ > - uio_event_notify(&udev->uio_info); > + if (!test_bit(TCM_DEV_BIT_PLUGGED, &udev->flags)) > + uio_event_notify(&udev->uio_info); > > return 0; > > @@ -2840,6 +2861,8 @@ static struct target_backend_ops tcmu_ops = { > .configure_device = tcmu_configure_device, > .destroy_device = tcmu_destroy_device, > .free_device = tcmu_free_device, > + .unplug_device = tcmu_unplug_device, > + .plug_device = tcmu_plug_device, > .parse_cdb = tcmu_parse_cdb, > .tmr_notify = tcmu_tmr_notify, > .set_configfs_dev_params = tcmu_set_configfs_dev_params, >
On 2/9/21 10:32 AM, Bodo Stroesser wrote: > On 09.02.21 13:38, Mike Christie wrote: >> This patch adds plug/unplug callouts for tcmu, so we can avoid the >> number of times we switch to userspace. Using this driver with tcm >> loop is a common config, and dependng on the nr_hw_queues >> (nr_hw_queues=1 performs much better) and fio jobs (lower num jobs >> around 4) this patch can increase IOPs by only around 5-10% because >> we hit other issues like the big per tcmu device mutex.> >> Signed-off-by: Mike Christie <michael.christie@oracle.com> >> --- >> drivers/target/target_core_user.c | 27 +++++++++++++++++++++++++-- >> 1 file changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c >> index a5991df23581..a030ca6f0f4c 100644 >> --- a/drivers/target/target_core_user.c >> +++ b/drivers/target/target_core_user.c >> @@ -111,6 +111,7 @@ struct tcmu_dev { >> struct kref kref; >> >> struct se_device se_dev; >> + struct se_dev_plug se_plug; >> >> char *name; >> struct se_hba *hba; >> @@ -119,6 +120,7 @@ struct tcmu_dev { >> #define TCMU_DEV_BIT_BROKEN 1 >> #define TCMU_DEV_BIT_BLOCKED 2 >> #define TCMU_DEV_BIT_TMR_NOTIFY 3 >> +#define TCM_DEV_BIT_PLUGGED 4 >> unsigned long flags; >> >> struct uio_info uio_info; >> @@ -959,6 +961,25 @@ static uint32_t ring_insert_padding(struct tcmu_dev *udev, size_t cmd_size) >> return cmd_head; >> } >> >> +static void tcmu_unplug_device(struct se_dev_plug *se_plug) >> +{ >> + struct se_device *se_dev = se_plug->se_dev; >> + struct tcmu_dev *udev = TCMU_DEV(se_dev); >> + >> + uio_event_notify(&udev->uio_info); > > Don't we have a race here? > > Let's assume that > - just here the thread is interrupted > - userspace starts,empties the ring and sleeps again > - another cpu queues a new CDB in the ring > In that - of course very rare condition - userspace will not wake up for the freshly queued CDB. > > I think, first clearing the bit, then doing the uio_event_notify would work (without need to take the big tcmu mutex). You,re right. Will fix. I have the same issue in iblock and there I made a mistake where it per cpu when it should be per task.
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index a5991df23581..a030ca6f0f4c 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -111,6 +111,7 @@ struct tcmu_dev { struct kref kref; struct se_device se_dev; + struct se_dev_plug se_plug; char *name; struct se_hba *hba; @@ -119,6 +120,7 @@ struct tcmu_dev { #define TCMU_DEV_BIT_BROKEN 1 #define TCMU_DEV_BIT_BLOCKED 2 #define TCMU_DEV_BIT_TMR_NOTIFY 3 +#define TCM_DEV_BIT_PLUGGED 4 unsigned long flags; struct uio_info uio_info; @@ -959,6 +961,25 @@ static uint32_t ring_insert_padding(struct tcmu_dev *udev, size_t cmd_size) return cmd_head; } +static void tcmu_unplug_device(struct se_dev_plug *se_plug) +{ + struct se_device *se_dev = se_plug->se_dev; + struct tcmu_dev *udev = TCMU_DEV(se_dev); + + uio_event_notify(&udev->uio_info); + clear_bit(TCM_DEV_BIT_PLUGGED, &udev->flags); +} + +static struct se_dev_plug *tcmu_plug_device(struct se_device *se_dev) +{ + struct tcmu_dev *udev = TCMU_DEV(se_dev); + + if (!test_and_set_bit(TCM_DEV_BIT_PLUGGED, &udev->flags)) + return &udev->se_plug; + + return NULL; +} + /** * queue_cmd_ring - queue cmd to ring or internally * @tcmu_cmd: cmd to queue @@ -1086,8 +1107,8 @@ static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err) list_add_tail(&tcmu_cmd->queue_entry, &udev->inflight_queue); - /* TODO: only if FLUSH and FUA? */ - uio_event_notify(&udev->uio_info); + if (!test_bit(TCM_DEV_BIT_PLUGGED, &udev->flags)) + uio_event_notify(&udev->uio_info); return 0; @@ -2840,6 +2861,8 @@ static struct target_backend_ops tcmu_ops = { .configure_device = tcmu_configure_device, .destroy_device = tcmu_destroy_device, .free_device = tcmu_free_device, + .unplug_device = tcmu_unplug_device, + .plug_device = tcmu_plug_device, .parse_cdb = tcmu_parse_cdb, .tmr_notify = tcmu_tmr_notify, .set_configfs_dev_params = tcmu_set_configfs_dev_params,
This patch adds plug/unplug callouts for tcmu, so we can avoid the number of times we switch to userspace. Using this driver with tcm loop is a common config, and dependng on the nr_hw_queues (nr_hw_queues=1 performs much better) and fio jobs (lower num jobs around 4) this patch can increase IOPs by only around 5-10% because we hit other issues like the big per tcmu device mutex. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/target/target_core_user.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)