Re: [PATCH] change gen_pool allocator to not touch managed memory

[Jes Sorensen <jes@sgi.com>]

Dean Nelson wrote:
> On Wed, Apr 26, 2006 at 11:12:48AM +0200, Jes Sorensen wrote:
>> Dean Nelson wrote:
>>> +	if (nbytes > PAGE_SIZE) {
>>> +		chunk = vmalloc_node(nbytes, nid);
>>> +	} else {
>>> +		chunk = kmalloc_node(nbytes, GFP_KERNEL, nid);
>>> +	}
>> Any patch that adds vmalloc() calls to code always makes the little
>> hairs on the back of my neck stand up. Any chance we could get away with
>> alloc_pages_node() for this?
> 
> Is it the mapping of the pages that bothers you? If using alloc_pages_node()
> is the preferred way, I certainly can make the change. But if I do there is
> a greater potential that we may have to return failure to the caller of
> gen_pool_add(), that is if we can't get the necessary number of contiguous
> pages. Now granted the likelyhood that anyone would require more than a
> page for a bitmap is very very small. I'd say the vast majority of callers
> will end up using kmalloc_node(). I can go either way, just let me know
> whether I should make the change or not.

vmalloc mappings are $$$ on many archs so they should be avoided if in
any way possible. Also, kmalloc can handle more than just a page, and it
might be better to just use that here rather than alloc_pages actually
since I presume there is nothing preventing the bitmap sharing pages
with other data.

In this case I think adding the vmalloc call is overkill, I would simply
make it call kmalloc_node() unconditionally for all sizes and let it
fail if that situation occurs, given how unlikely it is.

>>> Index: linux-2.6/arch/ia64/sn/kernel/sn2/cache.c
>>> =================================>>> --- linux-2.6.orig/arch/ia64/sn/kernel/sn2/cache.c	2006-04-24 12:25:36.234717101 -0500
>>> +++ linux-2.6/arch/ia64/sn/kernel/sn2/cache.c	2006-04-24 12:27:56.012899026 -0500
>> This part we should maybe do in a seperate patch? It seems valid on it's
>> own?
> 
> I thought of this, but if this patch were separated out then the remaining
> patch would be dependent on it since the uncached allocator is being
> changed to call sn_flush_all_caches() with an uncached address.
> It certainly could be done, but is it worth the effort? Let me know
> how I should proceed with this.

I would expect this part of the patch to be able to go in as is,
straight away so I don't think it should be a problem. It's not a big
deal whether we do it one way or another to me.

Cheers,
Jes