linux-kernel - Re: [PATCH] mm: vmscan: deactivate isolated pages with lru lock released

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: <alpine.LSU.2.00.1201111351080.1846@eggly.anvils>
Date:	Wed, 11 Jan 2012 14:33:34 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Hillf Danton <dhillf@...il.com>
cc:	linux-mm@...ck.org,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	David Rientjes <rientjes@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: vmscan: deactivate isolated pages with lru lock
 released

On Wed, 11 Jan 2012, Hillf Danton wrote:

> Spinners on other CPUs, if any, could take the lru lock and do their jobs while
> isolated pages are deactivated on the current CPU if the lock is released
> actively. And no risk of race raised as pages are already queued on locally
> private list.

You make a good point - except, I'm afraid as usual, I have difficulty
in understanding your comment, in separating how it is before your change
and how it is after your change.  Above you're describing how it is after
your change; and it would help if you point out that you're taking the
lock off clear_active_flags(), which goes all the way down the list of
pages we isolated (to a locally private list, yes, important point).

However... this patch is based on Linus's current, and will clash with a
patch of mine presently in akpm's tree - which I'm expecting will go on
to Linus soon, unless Andrew discards it in favour of yours (that might
involve a little unravelling, I didn't look).  Among other rearrangements,
I merged the code from clear_active_flags() into update_isolated_counts().

And something that worries me is that you're now dropping the spinlock
and reacquiring it shortly afterwards, just clear_active_flags in between.
That may bounce the lock around more than before, and actually prove worse.

I suspect that your patch can be improved, to take away that worry.
Why do we need to take the lock again?  Only to update reclaim_stat:
for the other stats, interrupts disabled is certainly good enough,
and more research might show that preemption disabled would be enough.

get_scan_count() is called at the (re)start of shrink_mem_cgroup_zone(),
before it goes down to do shrink_list()s: I think it would not be harmed
at all if we delayed updating reclaim_stat->recent_scanned until the
next time we take the lock, lower down.

Other things that strike me, looking here again: isn't it the case that
update_isolated_counts() is actually called either for file or for anon,
but never for both?  We might be able to make savings from that, perhaps
enlist help from isolate_lru_pages() to avoid having to go down the list
again to clear active flags.

I certainly do have more changes to make around here, in changing the
locking over to be per-memcg (and the locking on reclaim_stat is something
I have not got quite right yet): but if you've a good patch to reduce the
locking, I should rebase on top of yours.

Hugh

> 
> 
> Signed-off-by: Hillf Danton <dhillf@...il.com>
> ---
> 
> --- a/mm/vmscan.c	Thu Dec 29 20:20:16 2011
> +++ b/mm/vmscan.c	Wed Jan 11 20:40:40 2012
> @@ -1464,6 +1464,7 @@ update_isolated_counts(struct mem_cgroup
>  	struct zone_reclaim_stat *reclaim_stat = get_reclaim_stat(mz);
> 
>  	nr_active = clear_active_flags(isolated_list, count);
> +	spin_lock_irq(&zone->lru_lock);
>  	__count_vm_events(PGDEACTIVATE, nr_active);
> 
>  	__mod_zone_page_state(zone, NR_ACTIVE_FILE,
> @@ -1482,6 +1483,7 @@ update_isolated_counts(struct mem_cgroup
> 
>  	reclaim_stat->recent_scanned[0] += *nr_anon;
>  	reclaim_stat->recent_scanned[1] += *nr_file;
> +	spin_unlock_irq(&zone->lru_lock);
>  }
> 
>  /*
> @@ -1577,15 +1579,13 @@ shrink_inactive_list(unsigned long nr_to
>  			__count_zone_vm_events(PGSCAN_DIRECT, zone,
>  					       nr_scanned);
>  	}
> +	spin_unlock_irq(&zone->lru_lock);
> 
> -	if (nr_taken == 0) {
> -		spin_unlock_irq(&zone->lru_lock);
> +	if (nr_taken == 0)
>  		return 0;
> -	}
> 
>  	update_isolated_counts(mz, sc, &nr_anon, &nr_file, &page_list);
> 
> -	spin_unlock_irq(&zone->lru_lock);
> 
>  	nr_reclaimed = shrink_page_list(&page_list, mz, sc, priority,
>  						&nr_dirty, &nr_writeback);
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.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: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
> 
--
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/