| Message ID | 20210204113513.93204-10-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: fix cmd plugging and completion | expand |
> * queue_cmd_ring - queue cmd to ring or internally > * @tcmu_cmd: cmd to queue > @@ -1086,8 +1108,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); > Do we need to keep the TODO ? > return 0; > > @@ -2840,6 +2862,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/4/21 5:25 PM, Chaitanya Kulkarni wrote: >> * queue_cmd_ring - queue cmd to ring or internally >> * @tcmu_cmd: cmd to queue >> @@ -1086,8 +1108,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); >> > Do we need to keep the TODO ? I think it's not helpful. The reason for the TODO was to avoid calling uio_event_notify for every command. I think we had thought we could just key of a FLUSH but then later figured out we might not always get one so that wouldn't work. The comment should have been removed or if we like to keep TODOs like that in code it should have been updated to better reflect what the issue was and the idea to fix it.
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index a5991df23581..d67be2f959b9 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,26 @@ 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_cmd *se_cmd) +{ + struct se_device *se_dev = se_cmd->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 +1108,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 +2862,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 and fio jobs this patch can increase IOPs by only around 5% because we hit other issues like the big per tcmu device mutex. Bodo, because the improvement is so small I'm not sure if we want this patch. I was thinking when you fix those other issues you've been working on then it might be more useful. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/target/target_core_user.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)