Re: [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm

[Ingo Molnar]

* Andy Lutomirski <luto@xxxxxxxxxx> wrote:

> On Wed, May 10, 2017 at 1:24 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> >
> > * Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >
> >> On Wed, 10 May 2017, Ingo Molnar wrote:
> >> >
> >> > * Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >> >
> >> > > On Sun, 7 May 2017, Andy Lutomirski wrote:
> >> > > >  /* context.lock is held for us, so we don't need any locking. */
> >> > > >  static void flush_ldt(void *current_mm)
> >> > > >  {
> >> > > > +       struct mm_struct *mm = current_mm;
> >> > > >         mm_context_t *pc;
> >> > > >
> >> > > > -       if (current->active_mm != current_mm)
> >> > > > +       if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> >> > >
> >> > > While functional correct, this really should compare against 'mm'.
> >> > >
> >> > > >                 return;
> >> > > >
> >> > > > -       pc = &current->active_mm->context;
> >> > > > +       pc = &mm->context;
> >> >
> >> > So this appears to be the function:
> >> >
> >> >  static void flush_ldt(void *current_mm)
> >> >  {
> >> >         struct mm_struct *mm = current_mm;
> >> >         mm_context_t *pc;
> >> >
> >> >         if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> >> >                 return;
> >> >
> >> >         pc = &mm->context;
> >> >         set_ldt(pc->ldt->entries, pc->ldt->size);
> >> >  }
> >> >
> >> > why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?
> >>
> >> Because you cannot dereference a void pointer, i.e. &mm->context ....
> >
> > Indeed, doh! The naming totally confused me. The way I'd write it is the canonical
> > form for such callbacks:
> >
> >         static void flush_ldt(void *data)
> >         {
> >                 struct mm_struct *mm = data;
> >
> > ... which beyond unconfusing me would probably also have prevented any accidental
> > use of the 'current_mm' callback argument.
> >
> >
> 
> void *data and void *info both seem fairly common in the kernel.

Yes, the most common variants are:

  triton:~/tip> git grep -E 'void.*\(.*void \*.*' | grep -vE ',|\*\*|;' | cut -d\( -f2- | cut -d\) -f1 | sort | uniq -c | sort -n | tail -10
     38 void *args
     38 void *p
     39 void *ptr
     42 void *foo
     46 void *context
     55 void *addr
     69 void *priv
     95 void *info
    235 void *arg
    292 void *data

> How about my personal favorite for non-kernel work, though: void *mm_void? It 
> documents what the parameter means and avoids the confusion.

Dunno, and at the risk of painting that shed bright red it reads a bit weird to 
me: void pointers are fine and are often primary parameters - the _real_ quality 
here is not that it's void, but that's it's an opaque value passed in from a 
common callback. Note that sometimes opaque data is 'unsigned long' (such as in 
the case of timers), so it's really not the 'void' that matters.

In that sense 'data', 'arg' or 'info' seem the most readable names, as they 
clearly express the type opaqueness.

My personal favorite is double underscores prefix, i.e. 'void *__mm', which would 
clearly signal that this is something special. But this does not appear to have 
been picked up overly widely:

  triton:~/tip> git grep -E 'void.*\(.*void \*.*' | grep -vE ',|\*\*|;' | cut -d\( -f2- | cut -d\) -f1 | sort | uniq -c | sort -n | grep __
      1 void *__data
      1 void *__info
      2 void *__dev
      2 void *__tdata
      2 void *__tve
      3 void *__lock
      3 void * __user *
      3 volatile void *__p
      4 void *__map

... but either of these variants is fine to me.

Thanks,

	Ingo