On Fri, Apr 22, 2011 at 1:44 AM, KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> wrote:
> > > @@ -111,6 +113,8 @@ struct scan_control {ok.
> > > * are scanned.
> > > */
> > > nodemask_t *nodemask;
> > > +
> > > + int priority;
> > > };
> >
> > Bah!
> > If you need sc.priority, you have to make cleanup patch at first. and
> > all current reclaim path have to use sc.priority. Please don't increase
> > unnecessary mess.
> >
> > hmm. so then I would change it by passing the priority
> > as separate parameter.
I meant completely cut-n-paste code and comments is here.
> > > + /*
> > > + * If we've done a decent amount of scanning and
> > > + * the reclaim ratio is low, start doing writepage
> > > + * even in laptop mode
> > > + */
> > > + if (total_scanned > SWAP_CLUSTER_MAX * 2 &&
> > > + total_scanned > sc->nr_reclaimed + sc->nr_reclaimed /
> > 2) {
> > > + sc->may_writepage = 1;
> >
> > please make helper function for may_writepage. iow, don't cut-n-paste.
> >
> > hmm, can you help to clarify that?
> > > + total_scanned = 0;
> > > +
> > > + do_nodes = node_states[N_ONLINE];
> >
> > Why do we need care memoryless node? N_HIGH_MEMORY is wrong?
> >
> hmm, let me look into that.
> > > + sc.priority = priority;Why?
> > > + /* The swap token gets in the way of swapout... */
> > > + if (!priority)
> > > + disable_swap_token();
> >
> > Why?
> >
> > disable swap token mean "Please devest swap preventation privilege from
> > owner task. Instead we endure swap storm and performance hit".
> > However I doublt memcg memory shortage is good situation to make swap
> > storm.
> >
>
> I am not sure about that either way. we probably can leave as it is and make
> corresponding change if real problem is observed?
This is not only memcg issue, but also can lead to global swap ping-pong.
But I give up. I have no time to persuade you.
Thank you for pointing that out. I didn't pay much attention on the swap_token but just simply inherited
it from the global logic. Now after reading a bit more, i think you were right about it. It would be a bad
idea to have memcg kswapds affecting much the global swap token being set.
I will remove it from the next post.
Same with above N_HIGH_MEMORY comments.
> > > + nid = mem_cgroup_select_victim_node(mem_cont,
> > > + &do_nodes);
> > > +
> > > + pgdat = NODE_DATA(nid);
> > > + shrink_memcg_node(pgdat, order, &sc);
> > > + total_scanned += sc.nr_scanned;
> > > +
> > > + for (i = pgdat->nr_zones - 1; i >= 0; i--) {
> > > + struct zone *zone = pgdat->node_zones + i;
> > > +
> > > + if (populated_zone(zone))
> > > + break;
> > > + }
> >
> > memory less node check is here. but we can check it before.
>
> Not sure I understand this, can you help to clarify?
Ok, agree on the HIGH_MEMORY and will change that.
--Ying