Re: PATCH - raise max_anon limit

[Andrew Morton]

Jim Houston <jim.houston@xxxxxxxx> wrote:
>
> On Wed, 2004-02-11 at 20:20, Andrew Morton wrote:
> > Tim Hockin <thockin@xxxxxxx> wrote:
> > > No, it doesn't store the counter with the id.  They expect you to do that.
> > > My best understanding is that thi sis to prevent re-use of the same key.
> > > I'm not sure I grok why it is useful.  If you release a key, it should be
> > > safe to reuse.  Period.  I assume there was some use case that brought about
> > > this "feature" but if so, I don't know what it is.  The big comment about it
> > > is just confusing me.
> > 
> > Maybe Jim can tell us why it's there.  Certainly, the idr interface would
> > be more useful if it just returned id's which start from zero.
> 
> Hi Andrew, Everyone,
> 
> If this new use of idr.c as a sparse bitmap catches on,

I think it should catch on - it is a fairly common kernel requirement.  The
max_anon thing requires it, and I am also pressing it upon the scsi guys to
handle enormous numbers of disks (depends on how they end up doing that). 
In neither case is the associated pointer needed.

> When I wrote the original code, I was thinking of allocating process
> id values where there is a tradition of allocating sequential values.

File descriptors are like that too.

> George Anzinger rewrote most of my code.  The r in idr.c is for
> immediate reuse.  His version picks the lowest available bit in the
> sparse bitmap.  The RESERVED_BITS comments seem to be stale.
> 
> The rational for avoiding immediate reuse of id values is to catch
> application errors.   Consider:
> 
> 	fd1 = open_like_call(...);
> 	read(fd1,...);
> 	close(fd1);
> 	fd2 = open_like_call(...);
> 	write(fd1...);
> 
> If fd2 has a different value than the recently closed fd1, the
> error is detected immediately.
> 

In this case the debug capability is getting in the way of real-world
requirements, which is not good.

idr_pre_get() is not very good IMO.  For a start, it's racy:

	idr_pre_get();
	lock();
	idr_get_new();
	unlock();

how do we know that some other CPU didn't come in and steal our
preallocation?  That's why I (buggily) converted unnamed_dev_lock from a
spinlock to a semaphore, so we could perform the preallocation under the
same locking.

It would be better, and more idiomatic if idr_get_new() were to take a gfp
mask and to perform its own allocation.  That has its own problems and if
the code is under really heavy stress one might need to emulate
radix_tree_preload()/radix_tree_preload_end(), but for most things that's a
bit over the top.


-
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/