Re: [PATCH 06/43] mm: numa: Make pte_numa() and pmd_numa() a genericimplementation

[Mel Gorman]

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@xxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/