From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx154.postini.com [74.125.245.154]) by kanga.kvack.org (Postfix) with SMTP id D1E426B00AA for ; Wed, 6 Jun 2012 07:30:07 -0400 (EDT) Received: by wefh52 with SMTP id h52so5737422wef.14 for ; Wed, 06 Jun 2012 04:30:06 -0700 (PDT) MIME-Version: 1.0 Reply-To: konrad@darnok.org In-Reply-To: <1338980115-2394-5-git-send-email-levinsasha928@gmail.com> References: <1338980115-2394-1-git-send-email-levinsasha928@gmail.com> <1338980115-2394-5-git-send-email-levinsasha928@gmail.com> Date: Wed, 6 Jun 2012 07:30:05 -0400 Message-ID: Subject: Re: [PATCH 05/11] mm: frontswap: split frontswap_shrink further to eliminate locking games From: Konrad Rzeszutek Wilk Content-Type: multipart/alternative; boundary=0016e6d7e943bdc7d604c1cc1674 Sender: owner-linux-mm@kvack.org List-ID: To: Sasha Levin Cc: linux-mm@kvack.org, Konrad Rzeszutek Wilk , linux-kernel@vger.kernel.org, dan.magenheimer@oracle.com --0016e6d7e943bdc7d604c1cc1674 Content-Type: text/plain; charset=ISO-8859-1 On Jun 6, 2012 6:55 AM, "Sasha Levin" wrote: > > Split frontswap_shrink to eliminate the locking issues in the original code. Can you describe the locking issue please? > > Signed-off-by: Sasha Levin > --- > mm/frontswap.c | 36 +++++++++++++++++++++--------------- > 1 files changed, 21 insertions(+), 15 deletions(-) > > diff --git a/mm/frontswap.c b/mm/frontswap.c > index a9b76cb..618ef91 100644 > --- a/mm/frontswap.c > +++ b/mm/frontswap.c > @@ -244,6 +244,24 @@ static int __frontswap_unuse_pages(unsigned long total, unsigned long *unused, > return ret; > } > > +static int __frontswap_shrink(unsigned long target_pages, > + unsigned long *pages_to_unuse, > + int *type) > +{ > + unsigned long total_pages = 0, total_pages_to_unuse; > + > + lockdep_assert_held(&swap_lock); > + > + total_pages = __frontswap_curr_pages(); > + if (total_pages <= target_pages) { > + /* Nothing to do */ > + *pages_to_unuse = 0; > + return 0; > + } > + total_pages_to_unuse = total_pages - target_pages; > + return __frontswap_unuse_pages(total_pages_to_unuse, pages_to_unuse, type); > +} > + > /* > * Frontswap, like a true swap device, may unnecessarily retain pages > * under certain circumstances; "shrink" frontswap is essentially a > @@ -254,10 +272,8 @@ static int __frontswap_unuse_pages(unsigned long total, unsigned long *unused, > */ > void frontswap_shrink(unsigned long target_pages) > { > - unsigned long total_pages = 0, total_pages_to_unuse; > unsigned long pages_to_unuse = 0; > int type, ret; > - bool locked = false; > > /* > * we don't want to hold swap_lock while doing a very > @@ -265,20 +281,10 @@ void frontswap_shrink(unsigned long target_pages) > * so restart scan from swap_list.head each time > */ > spin_lock(&swap_lock); > - locked = true; > - total_pages = __frontswap_curr_pages(); > - if (total_pages <= target_pages) > - goto out; > - total_pages_to_unuse = total_pages - target_pages; > - ret = __frontswap_unuse_pages(total_pages_to_unuse, &pages_to_unuse, &type); > - if (ret < 0) > - goto out; > - locked = false; > + ret = __frontswap_shrink(target_pages, &pages_to_unuse, &type); > spin_unlock(&swap_lock); > - try_to_unuse(type, true, pages_to_unuse); > -out: > - if (locked) > - spin_unlock(&swap_lock); > + if (ret == 0 && pages_to_unuse) > + try_to_unuse(type, true, pages_to_unuse); > return; > } > EXPORT_SYMBOL(frontswap_shrink); > -- > 1.7.8.6 > > -- > 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/ . > Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ > Don't email: email@kvack.org > --0016e6d7e943bdc7d604c1cc1674 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Jun 6, 2012 6:55 AM, "Sasha Levin" <levinsasha928@gmail.com> wrote:
>
> Split frontswap_shrink to eliminate the locking issues in the original= code.

