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

[Ying Han <yinghan@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3428 bytes --]

On Fri, Apr 22, 2011 at 1:44 AM, KOSAKI Motohiro <
kosaki.motohiro@jp.fujitsu.com> wrote:

> > > > @@ -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.
>
> 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.

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

Ok, agree on the HIGH_MEMORY and will change that.

--Ying

[-- Attachment #2: Type: text/html, Size: 4841 bytes --]