Re: [PATCH] Convert idr's internal locking to _irqsave variant

[Andrew Morton]

On Thu, 13 Jul 2006 08:42:47 -0700
Roland Dreier <rdreier@xxxxxxxxx> wrote:

>  > Sigh.  It was always a mistake (of the kernel programming 101 type) to put
>  > any locking at all in the idr code.  At some stage we need to weed it all
>  > out and move it to callers.
>  > 
>  > Your fix is yet more fallout from that mistake.
> 
> Agreed.  Consider me on the hook to fix this up in a better way once
> my life is a little saner.  Maybe I'll try to cook something up on the
> plane ride to Ottawa.
> 

I suspect it'll get really ugly.  It's a container library which needs to
allocate memory when items are added, like the radix-tree.  Either it needs
to assume GFP_ATOMIC, which is bad and can easily fail or it does weird
things like radix_tree_preload().

The basic problem is:

	idr_pre_get(GFP_KERNEL);
	spin_lock(my_lock);
	idr_get_new(..);

which is racy, because some other CPU could have got in there and consumed
some of the pool which was precharged by idr_pre_get().

It's wildly improbable that it'll actually fail.  It requires all of:

a) that the race occur

b) that the racing thread consume an appreciable amount of the pool

c) that this thread also consume an appreciable amount (such that the
   total of both exceeds the pool size).

d) that a (needs to be added) GFP_ATOMIC attempt to replenish the pool
   inside idr_get_new() fails.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/