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@xxxxxxxxxx>
> ---
> 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,
>
