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
