> On Wed, Sep 01, 2010 at 08:37:41AM +0900, KOSAKI Motohiro wrote:
> > > +#ifdef CONFIG_SMP
> > > +/* Called when a more accurate view of NR_FREE_PAGES is needed */
> > > +unsigned long zone_nr_free_pages(struct zone *zone)
> > > +{
> > > + unsigned long nr_free_pages = zone_page_state(zone, NR_FREE_PAGES);
> > > +
> > > + /*
> > > + * While kswapd is awake, it is considered the zone is under some
> > > + * memory pressure. Under pressure, there is a risk that
> > > + * per-cpu-counter-drift will allow the min watermark to be breached
> > > + * potentially causing a live-lock. While kswapd is awake and
> > > + * free pages are low, get a better estimate for free pages
> > > + */
> > > + if (nr_free_pages < zone->percpu_drift_mark &&
> > > + !waitqueue_active(&zone->zone_pgdat->kswapd_wait)) {
> > > + int cpu;
> > > +
> > > + for_each_online_cpu(cpu) {
> > > + struct per_cpu_pageset *pset;
> > > +
> > > + pset = per_cpu_ptr(zone->pageset, cpu);
> > > + nr_free_pages += pset->vm_stat_diff[NR_FREE_PAGES];
> >
> > If my understanding is correct, we have no lock when reading pset->vm_stat_diff.
> > It mean nr_free_pages can reach negative value at very rarely race. boundary
> > check is necessary?
> >
>
> True, well spotted.
>
> How about the following? It records a delta and checks if delta is negative
> and would cause underflow.
>
> unsigned long zone_nr_free_pages(struct zone *zone)
> {
> unsigned long nr_free_pages = zone_page_state(zone, NR_FREE_PAGES);
> long delta = 0;
>
> /*
> * While kswapd is awake, it is considered the zone is under some
> * memory pressure. Under pressure, there is a risk that
> * per-cpu-counter-drift will allow the min watermark to be breached
> * potentially causing a live-lock. While kswapd is awake and
> * free pages are low, get a better estimate for free pages
> */
> if (nr_free_pages < zone->percpu_drift_mark &&
> !waitqueue_active(&zone->zone_pgdat->kswapd_wait)) {
> int cpu;
>
> for_each_online_cpu(cpu) {
> struct per_cpu_pageset *pset;
>
> pset = per_cpu_ptr(zone->pageset, cpu);
> delta += pset->vm_stat_diff[NR_FREE_PAGES];
> }
> }
>
> /* Watch for underflow */
> if (delta < 0 && abs(delta) > nr_free_pages)
> delta = -nr_free_pages;
>
> return nr_free_pages + delta;
> }
Looks good to me :)
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>