Re: Re: Re: [PATCH 4/13] memcg: force_empty moving account

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Tue, 2008-09-23 at 00:06 +0900, kamezawa.hiroyu@jp.fujitsu.com wrote:
> ----- Original Message -----
> >On Mon, 2008-09-22 at 23:50 +0900, kamezawa.hiroyu@jp.fujitsu.com wrote:
> >> ----- Original Message -----
> >> >> +			spin_lock_irqsave(&mz->lru_lock, flags);
> >> >> +		} else {
> >> >> +			unlock_page(page);
> >> >> +			put_page(page);
> >> >> +		}
> >> >> +		if (atomic_read(&mem->css.cgroup->count) > 0)
> >> >> +			break;
> >> >>  	}
> >> >>  	spin_unlock_irqrestore(&mz->lru_lock, flags);
> >> >
> >> >do _NOT_ use yield() ever! unless you know what you're doing, and
> >> >probably not even then.
> >> >
> >> >NAK!
> >> Hmm, sorry. cond_resched() is ok ?
> >
> >depends on what you want to do, please explain what you're trying to do.
> >
> Sorry again.
> 
> This force_empty is called only in following situation
>  - there is no user threas in this cgroup.
>  - a user tries to rmdir() this cgroup or explicitly type
>    echo 1 > ../memory.force_empty.
> 
> force_empty() scans lru list of this cgroup and check page_cgroup on the
> list one by one. Because there are no tasks in this group, force_empty can
> see following racy condtions while scanning.
> 
>  - global lru tries to remove the page which pointed by page_cgroup 
>    and it is not-on-LRU.

So you either skip the page because it already got un-accounted, or you
retry because its state is already updated to some new state.

>  - the page is locked by someone.
>    ....find some lock contetion with invalidation/truncate.

Then you just contend the lock and get woken when you obtain?

>  - in later patch, page_cgroup can be on pagevec(i added) and we have to drain
>    it to remove from LRU.

Then unlock, drain, lock, no need to sleep some arbitrary amount of time
[0-inf).

> In above situation, force_empty() have to wait for some event proceeds.
> 
> Hmm...detecting busy situation in loop and sleep in out-side-of-loop
> is better ? Anyway, ok, I'll rewrite this.

The better solution is to wait for events in a non-polling fashion, for
example by using wait_event().

yield() might not actually wait at all, suppose you're the highest
priority FIFO task on the system - if you used yield and rely on someone
else to run you'll deadlock.

Also, depending on sysctl_sched_compat_yield, SCHED_OTHER tasks using
yield() can behave radically different.

> BTW, sched.c::yield() is for what purpose now ?

There are some (lagacy) users of yield, sadly they are all incorrect,
but removing them is non-trivial for various reasons.

The -rt kernel has 2 sites where yield() is the correct thing to do. In
both cases its where 2 SCHED_FIFO-99 tasks (migration and stop_machine)
depend on each-other.