--- 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;
}
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
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
> 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
--- 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;
> 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
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
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
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;
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
Andi Kleen <[email protected]> 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.