Messages in this thread | | | Subject | Re: Re: Re: [PATCH 4/13] memcg: force_empty moving account | From | Peter Zijlstra <> | Date | Mon, 22 Sep 2008 17:32:48 +0200 |
| |
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.
| |