* 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 = ¤t->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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
