From: Ingo Molnar <mingo@kernel.org>
To: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>, X86 ML <x86@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Borislav Petkov <bpetkov@suse.de>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
Mel Gorman <mgorman@suse.de>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
Rik van Riel <riel@redhat.com>,
Dave Hansen <dave.hansen@intel.com>,
Nadav Amit <namit@vmware.com>, Michal Hocko <mhocko@suse.com>,
Arjan van de Ven <arjan@linux.intel.com>
Subject: Re: [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm
Date: Thu, 11 May 2017 09:13:48 +0200 [thread overview]
Message-ID: <20170511071348.jhgzdgi7blhgenqj@gmail.com> (raw)
In-Reply-To: <CALCETrV-c8n92v040HVw=6OdnNrLvN7ZAcAJ45Xs4wx-7H5r=g@mail.gmail.com>
* Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, May 10, 2017 at 1:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >> On Wed, 10 May 2017, Ingo Molnar wrote:
> >> >
> >> > * Thomas Gleixner <tglx@linutronix.de> 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
next prev parent reply other threads:[~2017-05-11 7:13 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-07 12:38 [RFC 00/10] x86 TLB flush cleanups, moving toward PCID support Andy Lutomirski
2017-05-07 12:38 ` [RFC 01/10] x86/mm: Reimplement flush_tlb_page() using flush_tlb_mm_range() Andy Lutomirski
2017-05-11 17:41 ` Borislav Petkov
2017-05-12 3:35 ` Andy Lutomirski
2017-05-07 12:38 ` [RFC 02/10] x86/mm: Reduce indentation in flush_tlb_func() Andy Lutomirski
2017-05-07 12:38 ` [RFC 03/10] x86/mm: Make the batched unmap TLB flush API more generic Andy Lutomirski
2017-05-08 15:34 ` Dave Hansen
2017-05-09 13:02 ` Andy Lutomirski
2017-05-09 14:39 ` Mel Gorman
2017-05-09 17:13 ` Dave Hansen
2017-05-09 22:54 ` Andy Lutomirski
2017-05-07 12:38 ` [RFC 04/10] x86/mm: Pass flush_tlb_info to flush_tlb_others() etc Andy Lutomirski
2017-05-11 20:01 ` Nadav Amit
2017-05-12 3:41 ` Andy Lutomirski
2017-05-07 12:38 ` [RFC 05/10] x86/mm: Change the leave_mm() condition for local TLB flushes Andy Lutomirski
2017-05-07 12:38 ` [RFC 06/10] x86/mm: Refactor flush_tlb_mm_range() to merge local and remote cases Andy Lutomirski
2017-05-07 12:38 ` [RFC 07/10] x86/mm: Use new merged flush logic in arch_tlbbatch_flush() Andy Lutomirski
2017-05-07 12:38 ` [RFC 08/10] x86/mm: Remove the UP tlbflush code; always use the formerly SMP code Andy Lutomirski
2017-05-07 12:38 ` [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm Andy Lutomirski
2017-05-09 20:41 ` Thomas Gleixner
2017-05-09 22:54 ` Andy Lutomirski
2017-05-10 5:57 ` Ingo Molnar
2017-05-10 8:19 ` Thomas Gleixner
2017-05-10 8:24 ` Ingo Molnar
2017-05-10 22:42 ` Andy Lutomirski
2017-05-11 7:13 ` Ingo Molnar [this message]
2017-05-12 3:36 ` Andy Lutomirski
2017-05-07 12:38 ` [RFC 10/10] x86,kvm: Teach KVM's VMX code that CR3 isn't a constant Andy Lutomirski
2017-05-07 13:00 ` [RFC 00/10] x86 TLB flush cleanups, moving toward PCID support Ingo Molnar
2017-05-07 16:05 ` Linus Torvalds
2017-05-08 16:36 ` Nadav Amit
2017-05-09 12:43 ` Andy Lutomirski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170511071348.jhgzdgi7blhgenqj@gmail.com \
--to=mingo@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=arjan@linux.intel.com \
--cc=bpetkov@suse.de \
--cc=dave.hansen@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=mgorman@suse.de \
--cc=mhocko@suse.com \
--cc=namit@vmware.com \
--cc=riel@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).