Message-ID: <2833559.jtDpNxbUmt@linux.local>
Date:   Mon, 26 Apr 2021 15:14:42 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     outreachy-kernel@...glegroups.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        David Kershner <david.kershner@...sys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [Outreachy kernel] [RFC PATCH] staging: unisys: visorhba: Convert module from IDR to XArray

On Monday, April 26, 2021 1:49:02 PM CEST Matthew Wilcox wrote:
> On Mon, Apr 26, 2021 at 11:50:15AM +0200, Fabio M. De Francesco wrote:
> >  #define VISORHBA_ERROR_COUNT 30
> > 
> > +static DEFINE_XARRAY_ALLOC(xa_dtstr);
> > +
> > 
> >  static struct dentry *visorhba_debugfs_dir;
> >  
> >  /* GUIDS for HBA channel type supported by this driver */
> > 
> > @@ -78,12 +80,6 @@ struct visorhba_devdata {
> > 
> >  	unsigned int max_buff_len;
> >  	int devnum;
> >  	struct uiscmdrsp *cmdrsp;
> > 
> > -	/*
> > -	 * allows us to pass int handles back-and-forth between us and
> > -	 * iovm, instead of raw pointers
> > -	 */
> > -	struct idr idr;
> > -
> 
> Why did you change the driver from having one namespace per HBA to having
> a global namespace?
> 
I made that change simply because I didn't catch that there could be more than 
just one HBA. 
> >  /*
> > 
> > - * 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
> > + * simple_xa_dtstr_get - Store a pointer to xa_dtstr xarray
> > + * @id: Pointer to ID
> > + * @entry: New entry
> > 
> >   */
> > 
> > -static unsigned int simple_idr_get(struct idr *idrtable, void *p,
> > -				   spinlock_t *lock)
> > +static int simple_xa_dtstr_get(u32 *id, void *entry)
> > 
> >  {
> > 
> > -	int id;
> > -	unsigned long flags;
> > +	int ret = xa_alloc_irq(&xa_dtstr, id, entry, xa_limit_32b, 
GFP_NOWAIT);
> > +	/* TODO: check for and manage errors */
> > 
> > -	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);
> > +	return ret;
> > 
> >  }
> 
> I would think that this wrapper should probably be removed.  It'll almost
> certainly be better to inline the call to xa_alloc_irq() at the call
> sites.
>
I think I got it now.
> 
> You've also changed the behaviour; it used to allocate an id between 1
> and INT_MAX; now it allocates an ID between 0 and UINT_MAX.  Maybe that's
> safe, but you need to argue for it in the changelog.
>
Same as above..
>
> And it shouldn't be using GFP_NOWAIT, but GFP_KERNEL, like the IDR code
> used to do.
>
I'm not sure to understand why idr_preload() uses GFP_KERNEL and instead  
idr_alloc() uses GFP_NOWAIT. I'd better read anew the documentation of the 
above-mentioned functions  
> 
> >  /*
> > 
> > @@ -216,22 +196,25 @@ static unsigned int simple_idr_get(struct idr 
*idrtable, void
> > *p,
> > 
> >   *				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_dtstr: The data object maintaining the pointer<-->int mappings
> 
> You added this in the documentation, but not in the function ...
>
It happened when I wrongly decided to have a global namespace (as I explained 
above).
> 
> >   * @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,
> > -				       wait_queue_head_t *event, 
int *result)
> > +static void setup_scsitaskmgmt_handles(struct uiscmdrsp *cmdrsp,
> > +				       wait_queue_head_t *event, 
u32 *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);
> > +	void *entry;
> > +	int ret;
> > +
> > +	/* specify the event that has to be triggered when this cmd is 
complete */
> > +	entry = &cmdrsp->scsitaskmgmt.notify_handle;
> > +	ret = simple_xa_dtstr_get(result, entry);
> > +	/* TODO: Check for and manage errors */
> 
> The prior code assigned the ID for 'event' to scsitaskmgmt.notify_handle.
> Now, you're allocating an ID for the address of scsitaskmgmt.notify_handle
> to 'result'.  That's clearly not right.
> 
That's due to my fault in understanding the semantics of those functions.
>
> > +	entry = &cmdrsp->scsitaskmgmt.notifyresult_handle;
> > +	ret = simple_xa_dtstr_get(result, entry);
> > +	/* TODO: Check for and manage errors */
> > 
> >  }
> >  
> >  /*
> > 
> > @@ -240,13 +223,17 @@ static void setup_scsitaskmgmt_handles(struct idr 
*idrtable,
> > spinlock_t *lock,> 
> >   * @idrtable: The data object maintaining the pointer<-->int mappings
> >   * @cmdrsp:   Response from the IOVM
> >   */
> > 
> > -static void cleanup_scsitaskmgmt_handles(struct idr *idrtable,
> > -					 struct uiscmdrsp 
*cmdrsp)
> > +static void cleanup_scsitaskmgmt_handles(struct uiscmdrsp_scsitaskmgmt 
*scsitaskmgmt)
> > 
> >  {
> > 
> > -	if (cmdrsp->scsitaskmgmt.notify_handle)
> > -		idr_remove(idrtable, cmdrsp-
>scsitaskmgmt.notify_handle);
> > -	if (cmdrsp->scsitaskmgmt.notifyresult_handle)
> > -		idr_remove(idrtable, cmdrsp-
>scsitaskmgmt.notifyresult_handle);
> > +	struct uiscmdrsp *cmdrsp;
> > +	unsigned long index;
> > +
> > +	xa_for_each(&xa_dtstr, index, cmdrsp) {
> > +		if (&cmdrsp->scsitaskmgmt != scsitaskmgmt)
> > +			continue;
> > +		xa_erase(&xa_dtstr, index);
> > +		kfree(cmdrsp);
> > +	}
> 
> I suspect this is part of the same confusion, but the old code passed in an
> ID that we just looked up & removed.  You've changed that to iterate over
> all the entries and remove the ones that match ...
>
I'm not sure if I understand it, however I suppose that you mean that there is 
no need to iterate over all the entries to find the one that matches for the 
purpose of erasing it. I'll look at this issue anew and find out the right way 
for the removals.
> 
> > @@ -1096,7 +1077,7 @@ static void visorhba_remove(struct visor_device 
*dev)
> > 
> >  	scsi_remove_host(scsihost);
> >  	scsi_host_put(scsihost);
> > 
> > -	idr_destroy(&devdata->idr);
> > +	xa_destroy(&xa_dtstr);
> > 
> >  	dev_set_drvdata(&dev->device, NULL);
> >  	debugfs_remove(devdata->debugfs_info);
> 
> What happens if you have two HBAs in the system, one is active and you
> remove the other one?
>
This will not be anymore a problem when I'll restore the use of one namespace 
per HBA. It's correct?
>
> More generally, the IDR required you call idr_destroy() to avoid leaking
> preallocated memory.  I changed that, but there are still many drivers
> that have unnecessary calls to idr_destroy().  It's good form to just
> delete them and not turn them into calls to xa_destroy().
>
This one is a bit obscure to me. I have to look into it more carefully. Maybe 
I'll ask for some further help.

Thanks for your review,

Fabio 

Powered by blists - more mailing lists