BUG in shared_policy_replace() ?

BUG in shared_policy_replace() ?

[Steve Longerbeam]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

Hi Andi,

Why free the shared policy created to split up an old
policy that spans the whole new range? Ie, see patch.

Steve

[-- Attachment #2: mempolicy.diff --]
[-- Type: text/plain, Size: 297 bytes --]

--- mm/mempolicy.c.orig	2005-01-18 16:13:35.573273351 -0800
+++ mm/mempolicy.c	2005-01-18 16:24:23.940608135 -0800
@@ -1052,10 +1052,6 @@
 	if (new)
 		sp_insert(sp, new);
 	spin_unlock(&sp->lock);
-	if (new2) {
-		mpol_free(new2->policy);
-		kmem_cache_free(sn_cache, new2);
-	}
 	return 0;
 }
 

Re: BUG in shared_policy_replace() ?

[Hugh Dickins]

On Tue, 18 Jan 2005, Steve Longerbeam wrote:
> 
> Why free the shared policy created to split up an old
> policy that spans the whole new range? Ie, see patch.

I think you're misreading it.  That code comes from when I changed it
over from sp->sem to sp->lock.  If it finds that it needs to split an
existing range, so needs to allocate a new2, then it has to drop and
reacquire the spinlock around that.  It's conceivable that a racing
task could change the tree while the spinlock is dropped, in such a
way that this split is no longer necessary once we reacquire the
spinlock.  The code you're looking at frees up new2 in that case;
whereas in the normal case, where it is still needed, there's a
new2 = NULL after inserting it, so that it won't be freed below.

Hugh

Re: BUG in shared_policy_replace() ?

[Steve Longerbeam]

Hugh Dickins wrote:

>On Tue, 18 Jan 2005, Steve Longerbeam wrote:
>  
>
>>Why free the shared policy created to split up an old
>>policy that spans the whole new range? Ie, see patch.
>>    
>>
>
>I think you're misreading it.  That code comes from when I changed it
>over from sp->sem to sp->lock.  If it finds that it needs to split an
>existing range, so needs to allocate a new2, then it has to drop and
>reacquire the spinlock around that.  It's conceivable that a racing
>task could change the tree while the spinlock is dropped, in such a
>way that this split is no longer necessary once we reacquire the
>spinlock.  The code you're looking at frees up new2 in that case;
>whereas in the normal case, where it is still needed, there's a
>new2 = NULL after inserting it, so that it won't be freed below.
>  
>

got it, except that there is no "new2 = NULL;" in 2.6.10-mm2!

Looks like it was misplaced, because I do see it now in 2.6.10.

Steve

Re: BUG in shared_policy_replace() ?

[Andi Kleen]

> got it, except that there is no "new2 = NULL;" in 2.6.10-mm2!
> 
> Looks like it was misplaced, because I do see it now in 2.6.10.

I double checked 2.6.10 and the code also looks correct me,
working as described by Hugh.

Optimistic locking can be ugly :)

-Andi

Re: BUG in shared_policy_replace() ?

[Steve Longerbeam]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]



Andi Kleen wrote:

>>got it, except that there is no "new2 = NULL;" in 2.6.10-mm2!
>>
>>Looks like it was misplaced, because I do see it now in 2.6.10.
>>    
>>
>
>I double checked 2.6.10 and the code also looks correct me,
>working as described by Hugh.
>
>Optimistic locking can be ugly :)
>  
>

yeah, 2.6.10 makes sense to me too. But I'm working in -mm2, and
the new2 = NULL line is missing, hence my initial confusion. Trivial
patch to -mm2 attached. Just want to make sure it has been, or will be,
put back in.

Steve

[-- Attachment #2: mempolicy-mm2.diff --]
[-- Type: text/plain, Size: 253 bytes --]

--- mm/mempolicy.c.orig	2005-01-19 09:52:47.153910873 -0800
+++ mm/mempolicy.c	2005-01-19 09:53:21.548999628 -0800
@@ -1041,6 +1041,7 @@
 				}
 				n->end = start;
 				sp_insert(sp, new2);
+				new2 = NULL;
 				break;
 			} else
 				n->end = start;

Re: BUG in shared_policy_replace() ?

[Andi Kleen]

> yeah, 2.6.10 makes sense to me too. But I'm working in -mm2, and
> the new2 = NULL line is missing, hence my initial confusion. Trivial
> patch to -mm2 attached. Just want to make sure it has been, or will be,
> put back in.

