Re: PATCH - raise max_anon limit

[George Anzinger <george@mvista.com>]

Andrew Morton wrote:
> Jim Houston <jim.houston@ccur.com> wrote:
> 
>>On Wed, 2004-02-11 at 20:20, Andrew Morton wrote:
>>
>>>Tim Hockin <thockin@sun.com> 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@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml