Re: [PATCH] Synchronize task mm counters on context switch

[Daniel Colascione <dancol@google.com>]

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

On Feb 22, 2018 1:40 AM, "Peter Zijlstra" <peterz@infradead.org> wrote:

On Thu, Feb 22, 2018 at 11:06:33AM +0900, Minchan Kim wrote:
> On Wed, Feb 21, 2018 at 04:23:43PM -0800, Daniel Colascione wrote:
> >  kernel/sched/core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index a7bf32aabfda..7f197a7698ee 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -3429,6 +3429,9 @@ asmlinkage __visible void __sched schedule(void)
> >         struct task_struct *tsk = current;
> >
> >         sched_submit_work(tsk);
> > +       if (tsk->mm)
> > +               sync_mm_rss(tsk->mm);
> > +
> >         do {
> >                 preempt_disable();
> >                 __schedule(false);
> >

Obviously I completely hate that; and you really _should_ have Cc'ed me
earlier ;-)


I thought I might get a reaction like that. :-)


That it still well over 100 cycles in the case when all counters did
change. Far _far_ more if the mm counters are contended (up to 150 times
more is quite possible).


I suppose it doesn't help to sync the counters only when dirty, detecting
this situation with a task status flag or something?

> > > > Ping? Is this approach just a bad idea? We could instead just
manually sync
> > > > all mm-attached tasks at counter-retrieval time.
> > >
> > > IMHO, yes, it should be done when user want to see which would be
really
> > > cold path while this shecule function is hot.
> > >
> >
> > The problem with doing it that way is that we need to look at each task
> > attached to a particular mm. AFAIK (and please tell me if I'm wrong),
the
> > only way to do that is to iterate over all processes, and for each
process
> > attached to the mm we want, iterate over all its tasks (since each one
has
> > to have the same mm, I think). Does that sound right?

You could just iterate the thread group and call it a day. Yes strictly
speaking its possible to have mm's shared outside the thread group,
practically that 'never' happens.

CLONE_VM without CLONE_THREAD just isn't a popular thing afaik.

So while its not perfect, it might well be good enough.


Take a look at the other patch I posted. Seems to work.

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