2.4.14 + Bug in swap_out.

2.4.14 + Bug in swap_out.

[Eric W. Biederman]

In swap_out we have the following code:

	spin_lock(&mmlist_lock);
	mm = swap_mm;
	while (mm->swap_address == TASK_SIZE || mm == &init_mm) {
		mm->swap_address = 0;
		mm = list_entry(mm->mmlist.next, struct mm_struct, mmlist);
		if (mm == swap_mm)
			goto empty;
		swap_mm = mm;
	}

	/* Make sure the mm doesn't disappear when we drop the lock.. */
	atomic_inc(&mm->mm_users);
	spin_unlock(&mmlist_lock);

	nr_pages = swap_out_mm(mm, nr_pages, &counter, classzone);

	mmput(mm);


And looking in fork.c mmput under with right circumstances becomes.
kmem_cache_free(mm_cachep, (mm)))

So it appears that there is nothing that keeps the mm_struct that
swap_mm points to as being valid. 

I guess the easy fix would be to increment the count on swap_mm,
and then do an mmput we assign something else to the value of swap_mm.  But
I don't know if that is what we want.

Thoughts?

Eric








--
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/

Re: 2.4.14 + Bug in swap_out.

[David S. Miller]

   And looking in fork.c mmput under with right circumstances becomes.
   kmem_cache_free(mm_cachep, (mm)))
   
   So it appears that there is nothing that keeps the mm_struct that
   swap_mm points to as being valid. 

I do not agree with your analysis.

If we hold the mmlist lock and we find the mm on the swap mm list, by
definition it must have a non-zero user count already.  (put an assert
there if you don't believe me :-)

Only when the user count drops to zero will mmput() free up the mm.
It simultaneously grabs the mmlist lock when it drops the user count
to zero, this is how it synchronizes with the rest of the world.
Perhaps you aren't noticing that it is using "atomic_dec_and_lock()"
or you don't understand how that primitive works?

We increment the mm user count before dropping the mmlist lock in the
swapper, so even if the user does a mmput() we still hold a reference.
ie. mmput won't put the user count to zero.
--
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/

Re: 2.4.14 + Bug in swap_out.

[Eric W. Biederman]

"David S. Miller" <davem@redhat.com> writes:

> I do not agree with your analysis.

Neither do I now but not for your reasons :)

I looked again we are o.k. but just barely.  mmput explicitly checks
to see if it is freeing the swap_mm, and fixes if we are.  It is a
nasty interplay with the swap_mm global, but the code is correct.

My apologies for freaking out I but I couldn't imagine mmput doing
something like that.

Eric
--
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/

Re: 2.4.14 + Bug in swap_out.

[Rik van Riel]

On 20 Nov 2001, Eric W. Biederman wrote:
> "David S. Miller" <davem@redhat.com> writes:
>
> > I do not agree with your analysis.
>
> Neither do I now but not for your reasons :)
>
> I looked again we are o.k. but just barely.  mmput explicitly checks
> to see if it is freeing the swap_mm, and fixes if we are.  It is a
> nasty interplay with the swap_mm global, but the code is correct.

To be honest I don't see the reason for this subtle
playing with swap_mm in mmput(), since the refcounting
should mean we're safe.

Rik
-- 
Shortwave goes a long way:  irc.starchat.net  #swl

http://www.surriel.com/		http://distro.conectiva.com/

--
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/

Re: 2.4.14 + Bug in swap_out.

[Eric W. Biederman]

Rik van Riel <riel@conectiva.com.br> writes:

> On 20 Nov 2001, Eric W. Biederman wrote:
> > "David S. Miller" <davem@redhat.com> writes:
> >
> > > I do not agree with your analysis.
> >
> > Neither do I now but not for your reasons :)
> >
> > I looked again we are o.k. but just barely.  mmput explicitly checks
> > to see if it is freeing the swap_mm, and fixes if we are.  It is a
> > nasty interplay with the swap_mm global, but the code is correct.
> 
> To be honest I don't see the reason for this subtle
> playing with swap_mm in mmput(), since the refcounting
> should mean we're safe.

We only hold a ref count for the duration of swap_out_mm.
Not for the duration of the value in swap_mm.

Eric
--
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/

Re: 2.4.14 + Bug in swap_out.

[Rik van Riel]

On 21 Nov 2001, Eric W. Biederman wrote:

> We only hold a ref count for the duration of swap_out_mm.
> Not for the duration of the value in swap_mm.

