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
mempolicy.c renamed from mempolicy.c.orig +0 -4
@@ -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 ; }
Quoted 3 lines · Steve Longerbeam
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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 8 lines · Hugh Dickins
Quoted 4 lines · Steve Longerbeam
Why free the shared policy created to split up an old policy that spans the whole new range? Ie, see patch.
Quoted 11 lines
I think you're misreading it. That code comes from when I changed it over from
sp->semtosp->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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 3 lines
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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 12 lines · Andi Kleen
Quoted 5 lines
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
mempolicy.c renamed from mempolicy.c.orig +1 -0
@@ -1041,6 +1041,7 @@ } n->end = start;sp_insert (sp, new2); + new2 =NULL ;break ; }else n->end = start;
Quoted 4 lines
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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 10 lines · Andi Kleen
Quoted 6 lines
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:
Steve
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 10 lines · Steve Longerbeam
Quoted 7 lines · Andi Kleen
Quoted 6 lines
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.
Quoted 7 lines
Quoted 3 lines
That sounds weird. Can you figure out which patch in mm removes it?
found it:
Are you sure? I don't see it touching the new2 free at the end of the function.
-Andi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 20 lines · Andi Kleen
Quoted 17 lines · Steve Longerbeam
Quoted 13 lines · Andi Kleen
Quoted 8 lines
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?
Quoted 9 lines
Quoted 5 lines
found it:
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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 38 lines · Steve Longerbeam
Quoted 20 lines · Andi Kleen
Quoted 17 lines · Steve Longerbeam
Quoted 13 lines · Andi Kleen
Quoted 8 lines
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?
Quoted 10 lines
Quoted 5 lines
found it:
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.
Review
Quoted 1 line
(quoted diff) +0
-1- 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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 8 lines · Andi Kleen
Review
Quoted 2 lines
(quoted diff) +0
-1- 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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>