lkml.org 
[lkml]   [2012]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 06/43] mm: numa: Make pte_numa() and pmd_numa() a generic implementation
On Fri, Nov 16, 2012 at 07:32:01AM -0800, Linus Torvalds wrote:
> On Fri, Nov 16, 2012 at 6:41 AM, Mel Gorman <mgorman@suse.de> wrote:
> >
> > I would have preferred asm-generic/pgtable.h myself and use
> > __HAVE_ARCH_whatever tricks
>
> PLEASE NO!
>
> Dammit, why is this disease still so prevalent, and why do people
> continue to do this crap?
>

By personal experience because they read the header, see the other examples
and say "fair enough". I'm tempted to...

diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index da3e761..572d3f1 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -7,6 +7,12 @@
#include <linux/mm_types.h>
#include <linux/bug.h>

+/*
+ * NOTE: Do NOT copy the __HAVE_ARCH convention when adding new generic
+ * helpers. You will have to wear a D hat and be called names
+ * https://lkml.org/lkml/2012/11/16/340
+ */
+
#ifndef __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
extern int ptep_set_access_flags(struct vm_area_struct *vma,
unsigned long address, pte_t *ptep,

> __HAVE_ARCH_xyzzy is a f*cking retarded thing to do, and that's
> actually an insult to retarded people.
>
> Use either:
>
> - Kconfig entries for bigger features where that makes sense, and
> using the Kconfig files allows you to use the Kconfig logic for things
> (ie there are dependencies etc, so you can avoid having to have
> complicated conditionals in the #ifdef's, and instead introduce them
> as rules in Kconfig files).
>
> - the SAME F*CKING NAME for the #ifdef, not some totally different
> namespace with __HAVE_ARCH_xyzzy crap.
>
> So if your architecture wants to override one (or more) of the
> pte_*numa() functions, just make it do so. And do it with
>
> static inline pmd_t pmd_mknuma(pmd_t pmd)
> {
> pmd = pmd_set_flags(pmd, _PAGE_NUMA);
> return pmd_clear_flags(pmd, _PAGE_PRESENT);
> }
> #define pmd_mknuma pmd_mknuma
>
> and then you can have the generic code have code like
>
> #ifndef pmd_mknuma
> .. generic version goes here ..
> #endif
>
> and the advantage is two-fold:
>
> - none of the "let's make up another name to test for this"
>
> - "git grep" actually _works_, and the end results make sense, and
> you can clearly see the logic of where things are declared, and which
> one is used.
>

Understood, makes sense and is a straight-forward conversion. Now that I
read this, this explanation feels familiar. Clearly it did not sink in
with me when you shouted at the last person that tried.

> The __ARCH_HAVE_xyzzy (and some places call it __HAVE_ARCH_xyzzy)
> thing is a disease.
>

And now I have been healed! I've had worse starts to a weekend.

> That said, the __weak thing works too (and greps fine, as long as you
> use the proper K&R C format, not the idiotic "let's put the name of
> the function on a different line than the type of the function"
> format), it just doesn't allow inlining.
>
> In this case, I suspect the inlined function is generally a single
> instruction, is it not? In which case I really do think that inlining
> makes sense.
>

I would expect a single instruction for the checks (pte_numa, pmd_numa).
It's probably two for the setters (pte_mknuma, pmd_mknuma, pte_mknonnuma,
pmd_mknonnuma) unless paravirt gets involved. paravirt might add a
function call in there but should be nothing crazy.

--
Mel Gorman
SUSE Labs


\
 
 \ /
  Last update: 2012-11-16 18:01    [W:0.145 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site