Can you describe the locking issue please?
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> =A0mm/frontswap.c | =A0 36 +++++++++++++++++++++---------------
> =A01 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/mm/frontswap.c b/mm/frontswap.c
> index a9b76cb..618ef91 100644
> --- a/mm/frontswap.c
> +++ b/mm/frontswap.c
> @@ -244,6 +244,24 @@ static int __frontswap_unuse_pages(unsigned long = total, unsigned long *unused,
> =A0 =A0 =A0 =A0return ret;
> =A0}
>
> +static int __frontswap_shrink(unsigned long target_pages,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned= long *pages_to_unuse,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int *typ= e)
> +{
> + =A0 =A0 =A0 unsigned long total_pages =3D 0, total_pages_to_unuse; > +
> + =A0 =A0 =A0 lockdep_assert_held(&swap_lock);
> +
> + =A0 =A0 =A0 total_pages =3D __frontswap_curr_pages();
> + =A0 =A0 =A0 if (total_pages <=3D target_pages) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Nothing to do */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *pages_to_unuse =3D 0;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 total_pages_to_unuse =3D total_pages - target_pages;
> + =A0 =A0 =A0 return __frontswap_unuse_pages(total_pages_to_unuse, pag= es_to_unuse, type);
> +}
> +
> =A0/*
> =A0* Frontswap, like a true swap device, may unnecessarily retain page= s
> =A0* under certain circumstances; "shrink" frontswap is esse= ntially a
> @@ -254,10 +272,8 @@ static int __frontswap_unuse_pages(unsigned long = total, unsigned long *unused,
> =A0*/
> =A0void frontswap_shrink(unsigned long target_pages)
> =A0{
> - =A0 =A0 =A0 unsigned long total_pages =3D 0, total_pages_to_unuse; > =A0 =A0 =A0 =A0unsigned long pages_to_unuse =3D 0;
> =A0 =A0 =A0 =A0int type, ret;
> - =A0 =A0 =A0 bool locked =3D false;
>
> =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 * we don't want to hold swap_lock while doing a ve= ry
> @@ -265,20 +281,10 @@ void frontswap_shrink(unsigned long target_pages= )
> =A0 =A0 =A0 =A0 * so restart scan from swap_list.head each time
> =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0spin_lock(&swap_lock);
> - =A0 =A0 =A0 locked =3D true;
> - =A0 =A0 =A0 total_pages =3D __frontswap_curr_pages();
> - =A0 =A0 =A0 if (total_pages <=3D target_pages)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
> - =A0 =A0 =A0 total_pages_to_unuse =3D total_pages - target_pages;
> - =A0 =A0 =A0 ret =3D __frontswap_unuse_pages(total_pages_to_unuse, &a= mp;pages_to_unuse, &type);
> - =A0 =A0 =A0 if (ret < 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
> - =A0 =A0 =A0 locked =3D false;
> + =A0 =A0 =A0 ret =3D __frontswap_shrink(target_pages, &pages_to_u= nuse, &type);
> =A0 =A0 =A0 =A0spin_unlock(&swap_lock);
> - =A0 =A0 =A0 try_to_unuse(type, true, pages_to_unuse);
> -out:
> - =A0 =A0 =A0 if (locked)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock(&swap_lock);
> + =A0 =A0 =A0 if (ret =3D=3D 0 && pages_to_unuse)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 try_to_unuse(type, true, pages_to_unuse)= ;
> =A0 =A0 =A0 =A0return;
> =A0}
> =A0EXPORT_SYMBOL(frontswap_shrink);
> --
> 1.7.8.6
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in<= br> > the body to majordomo@kvack.org= . =A0For more info on Linux MM,
> see: http://www.linux-mm.org/= .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=3Dmailto:"dont@kvack.org"> emai= l@kvack.org </a>
>

--0016e6d7e943bdc7d604c1cc1674-- -- 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/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org