2021-04-27 13:26:20

by Fabio M. De Francesco

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

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 <[email protected]>
---

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);
}

/*
@@ -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;

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


2021-04-27 13:59:46

by Matthew Wilcox

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

On Tue, Apr 27, 2021 at 03:25:22PM +0200, Fabio M. De Francesco wrote:
> +++ 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

I'm scared of this change. Read the top of the file:

* Everything needed for IOPart-GuestPart communication is define in
* this file. Note: Everything is OS-independent because this file is
* used by Windows, Linux and possible EFI drivers.

I don't know that you can make any changes to this file.

> +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);

You're still not handling the error here.

2021-04-27 14:09:28

by Fabio Aiuto

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

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 <[email protected]>
> ---
>
> 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

2021-04-27 14:28:46

by Fabio M. De Francesco

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

On Tuesday, April 27, 2021 4:07:56 PM CEST Fabio Aiuto wrote:
> 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 <[email protected]>
> > ---
> >
> > 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?
>
No reason at all. I unwittingly removed them while deleting idr_remove()
calls. I'll restore them.
>
> > /*
> >
> > @@ -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?
>
Yes, I forgot to restore the previous format. Julia also pointed it out a few
days ago.

Thanks,

Fabio
>
> > @@ -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);
>
> thank you,
>
> fabio