linux-kernel - Re: [PATCH v2 03/10] mm/page_alloc: insert memory barriers to allow async update of pcp batch and high

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOtvUMdXJSzV5V3WQpDrU1DqzFk4G4RtBLdrgJyGR-AZhY6RNw@mail.gmail.com>
Date:	Wed, 10 Apr 2013 09:19:40 +0300
From:	Gilad Ben-Yossef <gilad@...yossef.com>
To:	Cody P Schafer <cody@...ux.vnet.ibm.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Simon Jeons <simon.jeons@...il.com>,
	KOSAKI Motohiro <kosaki.motohiro@...il.com>,
	Mel Gorman <mgorman@...e.de>, Linux MM <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 03/10] mm/page_alloc: insert memory barriers to allow
 async update of pcp batch and high

On Wed, Apr 10, 2013 at 2:28 AM, Cody P Schafer <cody@...ux.vnet.ibm.com> wrote:
> In pageset_set_batch() and setup_pagelist_highmark(), ensure that batch
> is always set to a safe value (1) prior to updating high, and ensure
> that high is fully updated before setting the real value of batch.
>
> Suggested by Gilad Ben-Yossef <gilad@...yossef.com> in this thread:
>
>         https://lkml.org/lkml/2013/4/9/23
>
> Also reproduces his proposed comment.
>
> Signed-off-by: Cody P Schafer <cody@...ux.vnet.ibm.com>
> ---
>  mm/page_alloc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d259599..a07bd4c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4007,11 +4007,26 @@ static int __meminit zone_batchsize(struct zone *zone)
>  #endif
>  }
>
> +static void pageset_update_prep(struct per_cpu_pages *pcp)
> +{
> +       /*
> +        * We're about to mess with PCP in an non atomic fashion.  Put an
> +        * intermediate safe value of batch and make sure it is visible before
> +        * any other change
> +        */
> +       pcp->batch = 1;
> +       smp_wmb();
> +}
> +
>  /* a companion to setup_pagelist_highmark() */
>  static void pageset_set_batch(struct per_cpu_pageset *p, unsigned long batch)
>  {
>         struct per_cpu_pages *pcp = &p->pcp;
> +       pageset_update_prep(pcp);
> +
>         pcp->high = 6 * batch;
> +       smp_wmb();
> +
>         pcp->batch = max(1UL, 1 * batch);
>  }
>
> @@ -4039,7 +4054,11 @@ static void setup_pagelist_highmark(struct per_cpu_pageset *p,
>         struct per_cpu_pages *pcp;
>
>         pcp = &p->pcp;
> +       pageset_update_prep(pcp);
> +
>         pcp->high = high;
> +       smp_wmb();
> +
>         pcp->batch = max(1UL, high/4);
>         if ((high/4) > (PAGE_SHIFT * 8))
>                 pcp->batch = PAGE_SHIFT * 8;
> --
> 1.8.2
>

That is very good.
However, now we've created a "protocol" for updating ->high and ->batch:

1. Call pageset_update_prep(pcp)
2. Update ->high
3. smp_wmb()
4. Update ->batch

But that protocol is not documented anywhere and someone  that reads
the code two
years from now will not be aware of it or why it is done like that.

How about if we create:

/*
* pcp->high and pcp->batch values are related and dependent on one another:
* ->batch must never be higher then ->high.
* The following function updates them in a safe manner without a
costly atomic transaction.
*/
static void pageset_update(struct per_cpu_pages *pcp, unsigned int
high, unsigned int batch)
{
       /* start with a fail safe value for batch */
       pcp->batch = 1;
       smp_wmb();

       /* Update high, then batch, in order */
       pcp->high = high;
       smp_wmb();
       pcp->batch = batch;
}

And use that at the update sites? then the protocol becomes explicit.

Thanks,
Gilad.

-- 
Gilad Ben-Yossef
Chief Coffee Drinker
gilad@...yossef.com
Israel Cell: +972-52-8260388
US Cell: +1-973-8260388
http://benyossef.com

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/