Re: [Outreachy kernel] [PATCH v4] staging: unisys: visorhba: Convert module from IDR to XArray

From: Fabio Aiuto
Date: Tue Apr 27 2021 - 10:10:01 EST

Hi Fabio,

On Tue, Apr 27, 2021 at 03:25:22PM +0200, Fabio M. De Francesco wrote:
> Converted visorhba from IDR to XArray. The abstract data type XArray is
> more memory-efficient, parallelisable and cache friendly. It takes
> advantage of RCU to perform lookups without locking.
>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> ---
>
> Changes from v3: Matthew Wilcox found that the XArray was not
> initialized: now it is. Changed types handles from u64 to u32 because
> they can't work as arguments of xa_alloc_irq() and u32 is enough large
> for storing XArray indexes.
> Changes from v2: Some residual errors from v1 were not fixed in v2. Now
> they have been removed.
> Changes from v1: After a first review by Matthew Wilcox, who found a
> series of errors and gave suggestions on how to fix them, I rewrote a
> larger part of the patch.
>
> drivers/staging/unisys/include/iochannel.h | 4 +-
> .../staging/unisys/visorhba/visorhba_main.c | 89 ++++++-------------
> 2 files changed, 28 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/staging/unisys/include/iochannel.h b/drivers/staging/unisys/include/iochannel.h
> index 9ef812c0bc42..fac89eac148b 100644
> --- a/drivers/staging/unisys/include/iochannel.h
> +++ b/drivers/staging/unisys/include/iochannel.h
> @@ -474,8 +474,8 @@ struct uiscmdrsp_scsitaskmgmt {
> enum task_mgmt_types tasktype;
> struct uisscsi_dest vdest;
> u64 handle;
> - u64 notify_handle;
> - u64 notifyresult_handle;
> + u32 notify_handle;
> + u32 notifyresult_handle;
> char result;
>
> #define TASK_MGMT_FAILED 0
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 4455d26f7c96..2b6cde254f17 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -6,10 +6,10 @@
>
> #include <linux/debugfs.h>
> #include <linux/kthread.h>
> -#include <linux/idr.h>
> #include <linux/module.h>
> #include <linux/seq_file.h>
> #include <linux/visorbus.h>
> +#include <linux/xarray.h>
> #include <scsi/scsi.h>
> #include <scsi/scsi_host.h>
> #include <scsi/scsi_cmnd.h>
> @@ -82,7 +82,7 @@ struct visorhba_devdata {
> * allows us to pass int handles back-and-forth between us and
> * iovm, instead of raw pointers
> */
> - struct idr idr;
> + struct xarray xa;
>
> struct dentry *debugfs_dir;
> struct dentry *debugfs_info;
> @@ -182,71 +182,40 @@ static struct uiscmdrsp *get_scsipending_cmdrsp(struct visorhba_devdata *ddata,
> return NULL;
> }
>
> -/*
> - * simple_idr_get - Associate a provided pointer with an int value
> - * 1 <= value <= INT_MAX, and return this int value;
> - * the pointer value can be obtained later by passing
> - * this int value to idr_find()
> - * @idrtable: The data object maintaining the pointer<-->int mappings
> - * @p: The pointer value to be remembered
> - * @lock: A spinlock used when exclusive access to idrtable is needed
> - *
> - * Return: The id number mapped to pointer 'p', 0 on failure
> - */
> -static unsigned int simple_idr_get(struct idr *idrtable, void *p,
> - spinlock_t *lock)
> -{
> - int id;
> - unsigned long flags;
> -
> - idr_preload(GFP_KERNEL);
> - spin_lock_irqsave(lock, flags);
> - id = idr_alloc(idrtable, p, 1, INT_MAX, GFP_NOWAIT);
> - spin_unlock_irqrestore(lock, flags);
> - idr_preload_end();
> - /* failure */
> - if (id < 0)
> - return 0;
> - /* idr_alloc() guarantees > 0 */
> - return (unsigned int)(id);
> -}
> -
> /*
> * setup_scsitaskmgmt_handles - Stash the necessary handles so that the
> * completion processing logic for a taskmgmt
> * cmd will be able to find who to wake up
> * and where to stash the result
> - * @idrtable: The data object maintaining the pointer<-->int mappings
> - * @lock: A spinlock used when exclusive access to idrtable is needed
> + * @xa: The data object maintaining the pointer<-->int mappings
> * @cmdrsp: Response from the IOVM
> * @event: The event handle to associate with an id
> * @result: The location to place the result of the event handle into
> */
> -static void setup_scsitaskmgmt_handles(struct idr *idrtable, spinlock_t *lock,
> - struct uiscmdrsp *cmdrsp,
> +static void setup_scsitaskmgmt_handles(struct xarray *xa, struct uiscmdrsp *cmdrsp,
> wait_queue_head_t *event, int *result)
> {
> - /* specify the event that has to be triggered when this */
> - /* cmd is complete */
> - cmdrsp->scsitaskmgmt.notify_handle =
> - simple_idr_get(idrtable, event, lock);
> - cmdrsp->scsitaskmgmt.notifyresult_handle =
> - simple_idr_get(idrtable, result, lock);
> + int ret;
> + u32 *id;
> +
> + /* specify the event that has to be triggered when this cmd is complete */
> + id = &cmdrsp->scsitaskmgmt.notify_handle;
> + ret = xa_alloc_irq(xa, id, event, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
> + id = &cmdrsp->scsitaskmgmt.notifyresult_handle;
> + ret = xa_alloc_irq(xa, id, result, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
> }
>
> /*
> * cleanup_scsitaskmgmt_handles - Forget handles created by
> * setup_scsitaskmgmt_handles()
> - * @idrtable: The data object maintaining the pointer<-->int mappings
> + * @xa: The data object maintaining the pointer<-->int mappings
> * @cmdrsp: Response from the IOVM
> */
> -static void cleanup_scsitaskmgmt_handles(struct idr *idrtable,
> +static void cleanup_scsitaskmgmt_handles(struct xarray *xa,
> struct uiscmdrsp *cmdrsp)
> {
> - if (cmdrsp->scsitaskmgmt.notify_handle)
> - idr_remove(idrtable, cmdrsp->scsitaskmgmt.notify_handle);
> - if (cmdrsp->scsitaskmgmt.notifyresult_handle)
> - idr_remove(idrtable, cmdrsp->scsitaskmgmt.notifyresult_handle);
> + xa_erase(xa, cmdrsp->scsitaskmgmt.notify_handle);
> + xa_erase(xa, cmdrsp->scsitaskmgmt.notifyresult_handle);
> }

why were the conditions removed before each entry deletion?

>
> /*
> @@ -273,8 +242,7 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
> if (devdata->serverdown || devdata->serverchangingstate)
> return FAILED;
>
> - scsicmd_id = add_scsipending_entry(devdata, CMD_SCSITASKMGMT_TYPE,
> - NULL);
> + scsicmd_id = add_scsipending_entry(devdata, CMD_SCSITASKMGMT_TYPE, NULL);
> if (scsicmd_id < 0)
> return FAILED;
>

this is a code format change, maybe this go in a separate patch?

> @@ -284,8 +252,7 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
>
> /* issue TASK_MGMT_ABORT_TASK */
> cmdrsp->cmdtype = CMD_SCSITASKMGMT_TYPE;
> - setup_scsitaskmgmt_handles(&devdata->idr, &devdata->privlock, cmdrsp,
> - &notifyevent, &notifyresult);
> + setup_scsitaskmgmt_handles(&devdata->xa, cmdrsp, &notifyevent, &notifyresult);
>
> /* save destination */
> cmdrsp->scsitaskmgmt.tasktype = tasktype;
> @@ -311,14 +278,14 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
> dev_dbg(&scsidev->sdev_gendev,
> "visorhba: taskmgmt type=%d success; result=0x%x\n",
> tasktype, notifyresult);
> - cleanup_scsitaskmgmt_handles(&devdata->idr, cmdrsp);
> + cleanup_scsitaskmgmt_handles(&devdata->xa, cmdrsp);
> return SUCCESS;
>
> err_del_scsipending_ent:
> dev_dbg(&scsidev->sdev_gendev,
> "visorhba: taskmgmt type=%d not executed\n", tasktype);
> del_scsipending_ent(devdata, scsicmd_id);
> - cleanup_scsitaskmgmt_handles(&devdata->idr, cmdrsp);
> + cleanup_scsitaskmgmt_handles(&devdata->xa, cmdrsp);
> return FAILED;
> }
>
> @@ -654,13 +621,12 @@ DEFINE_SHOW_ATTRIBUTE(info_debugfs);
> * Service Partition returned the result of the task management
> * command. Wake up anyone waiting for it.
> */
> -static void complete_taskmgmt_command(struct idr *idrtable,
> - struct uiscmdrsp *cmdrsp, int result)
> +static void complete_taskmgmt_command(struct xarray *xa, struct uiscmdrsp *cmdrsp, int result)
> {
> wait_queue_head_t *wq =
> - idr_find(idrtable, cmdrsp->scsitaskmgmt.notify_handle);
> + xa_load(xa, cmdrsp->scsitaskmgmt.notify_handle);
> int *scsi_result_ptr =
> - idr_find(idrtable, cmdrsp->scsitaskmgmt.notifyresult_handle);
> + xa_load(xa, cmdrsp->scsitaskmgmt.notifyresult_handle);
> if (unlikely(!(wq && scsi_result_ptr))) {
> pr_err("visorhba: no completion context; cmd will time out\n");
> return;
> @@ -708,8 +674,7 @@ static void visorhba_serverdown_complete(struct visorhba_devdata *devdata)
> break;
> case CMD_SCSITASKMGMT_TYPE:
> cmdrsp = pendingdel->sent;
> - complete_taskmgmt_command(&devdata->idr, cmdrsp,
> - TASK_MGMT_FAILED);
> + complete_taskmgmt_command(&devdata->xa, cmdrsp, TASK_MGMT_FAILED);
> break;
> default:
> break;
> @@ -905,7 +870,7 @@ static void drain_queue(struct uiscmdrsp *cmdrsp,
> if (!del_scsipending_ent(devdata,
> cmdrsp->scsitaskmgmt.handle))
> break;
> - complete_taskmgmt_command(&devdata->idr, cmdrsp,
> + complete_taskmgmt_command(&devdata->xa, cmdrsp,
> cmdrsp->scsitaskmgmt.result);
> } else if (cmdrsp->cmdtype == CMD_NOTIFYGUEST_TYPE)
> dev_err_once(&devdata->dev->device,
> @@ -1053,7 +1018,7 @@ static int visorhba_probe(struct visor_device *dev)
> if (err)
> goto err_debugfs_info;
>
> - idr_init(&devdata->idr);
> + xa_init(&devdata->xa);
>
> devdata->cmdrsp = kmalloc(sizeof(*devdata->cmdrsp), GFP_ATOMIC);
> visorbus_enable_channel_interrupts(dev);
> @@ -1096,8 +1061,6 @@ static void visorhba_remove(struct visor_device *dev)
> scsi_remove_host(scsihost);
> scsi_host_put(scsihost);
>
> - idr_destroy(&devdata->idr);
> -
> dev_set_drvdata(&dev->device, NULL);
> debugfs_remove(devdata->debugfs_info);
> debugfs_remove_recursive(devdata->debugfs_dir);
> --
> 2.31.1
>
>

thank you,

fabio