Re: [PATCH V7 7/9] Per-memcg background reclaim.

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

> > > @@ -111,6 +113,8 @@ struct scan_control {
> > >        * 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.

ok.

> > > +             /*
> > > +              * 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?

I meant completely cut-n-paste code and comments is here.


> > > +     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;
> > > +             /* 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?

Why?
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.


> > > +                     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?

Same with above N_HIGH_MEMORY comments.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>