* [PATCH v8 RESEND] staging: unisys: visorhba: Convert module from IDR to XArray
@ 2021-05-14 8:11 Fabio M. De Francesco
2021-06-15 11:50 ` Greg Kroah-Hartman
0 siblings, 1 reply; 3+ messages in thread
From: Fabio M. De Francesco @ 2021-05-14 8:11 UTC (permalink / raw
To: David Kershner, Greg Kroah-Hartman, sparmaintainer, linux-staging,
linux-kernel, Matthew Wilcox, Daniel Vetter, Dan Carpenter
Cc: Fabio M. De Francesco
Converted visorhba from IDR to XArray. The abstract data type XArray is
more memory-efficient, parallelizable, and cache friendly. It takes
advantage of RCU to perform lookups without locking. Furthermore, IDR is
deprecated because XArray has a better (cleaner and more consistent)
API.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Resubmitting the patch with the addition of "Reviewed-by:
Dan Carpenter" (thanks, Dan) and because I suspect the S-PAR
maintainers missed this in the noise.
I'd appreciate a review by Unisys engineers on this patch.
Also, I'd appreciate a Matthew Wilcox review as the XArray
maintainer.
Changes from v7: Removed xa_destroy, since its insertion was due to the
misunderstanding of the reviews on v5. Removed unnecessary 'else'
clauses.
Changes from v6: Added a call to xa_destroy() that I had forgotten.
Fixed taking locks with interrupts disabled (use xa_erase_irq()).
Replaced checking for success with checking for failure. Issues detected
by Dan Carpenter and Matthew Wilcox.
Changes from v5: As suggested by Matthew Wilcox, reworded the commit
message, modified setup_scsitaskmgmt_handles() to manage and return errors,
used 'xa_limit_32b' as the range of indexes (because there is no need to
use 0 as a marker of no allocation).
Changes from v4: Fixed some issues detected by Matthew Wilcox and Fabio
Aiuto.
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.
.../staging/unisys/visorhba/visorhba_main.c | 101 +++++++-----------
1 file changed, 41 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 4455d26f7c96..41f8a72a2a95 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,8 +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 +181,48 @@ 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 int 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 */
+ ret = xa_alloc_irq(xa, &id, event, xa_limit_32b, GFP_KERNEL);
+ if (ret)
+ return ret;
+ cmdrsp->scsitaskmgmt.notify_handle = id;
+ ret = xa_alloc_irq(xa, &id, result, xa_limit_32b, GFP_KERNEL);
+ if (ret) {
+ xa_erase_irq(xa, cmdrsp->scsitaskmgmt.notify_handle);
+ return ret;
+ }
+ cmdrsp->scsitaskmgmt.notifyresult_handle = id;
+
+ return 0;
}
/*
* 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_irq(xa, cmdrsp->scsitaskmgmt.notify_handle);
+ xa_erase_irq(xa, cmdrsp->scsitaskmgmt.notifyresult_handle);
}
/*
@@ -269,6 +245,7 @@ static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
int notifyresult = 0xffff;
wait_queue_head_t notifyevent;
int scsicmd_id;
+ int ret;
if (devdata->serverdown || devdata->serverchangingstate)
return FAILED;
@@ -284,8 +261,14 @@ 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,
- ¬ifyevent, ¬ifyresult);
+
+ ret = setup_scsitaskmgmt_handles(&devdata->xa, cmdrsp,
+ ¬ifyevent, ¬ifyresult);
+ if (ret) {
+ dev_dbg(&scsidev->sdev_gendev,
+ "visorhba: setup_scsitaskmgmt_handles returned %d\n", ret);
+ return FAILED;
+ }
/* save destination */
cmdrsp->scsitaskmgmt.tasktype = tasktype;
@@ -311,14 +294,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 +637,13 @@ 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,
+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,7 +691,7 @@ static void visorhba_serverdown_complete(struct visorhba_devdata *devdata)
break;
case CMD_SCSITASKMGMT_TYPE:
cmdrsp = pendingdel->sent;
- complete_taskmgmt_command(&devdata->idr, cmdrsp,
+ complete_taskmgmt_command(&devdata->xa, cmdrsp,
TASK_MGMT_FAILED);
break;
default:
@@ -905,7 +888,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 +1036,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 +1079,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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v8 RESEND] staging: unisys: visorhba: Convert module from IDR to XArray
2021-05-14 8:11 [PATCH v8 RESEND] staging: unisys: visorhba: Convert module from IDR to XArray Fabio M. De Francesco
@ 2021-06-15 11:50 ` Greg Kroah-Hartman
2021-06-16 16:56 ` Fabio M. De Francesco
0 siblings, 1 reply; 3+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-15 11:50 UTC (permalink / raw
To: Fabio M. De Francesco
Cc: David Kershner, sparmaintainer, linux-staging, linux-kernel,
Matthew Wilcox, Daniel Vetter, Dan Carpenter
On Fri, May 14, 2021 at 10:11:11AM +0200, Fabio M. De Francesco wrote:
> Converted visorhba from IDR to XArray. The abstract data type XArray is
> more memory-efficient, parallelizable, and cache friendly. It takes
> advantage of RCU to perform lookups without locking. Furthermore, IDR is
> deprecated because XArray has a better (cleaner and more consistent)
> API.
>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
Given a lack of response from the unisys maintainer, I'll go apply this
now and see what breaks :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v8 RESEND] staging: unisys: visorhba: Convert module from IDR to XArray
2021-06-15 11:50 ` Greg Kroah-Hartman
@ 2021-06-16 16:56 ` Fabio M. De Francesco
0 siblings, 0 replies; 3+ messages in thread
From: Fabio M. De Francesco @ 2021-06-16 16:56 UTC (permalink / raw
To: Greg Kroah-Hartman
Cc: David Kershner, sparmaintainer, linux-staging, linux-kernel,
Matthew Wilcox, Daniel Vetter, Dan Carpenter
On Tuesday, June 15, 2021 1:50:15 PM CEST Greg Kroah-Hartman wrote:
> On Fri, May 14, 2021 at 10:11:11AM +0200, Fabio M. De Francesco wrote:
> > Converted visorhba from IDR to XArray. The abstract data type XArray is
> > more memory-efficient, parallelizable, and cache friendly. It takes
> > advantage of RCU to perform lookups without locking. Furthermore, IDR is
> > deprecated because XArray has a better (cleaner and more consistent)
> > API.
> >
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
>
> Given a lack of response from the unisys maintainer, I'll go apply this
> now and see what breaks :)
>
Hi Greg,
I'm really grateful to you for having applied that patch notwithstanding the
lack of response from Unisys. Understanding IDR, XArray, and the flow of the
code, has been a hard task for a newcomer having only a month of experience to
kernel development.
Thanks very much,
Fabio
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-16 16:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-14 8:11 [PATCH v8 RESEND] staging: unisys: visorhba: Convert module from IDR to XArray Fabio M. De Francesco
2021-06-15 11:50 ` Greg Kroah-Hartman
2021-06-16 16:56 ` Fabio M. De Francesco
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.