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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On Monday, April 26, 2021 3:29:28 PM CEST Matthew Wilcox wrote:
> On Mon, Apr 26, 2021 at 03:14:42PM +0200, Fabio M. De Francesco wrote:
> > > > -	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);
> > > 
> > > 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
> 
> If you're holding a spinlock, you can't do a GFP_KERNEL allocation,
> because it can sleep, and sleeping while holding a spinlock isn't allowed.
>
I know that since a long time... that is last week, day more day less  :) 

I've just started to read R.Love's LKD 3rd ed. I don't have enough time to 
read it at the moment, however I skipped a few chapters and read "Kernel 
synchronization methods" and "Memory management".
> 
> The IDR and radix tree have an approach where you first preallocate
> memory using GFP_KERNEL and then use GFP_NOWAIT or GFP_ATOMIC after
> you've taken the spinlock.  XArray doesn't do that; it takes the spinlock
> and does a GFP_NOWAIT allocation.  If it fails, it drops the spinlock,
> allocates the memory using GFP_KERNEL, and retries.
>
This is something one cannot find in Love's book, unless I overlooked that.
> 
> > This will not be anymore a problem when I'll restore the use of one 
namespace
> > per HBA. It's correct?
> 
> true ...
> 
> > > 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.
> 
> The IDR used to have a per-idr preallocation, so you had to destroy it
> in order to make sure they were freed.  I got rid of that about five
> years ago because most IDR users weren't calling idr_destroy().
>
OK, I think I got it.
>
Thanks again,

Fabio