Re: PATCH - raise max_anon limit

[George Anzinger]

Andrew Morton wrote:
> 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.


Another option might be to use a pre allocate pool size based on the number of 
CPUs.  This does not require CPU*n elements as it is is a tree and "n" here is 
worst case so something like "n+#CPUs" elements would be enough.  (n, by the 
way, is the maximum number of levels in the tree).  Its a bit sloppy but, IMHO, 
would survive almost all storms.
>

--
George Anzinger   george@xxxxxxxxxx
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

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