Re: BUG in shared_policy_replace() ?
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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 --- 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() ?
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: BUG in shared_policy_replace() ?
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
BUG in shared_policy_replace() ?
Hi Andi,
Why free the shared policy created to split up an old
policy that spans the whole new range? Ie, see patch.
Steve
--- 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;
}