In that case, why can't we just take the next mm from
init_mm and just "roll over" our mm to the back of the
list once we're done with it ?

Removing magic is good ;)

regards,

Rik
-- 
DMCA, SSSCA, W3C?  Who cares?  http://thefreeworld.net/

http://www.surriel.com/		http://distro.conectiva.com/

--
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/

Re: 2.4.14 + Bug in swap_out.

[Eric W. Biederman]

Rik van Riel <riel@conectiva.com.br> writes:

> On 21 Nov 2001, Eric W. Biederman wrote:
> 
> > We only hold a ref count for the duration of swap_out_mm.
> > Not for the duration of the value in swap_mm.
> 
> In that case, why can't we just take the next mm from
> init_mm and just "roll over" our mm to the back of the
> list once we're done with it ?

Sounds good to me.  Unless we have another user for that list.
 
> Removing magic is good ;)

Definitely.  Things that are locally correct are much easier
to verify and trust.  I'm satisfied for the moment that it isn't
actually broken.  But more obvious code is definitely a plus
if we can get it.

Eric
--
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/

Re: 2.4.14 + Bug in swap_out.

[Rik van Riel]

On 20 Nov 2001, Eric W. Biederman wrote:

> 	/* Make sure the mm doesn't disappear when we drop the lock.. */
> 	atomic_inc(&mm->mm_users);
> 	spin_unlock(&mmlist_lock);
>
> 	nr_pages = swap_out_mm(mm, nr_pages, &counter, classzone);
>
> 	mmput(mm);
>
>
> And looking in fork.c mmput under with right circumstances becomes.
> kmem_cache_free(mm_cachep, (mm)))
>
> So it appears that there is nothing that keeps the mm_struct that
> swap_mm points to as being valid.

The atomic_inc(&mm->mm_users) above should make sure this
mm_struct stays valid.

regards,

Rik
-- 
Shortwave goes a long way:  irc.starchat.net  #swl

http://www.surriel.com/		http://distro.conectiva.com/

--
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/

Re: 2.4.14 + Bug in swap_out.

[Rik van Riel]

On Wed, 21 Nov 2001, Hugh Dickins wrote:

> > In that case, why can't we just take the next mm from
> > init_mm and just "roll over" our mm to the back of the
> > list once we're done with it ?
>
> No.  That's how it used to be, that's what I changed it from.
>
> fork and exec are well ordered in how they add to the mmlist,
> and that ordering (children after parent) suited swapoff nicely,
> to minimize duplication of a swapent while it's being unused;
> except swap_out randomized the order by cycling init_mm around it.

Urmmm, so the code was obfuscated in order to optimise
swapoff() ?

Exactly how bad was the "mmlist randomising" for swapoff() ?

regards,

Rik
-- 
DMCA, SSSCA, W3C?  Who cares?  http://thefreeworld.net/

http://www.surriel.com/		http://distro.conectiva.com/

--
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/

Re: 2.4.14 + Bug in swap_out.

[Eric W. Biederman]

Hugh Dickins <hugh@veritas.com> writes:

> On Wed, 21 Nov 2001, Rik van Riel wrote:
> > On Wed, 21 Nov 2001, Hugh Dickins wrote:
> > >
> > > fork and exec are well ordered in how they add to the mmlist,
> > > and that ordering (children after parent) suited swapoff nicely,
> > > to minimize duplication of a swapent while it's being unused;
> > > except swap_out randomized the order by cycling init_mm around it.
> > 
> > Urmmm, so the code was obfuscated in order to optimise
> > swapoff() ?
> 
> To speed swapoff, I changed the code back to how fork (see comment
> on "Add it to the mmlist" in fork.c old and new) and exec seemed to
> intend.  I don't see see that I _obfuscated_ the code:
> what's so difficult about swap_mm?

Practical test when I pointed out that something needed to be done
(and I didn't see the code in mmput) both David & Rik didn't even
see the problem much less where it was worked around.  And neither
of the saw the code in mmput.  If people can look at the code
and see what is going on that is hard to follow, by definition.

The primary problem with swap_mm is that swap_mm is used totally
unexpectedly in a different file.  Instead of it's usage being small
local and contained.

> > Exactly how bad was the "mmlist randomising" for swapoff() ?
> 
> It was unnecessary and counter-productive, I changed it.
> Exact number?  No, but small.

There is some sense in allowing swapoff not to check new processes
but...  The only optimization that really makes sense with swapoff is
to turn it inside out and traverse each process only once...  With
possibly a little of the current logic to handle the shared swap case.

Eric
--
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/