Re: [PATCH] memcg: remove trylock_page_cgroup

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 22 Apr 2009 12:16:41 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> How about this ? worth to be tested, I think.
> -Kame
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Before synchronized-LRU patch, mem cgroup had its own LRU lock.
> And there was a code which does
> # assume mz as per zone struct of memcg. 
> 
>    spin_lock mz->lru_lock
> 	lock_page_cgroup(pc).
>    and
>    lock_page_cgroup(pc)
> 	spin_lock mz->lru_lock
> 
> because we cannot locate "mz" until we see pc->page_cgroup, we used
> trylock(). But now, we don't have mz->lru_lock. All cgroup
> uses zone->lru_lock for handling list. Moreover, manipulation of
> LRU depends on global LRU now and we can isolate page from LRU by
> very generic way.(isolate_lru_page()).
> So, this kind of trylock is not necessary now.
> 
> I thought I removed all trylock in synchronized-LRU patch but there
> is still one. This patch removes trylock used in memcontrol.c and
> its definition. If someone needs, he should add this again with enough
> reason.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/page_cgroup.h |    5 -----
>  mm/memcontrol.c             |    3 +--
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> Index: mmotm-2.6.30-Apr21/include/linux/page_cgroup.h
> ===================================================================
> --- mmotm-2.6.30-Apr21.orig/include/linux/page_cgroup.h
> +++ mmotm-2.6.30-Apr21/include/linux/page_cgroup.h
> @@ -61,11 +61,6 @@ static inline void lock_page_cgroup(stru
>  	bit_spin_lock(PCG_LOCK, &pc->flags);
>  }
>  
> -static inline int trylock_page_cgroup(struct page_cgroup *pc)
> -{
> -	return bit_spin_trylock(PCG_LOCK, &pc->flags);
> -}
> -
>  static inline void unlock_page_cgroup(struct page_cgroup *pc)
>  {
>  	bit_spin_unlock(PCG_LOCK, &pc->flags);
> Index: mmotm-2.6.30-Apr21/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.30-Apr21.orig/mm/memcontrol.c
> +++ mmotm-2.6.30-Apr21/mm/memcontrol.c
> @@ -1148,8 +1148,7 @@ static int mem_cgroup_move_account(struc
>  	from_mz =  mem_cgroup_zoneinfo(from, nid, zid);
>  	to_mz =  mem_cgroup_zoneinfo(to, nid, zid);
>  
> -	if (!trylock_page_cgroup(pc))
> -		return ret;
> +	lock_page_cgroup(pc);
>  
>  	if (!PageCgroupUsed(pc))
>  		goto out;

But we can't remove that nasty `while (loop--)' thing?

I expect that it will reliably fail if the caller is running as
SCHED_FIFO and the machine is single-CPU, or if we're trying to yield
to a SCHED_OTHER task which is pinned to this CPU, etc.  The cond_resched()
won't work.