[git pull] slub fallback fix

[git pull] slub fallback fix

[Christoph Lameter]

We need to reenable interrupts before calling the page allocator from the
fallback path for kmalloc. Used to be okay with the alternate fastpath 
which shifted interrupt enable/disable to the fastpath. But the slowpath
is always called with interrupts disabled now. So we need this fix.


  git://git.kernel.org/pub/scm/linux/kernel/git/christoph/vm.git slab-linus

Christoph Lameter (1):
      slub page alloc fallback: Enable interrupts for GFP_WAIT.

 mm/slub.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 96d63eb..ca71d5b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1536,9 +1536,15 @@ new_slab:
         * That is only possible if certain conditions are met that are 
being
         * checked when a slab is created.
         */
-       if (!(gfpflags & __GFP_NORETRY) && (s->flags & 
__PAGE_ALLOC_FALLBACK))
-               return kmalloc_large(s->objsize, gfpflags);
-
+       if (!(gfpflags & __GFP_NORETRY) &&
+                               (s->flags & __PAGE_ALLOC_FALLBACK)) {
+               if (gfpflags & __GFP_WAIT)
+                       local_irq_enable();
+               object = kmalloc_large(s->objsize, gfpflags);
+               if (gfpflags & __GFP_WAIT)
+                       local_irq_disable();
+               return object;
+       }
        return NULL;
 debug:
        if (!alloc_debug_processing(s, c->page, object, addr))

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [git pull] slub fallback fix

[Linus Torvalds]

On Mon, 17 Mar 2008, Christoph Lameter wrote:
>
> We need to reenable interrupts before calling the page allocator from the
> fallback path for kmalloc. Used to be okay with the alternate fastpath 
> which shifted interrupt enable/disable to the fastpath. But the slowpath
> is always called with interrupts disabled now. So we need this fix.

I think this fix is bogus and inefficient.

The proper fix would seem to be to just not disable the irq's 
unnecessarily!

We don't care what the return state of the interrupts are, since the 
caller will restore the _true_ interrupt state (which we don't even know 
about, so we can't do it). 

So why isn't the patch just doing something like the appended instead of 
disabling and enabling interrupts unnecessarily?

		Linus
---
 mm/slub.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 96d63eb..a082390 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1511,10 +1511,10 @@ new_slab:
 
 	new = new_slab(s, gfpflags, node);
 
-	if (gfpflags & __GFP_WAIT)
-		local_irq_disable();
-
 	if (new) {
+		if (gfpflags & __GFP_WAIT)
+			local_irq_disable();
+
 		c = get_cpu_slab(s, smp_processor_id());
 		stat(c, ALLOC_SLAB);
 		if (c->page)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [git pull] slub fallback fix

[Christoph Lameter]

On Tue, 18 Mar 2008, Linus Torvalds wrote:

> > We need to reenable interrupts before calling the page allocator from the
> > fallback path for kmalloc. Used to be okay with the alternate fastpath 
> > which shifted interrupt enable/disable to the fastpath. But the slowpath
> > is always called with interrupts disabled now. So we need this fix.
> 
> I think this fix is bogus and inefficient.
> 
> The proper fix would seem to be to just not disable the irq's 
> unnecessarily!
> 
> We don't care what the return state of the interrupts are, since the 
> caller will restore the _true_ interrupt state (which we don't even know 
> about, so we can't do it). 
> 
> So why isn't the patch just doing something like the appended instead of 
> disabling and enabling interrupts unnecessarily?

Fallback is rare and I'd like to have the fallback logic in one place. It 
would also mean that the interrupt state on return to slab_alloc() would 
be indeterminate. Currently that works but it may be surprising in the 
future when changes are made there.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [git pull] slub fallback fix

[Linus Torvalds]

On Tue, 18 Mar 2008, Christoph Lameter wrote:
> 
> Fallback is rare and I'd like to have the fallback logic in one place. It 
> would also mean that the interrupt state on return to slab_alloc() would 
> be indeterminate. Currently that works but it may be surprising in the 
> future when changes are made there.

But your version is not just larger and slower, I think it's less obvious 
too exactly because it has a lot *more* of those special cases.

That irq handling in the allocator doesn't "nest" correctly, and it has 
never ever been "good practice" to re-enable interrupts when they've been 
disabled by the caller anyway, so all this code already violates the 
standard rules.

For good reasons, don't get me wrong. But this code is not normal, and 
it's already violating all rules. Wouldn't it be better to at least try to 
keep it small and simple rather than try to have some made-up rules that 
still violate all the common rules?

Just look at the patch:

	-       if (!(gfpflags & __GFP_NORETRY) && (s->flags & __PAGE_ALLOC_FALLBACK))
	-               return kmalloc_large(s->objsize, gfpflags);
	-
	+       if (!(gfpflags & __GFP_NORETRY) &&
	+                               (s->flags & __PAGE_ALLOC_FALLBACK)) {
	+               if (gfpflags & __GFP_WAIT)
	+                       local_irq_enable();
	+               object = kmalloc_large(s->objsize, gfpflags);
	+               if (gfpflags & __GFP_WAIT)
	+                       local_irq_disable();
	+               return object;
	+       }

and try to tell me that the new code is more readable? I *really* don't 
agree.

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [git pull] slub fallback fix

[Christoph Lameter]

On Tue, 18 Mar 2008, Linus Torvalds wrote:

> That irq handling in the allocator doesn't "nest" correctly, and it has 
> never ever been "good practice" to re-enable interrupts when they've been 
> disabled by the caller anyway, so all this code already violates the 
> standard rules.

Yes this interupt stuff in the slab allocators is not the nicest thing. 
Surely wish we could get rid of it. The realtime folks may be able to get 
there by simply not using the slab allocator from interrupt contexts.
 
> Just look at the patch:
> 
> 	-       if (!(gfpflags & __GFP_NORETRY) && (s->flags & __PAGE_ALLOC_FALLBACK))
> 	-               return kmalloc_large(s->objsize, gfpflags);
> 	-
> 	+       if (!(gfpflags & __GFP_NORETRY) &&
> 	+                               (s->flags & __PAGE_ALLOC_FALLBACK)) {
> 	+               if (gfpflags & __GFP_WAIT)
> 	+                       local_irq_enable();
> 	+               object = kmalloc_large(s->objsize, gfpflags);
> 	+               if (gfpflags & __GFP_WAIT)
> 	+                       local_irq_disable();
> 	+               return object;
> 	+       }
> 
> and try to tell me that the new code is more readable? I *really* don't 
> agree.

Well it may now have become not so readable anymore. However, this 
contains the kmalloc fallback logic in one spot. And that logic is likely 
going to be generalized for 2.6.26 removing __PAGE_ALLOC_FALLBACK etc. The 
chunk is going away. Either solution is fine with me. Just get it fixed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [git pull] slub fallback fix

[Linus Torvalds]

On Tue, 18 Mar 2008, Christoph Lameter wrote:
> 
> Well it may now have become not so readable anymore. However, this 
> contains the kmalloc fallback logic in one spot. And that logic is likely 
> going to be generalized for 2.6.26 removing __PAGE_ALLOC_FALLBACK etc. The 
> chunk is going away. Either solution is fine with me. Just get it fixed.

Ok, if the code is going away, I don't care enough. I pulled your tree.

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>