That sounds weird. Can you figure out which patch in mm removes it?

-Andi

Re: BUG in shared_policy_replace() ?

[Steve Longerbeam]

Andi Kleen wrote:

>>yeah, 2.6.10 makes sense to me too. But I'm working in -mm2, and
>>the new2 = NULL line is missing, hence my initial confusion. Trivial
>>patch to -mm2 attached. Just want to make sure it has been, or will be,
>>put back in.
>>    
>>
>
>That sounds weird. Can you figure out which patch in mm removes it?
>  
>

found it:

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10/2.6.10-mm1/broken-out/mempolicy-optimization.patch

Steve

Re: BUG in shared_policy_replace() ?

[Andi Kleen]

On Wed, Jan 19, 2005 at 10:59:16AM -0800, Steve Longerbeam wrote:
> 
> Andi Kleen wrote:
> 
> >>yeah, 2.6.10 makes sense to me too. But I'm working in -mm2, and
> >>the new2 = NULL line is missing, hence my initial confusion. Trivial
> >>patch to -mm2 attached. Just want to make sure it has been, or will be,
> >>put back in.
> >>   
> >>
> >
> >That sounds weird. Can you figure out which patch in mm removes it?
> > 
> >
> 
> found it:
> 
> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10/2.6.10-mm1/broken-out/mempolicy-optimization.patch

Are you sure? I don't see it touching the new2 free at the end of the function.

-Andi

Re: BUG in shared_policy_replace() ?

[Steve Longerbeam]

Andi Kleen wrote:

>On Wed, Jan 19, 2005 at 10:59:16AM -0800, Steve Longerbeam wrote:
>  
>
>>Andi Kleen wrote:
>>
>>    
>>
>>>>yeah, 2.6.10 makes sense to me too. But I'm working in -mm2, and
>>>>the new2 = NULL line is missing, hence my initial confusion. Trivial
>>>>patch to -mm2 attached. Just want to make sure it has been, or will be,
>>>>put back in.
>>>>  
>>>>
>>>>        
>>>>
>>>That sounds weird. Can you figure out which patch in mm removes it?
>>>
>>>
>>>      
>>>
>>found it:
>>
>>http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10/2.6.10-mm1/broken-out/mempolicy-optimization.patch
>>    
>>
>
>Are you sure? I don't see it touching the new2 free at the end of the function.
>  
>

it's not touching the new2 free, it's removing the new2 = NULL which is 
the problem.

-				new2 = NULL;

Re: BUG in shared_policy_replace() ?

[Andi Kleen]

On Wed, Jan 19, 2005 at 11:25:52AM -0800, Steve Longerbeam wrote:
> 
> 
> Andi Kleen wrote:
> 
> >On Wed, Jan 19, 2005 at 10:59:16AM -0800, Steve Longerbeam wrote:
> > 
> >
> >>Andi Kleen wrote:
> >>
> >>   
> >>
> >>>>yeah, 2.6.10 makes sense to me too. But I'm working in -mm2, and
> >>>>the new2 = NULL line is missing, hence my initial confusion. Trivial
> >>>>patch to -mm2 attached. Just want to make sure it has been, or will be,
> >>>>put back in.
> >>>> 
> >>>>
> >>>>       
> >>>>
> >>>That sounds weird. Can you figure out which patch in mm removes it?
> >>>
> >>>
> >>>     
> >>>
> >>found it:
> >>
> >>http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10/2.6.10-mm1/broken-out/mempolicy-optimization.patch
> >>   
> >>
> >
> >Are you sure? I don't see it touching the new2 free at the end of the 
> >function.
> > 
> >
> 
> it's not touching the new2 free, it's removing the new2 = NULL which is 
> the problem.
> 
> -				new2 = NULL;

Ah, I agree. Yes, it looks like a merging error when merging
with Hugh's changes. Thanks for catching this.

The line should not be removed. Andrew should I submit a new patch or can 
you just fix it up?

-Andi

Re: BUG in shared_policy_replace() ?

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
>  > -				new2 = NULL;
> 
>  Ah, I agree. Yes, it looks like a merging error when merging
>  with Hugh's changes. Thanks for catching this.
> 
>  The line should not be removed. Andrew should I submit a new patch or can 
>  you just fix it up?

I'll fix it up, thanks.