Messages in this thread | | | Date | Fri, 20 May 2011 12:30:04 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH v2 3/3] vmscan: implement swap token priority aging |
| |
On Thu, 19 May 2011 11:34:15 +0900 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> While testing for memcg aware swap token, I observed a swap token > was often grabbed an intermittent running process (eg init, auditd) > and they never release a token. > > Why? > > Some processes (eg init, auditd, audispd) wake up when a process > exiting. And swap token can be get first page-in process when > a process exiting makes no swap token owner. Thus such above > intermittent running process often get a token. > > And currently, swap token priority is only decreased at page fault > path. Then, if the process sleep immediately after to grab swap > token, the swap token priority never be decreased. That's obviously > undesirable. > > This patch implement very poor (and lightweight) priority aging. > It only be affect to the above corner case and doesn't change swap > tendency workload performance (eg multi process qsbench load) > > ... > > --- a/mm/thrash.c > +++ b/mm/thrash.c > @@ -25,10 +25,13 @@ > > #include <trace/events/vmscan.h> > > +#define TOKEN_AGING_INTERVAL (0xFF)
Needs a comment describing its units and what it does, please. Sufficient for readers to understand why this value was chosen and what effect they could expect to see from changing it.
> static DEFINE_SPINLOCK(swap_token_lock); > struct mm_struct *swap_token_mm; > struct mem_cgroup *swap_token_memcg; > static unsigned int global_faults; > +static unsigned int last_aging;
Is this a good name? Would something like prev_global_faults be better?
`global_faults' and `last_aging' could be made static local in grab_swap_token().
> void grab_swap_token(struct mm_struct *mm) > { > @@ -47,6 +50,11 @@ void grab_swap_token(struct mm_struct *mm) > if (!swap_token_mm) > goto replace_token; > > + if ((global_faults - last_aging) > TOKEN_AGING_INTERVAL) { > + swap_token_mm->token_priority /= 2; > + last_aging = global_faults; > + }
It's really hard to reverse-engineer the design decisions from the implementation here, therefore... ?
> if (mm == swap_token_mm) { > mm->token_priority += 2; > goto update_priority; > @@ -64,7 +72,7 @@ void grab_swap_token(struct mm_struct *mm) > goto replace_token; > > update_priority: > - trace_update_swap_token_priority(mm, old_prio); > + trace_update_swap_token_priority(mm, old_prio, swap_token_mm); > > out: > mm->faultstamp = global_faults; > @@ -80,6 +88,7 @@ replace_token: > trace_replace_swap_token(swap_token_mm, mm); > swap_token_mm = mm; > swap_token_memcg = memcg; > + last_aging = global_faults; > goto out; > }
In fact all of grab_swap_token() and the thrash-detection code in general are pretty tricky and unobvious stuff. So we left it undocumented :(
| |