* [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray
@ 2021-04-26 18:42 Fabio M. De Francesco
2021-04-26 19:21 ` Matthew Wilcox
0 siblings, 1 reply; 5+ messages in thread
From: Fabio M. De Francesco @ 2021-04-26 18:42 UTC (permalink / raw)
To: outreachy-kernel, linux-staging, linux-kernel, David Kershner,
Greg Kroah-Hartman, Daniel Vetter, Matthew Wilcox
Cc: Fabio M. De Francesco
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@gmail.com>
---
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 | 89 ++++++-------------
1 file changed, 25 insertions(+), 64 deletions(-)
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 4455d26f7c96..938a5081b835 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);
+ u32 id;
+ int ret;
+
+ /* specify the event that has to be triggered when this cmd is complete */
+ id = (u32)cmdrsp->scsitaskmgmt.notify_handle;
+ ret = xa_alloc_irq(xa, &id, event, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
+ id = (u32)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,
- ¬ifyevent, ¬ifyresult);
+ setup_scsitaskmgmt_handles(&devdata->xa, cmdrsp, ¬ifyevent, ¬ifyresult);
/* 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,8 +1018,6 @@ static int visorhba_probe(struct visor_device *dev)
if (err)
goto err_debugfs_info;
- idr_init(&devdata->idr);
-
devdata->cmdrsp = kmalloc(sizeof(*devdata->cmdrsp), GFP_ATOMIC);
visorbus_enable_channel_interrupts(dev);
@@ -1096,8 +1059,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] 5+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray
2021-04-26 18:42 [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray Fabio M. De Francesco
@ 2021-04-26 19:21 ` Matthew Wilcox
2021-04-26 21:15 ` Fabio M. De Francesco
0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2021-04-26 19:21 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: outreachy-kernel, linux-staging, linux-kernel, David Kershner,
Greg Kroah-Hartman, Daniel Vetter
On Mon, Apr 26, 2021 at 08:42:45PM +0200, Fabio M. De Francesco wrote:
> +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);
> + u32 id;
> + int ret;
> +
> + /* specify the event that has to be triggered when this cmd is complete */
> + id = (u32)cmdrsp->scsitaskmgmt.notify_handle;
> + ret = xa_alloc_irq(xa, &id, event, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
OK, think this one through a bit. When xa_alloc_irq() stores the ID that
it assigned into 'id', what happens to it next?
> + id = (u32)cmdrsp->scsitaskmgmt.notifyresult_handle;
> + ret = xa_alloc_irq(xa, &id, result, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
> }
> @@ -1053,8 +1018,6 @@ static int visorhba_probe(struct visor_device *dev)
> if (err)
> goto err_debugfs_info;
>
> - idr_init(&devdata->idr);
You still need to initialise the XArray, either with xa_init() or by
using DEFINE_XARRAY. Since it's dynamically allocated, the former is
correct.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray
2021-04-26 19:21 ` Matthew Wilcox
@ 2021-04-26 21:15 ` Fabio M. De Francesco
2021-04-26 22:03 ` Matthew Wilcox
0 siblings, 1 reply; 5+ messages in thread
From: Fabio M. De Francesco @ 2021-04-26 21:15 UTC (permalink / raw)
To: Matthew Wilcox
Cc: outreachy-kernel, linux-staging, linux-kernel, David Kershner,
Greg Kroah-Hartman, Daniel Vetter
On Monday, April 26, 2021 9:21:01 PM CEST Matthew Wilcox wrote:
> On Mon, Apr 26, 2021 at 08:42:45PM +0200, Fabio M. De Francesco wrote:
> > +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);
> > + u32 id;
> > + int ret;
> > +
> > + /* specify the event that has to be triggered when this cmd is
complete */
> > + id = (u32)cmdrsp->scsitaskmgmt.notify_handle;
> > + ret = xa_alloc_irq(xa, &id, event, XA_LIMIT(1, INT_MAX),
GFP_KERNEL);
>
> OK, think this one through a bit. When xa_alloc_irq() stores the ID that
> it assigned into 'id', what happens to it next?
>
Oh, I overlooked that... The ID in 'id' is lost when the function exits and
the stack frame is unwound.
Now I have another problem: xa_alloc_irq() writes id u32* but I have an u64*
in 'id'. What happens if I cast 'id' to an u32* when passing it to
xa_alloc_irq()?
u64 *id;
int ret;
id = &cmdrsp->scsitaskmgmt.notify_handle;
ret = xa_alloc_irq(xa, (u32 *)id, event, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
Do I destroy the information stored in 'id' with that cast?
>
> > + id = (u32)cmdrsp->scsitaskmgmt.notifyresult_handle;
> > + ret = xa_alloc_irq(xa, &id, result, XA_LIMIT(1, INT_MAX),
GFP_KERNEL);
> >
> > }
> >
> > @@ -1053,8 +1018,6 @@ static int visorhba_probe(struct visor_device *dev)
> >
> > if (err)
> >
> > goto err_debugfs_info;
> >
> > - idr_init(&devdata->idr);
>
> You still need to initialise the XArray, either with xa_init() or by
> using DEFINE_XARRAY. Since it's dynamically allocated, the former is
> correct.
>
I had read that XArray must be initialized with xa_init(). However, when I
deleted the line with the initialization of the IDR, I forgot to replace it
with a statement that uses xa_init().
Thanks,
Fabio
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray
2021-04-26 21:15 ` Fabio M. De Francesco
@ 2021-04-26 22:03 ` Matthew Wilcox
2021-04-27 11:59 ` Fabio M. De Francesco
0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2021-04-26 22:03 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: outreachy-kernel, linux-staging, linux-kernel, David Kershner,
Greg Kroah-Hartman, Daniel Vetter
On Mon, Apr 26, 2021 at 11:15:40PM +0200, Fabio M. De Francesco wrote:
> On Monday, April 26, 2021 9:21:01 PM CEST Matthew Wilcox wrote:
> > On Mon, Apr 26, 2021 at 08:42:45PM +0200, Fabio M. De Francesco wrote:
> > > +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);
> > > + u32 id;
> > > + int ret;
> > > +
> > > + /* specify the event that has to be triggered when this cmd is
> complete */
> > > + id = (u32)cmdrsp->scsitaskmgmt.notify_handle;
> > > + ret = xa_alloc_irq(xa, &id, event, XA_LIMIT(1, INT_MAX),
> GFP_KERNEL);
> >
> > OK, think this one through a bit. When xa_alloc_irq() stores the ID that
> > it assigned into 'id', what happens to it next?
> >
> Oh, I overlooked that... The ID in 'id' is lost when the function exits and
> the stack frame is unwound.
>
> Now I have another problem: xa_alloc_irq() writes id u32* but I have an u64*
> in 'id'. What happens if I cast 'id' to an u32* when passing it to
> xa_alloc_irq()?
>
> u64 *id;
> int ret;
> id = &cmdrsp->scsitaskmgmt.notify_handle;
> ret = xa_alloc_irq(xa, (u32 *)id, event, XA_LIMIT(1, INT_MAX), GFP_KERNEL);
>
> Do I destroy the information stored in 'id' with that cast?
That is a great question! That would be a really serious bug because
it behaves differently on big and little endian systems. That is, on a
little endian system, a pointer to a u64 can be treated as a pointer to a
u32 and it will write to the bottom 32 bits of the u64. On a big endian
system, treating a pointer to a u64 as if it's a pointer to a u32 means
you write to the _top_ 32 bits of the u64, and things go wrong from there!
Similarly, if you have a u16, you can't pass a pointer to it, because
the called function has no idea that it's only 16 bits, and will do a
32-bit store to it, overwriting the 16 bits after it.
So you need to pass a pointer to a u32 on the stack, and then copy the
id out of it afterwards.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray
2021-04-26 22:03 ` Matthew Wilcox
@ 2021-04-27 11:59 ` Fabio M. De Francesco
0 siblings, 0 replies; 5+ messages in thread
From: Fabio M. De Francesco @ 2021-04-27 11:59 UTC (permalink / raw)
To: Matthew Wilcox
Cc: outreachy-kernel, linux-staging, linux-kernel, David Kershner,
Greg Kroah-Hartman, Daniel Vetter
On Tuesday, April 27, 2021 12:03:27 AM CEST Matthew Wilcox wrote:
> On Mon, Apr 26, 2021 at 11:15:40PM +0200, Fabio M. De Francesco wrote:
> > On Monday, April 26, 2021 9:21:01 PM CEST Matthew Wilcox wrote:
> > > On Mon, Apr 26, 2021 at 08:42:45PM +0200, Fabio M. De Francesco wrote:
> > > > +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);
> > > > + u32 id;
> > > > + int ret;
> > > > +
> > > > + /* specify the event that has to be triggered when this cmd is
> >
> > complete */
> >
> > > > + id = (u32)cmdrsp->scsitaskmgmt.notify_handle;
> > > > + ret = xa_alloc_irq(xa, &id, event, XA_LIMIT(1, INT_MAX),
> >
> > GFP_KERNEL);
> >
> > > OK, think this one through a bit. When xa_alloc_irq() stores the ID
that
> > > it assigned into 'id', what happens to it next?
> >
> > Oh, I overlooked that... The ID in 'id' is lost when the function exits
and
> > the stack frame is unwound.
> >
> > Now I have another problem: xa_alloc_irq() writes id u32* but I have an
u64*
> > in 'id'. What happens if I cast 'id' to an u32* when passing it to
> > xa_alloc_irq()?
> >
> > u64 *id;
> > int ret;
> > id = &cmdrsp->scsitaskmgmt.notify_handle;
> > ret = xa_alloc_irq(xa, (u32 *)id, event, XA_LIMIT(1, INT_MAX),
GFP_KERNEL);
> >
> > Do I destroy the information stored in 'id' with that cast?
>
> That is a great question! That would be a really serious bug because
> it behaves differently on big and little endian systems. That is, on a
> little endian system, a pointer to a u64 can be treated as a pointer to a
> u32 and it will write to the bottom 32 bits of the u64. On a big endian
> system, treating a pointer to a u64 as if it's a pointer to a u32 means
> you write to the _top_ 32 bits of the u64, and things go wrong from there!
>
> Similarly, if you have a u16, you can't pass a pointer to it, because
> the called function has no idea that it's only 16 bits, and will do a
> 32-bit store to it, overwriting the 16 bits after it.
>
> So you need to pass a pointer to a u32 on the stack, and then copy the
> id out of it afterwards.
>
As far as I understand, in setup_scsitaskmgmt_handles(), the task of
xa_alloc_irq() is: find two empty entries in 'xa', store the indexes into the
'id' pointer (before the first call this is set to &cmdrsp-
>scsitaskmgmt.notify_handle, and before the second call it is changed to
&cmdrsp->scsitaskmgmt.notifyresult_handle), then store the entries at their
respective indexes.
Indexes that xa_alloc_* set are of type u32*, so why not just change the type
of notify_handle and notifyresult_handle from u64 to u32?
Furthermore, I cannot understand why those indexes should be passed in and out
as arguments of the function . It seems that they are not needed anywhere else
in that file. Are they?
Maybe that I'm still missing something...
Thanks,
Fabio
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-27 11:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-26 18:42 [Outreachy kernel] [PATCH v3] staging: unisys: visorhba: Convert module from IDR to XArray Fabio M. De Francesco
2021-04-26 19:21 ` Matthew Wilcox
2021-04-26 21:15 ` Fabio M. De Francesco
2021-04-26 22:03 ` Matthew Wilcox
2021-04-27 11:59 ` Fabio M. De Francesco
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).