Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, Feb 07, 2014 at 05:02:02PM -0800, David Rientjes wrote:
> On Fri, 7 Feb 2014, Josh Triplett wrote:
>
> > > Why?? If CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID then, yes, we need it to be
> > > global. Otherwise it's perfectly fine just being static in file scope.
> > > This causes the compilation unit to break when you compile it, not wait
> > > until vmlinux and find undefined references.
> > >
> > > I see no reason it can't be done like this in mm/page_alloc.c:
> > >
> > > #ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > > extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> >
> > No, a .c file should not have an extern declaration in it. This should
> > live in an appropriate header file, to be included in both page_alloc.c
> > and any arch file that defines an overriding function.
> >
>
> Ok, so you have religious beliefs about extern being used in files ending
> in .c and don't mind the 2900 occurrences of it in the kernel tree and
> desire 14 line obfuscation in header files with comments to what is being
> defined in .c files such as "please see mm/page_alloc.c" as mm.h has.
I (and many others) have very specific technical objections to not
having the same prototype seen by both the definition and use of a
function: that helps keep the prototype in sync with the definition(s),
helps keep all definitions in sync if there are multiple such
definitions, and eliminates -Wmissing-prototype warnings (which in
addition to those benefits also help detect functions that should be
made static or eliminated).
And as mentioned before, those 14 lines can be significantly simplified;
Rashika's patch already does one such simplification.
Those 2900 occurrences should go away as well, and Rashika's previous
patches have already fixed many of them.
> > > Both of these options look much better than
> > >
> > > include/linux/mm.h:
> > >
> > > #if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
> > > !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
> > > static inline int __early_pfn_to_nid(unsigned long pfn)
> > > {
> > > return 0;
> > > }
> > > #else
> > > /* please see mm/page_alloc.c */
> > > extern int __meminit early_pfn_to_nid(unsigned long pfn);
> > > #ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > > /* there is a per-arch backend function. */
> > > extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> > > #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > > #endif
> > >
> > > where all this confusion is originating from.
> >
> > The proposal is to first simplify those ifdefs by eliminating the inner
> > one in the #else; I agree with Andrew that we ought to go ahead and take
> > that step given the patch at hand, and then figure out if there's an
> > additional simplification possible.
> >
>
> If additional simplification is possible? Yeah, it's __weak which is
> designed for this purpose.
No objections here if someone wants to write that patch.
- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, 7 Feb 2014, Josh Triplett wrote:
> > Why?? If CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID then, yes, we need it to be
> > global. Otherwise it's perfectly fine just being static in file scope.
> > This causes the compilation unit to break when you compile it, not wait
> > until vmlinux and find undefined references.
> >
> > I see no reason it can't be done like this in mm/page_alloc.c:
> >
> > #ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > extern int __meminit __early_pfn_to_nid(unsigned long pfn);
>
> No, a .c file should not have an extern declaration in it. This should
> live in an appropriate header file, to be included in both page_alloc.c
> and any arch file that defines an overriding function.
>
Ok, so you have religious beliefs about extern being used in files ending
in .c and don't mind the 2900 occurrences of it in the kernel tree and
desire 14 line obfuscation in header files with comments to what is being
defined in .c files such as "please see mm/page_alloc.c" as mm.h has.
Good point.
> > Both of these options look much better than
> >
> > include/linux/mm.h:
> >
> > #if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
> > !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
> > static inline int __early_pfn_to_nid(unsigned long pfn)
> > {
> > return 0;
> > }
> > #else
> > /* please see mm/page_alloc.c */
> > extern int __meminit early_pfn_to_nid(unsigned long pfn);
> > #ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > /* there is a per-arch backend function. */
> > extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> > #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > #endif
> >
> > where all this confusion is originating from.
>
> The proposal is to first simplify those ifdefs by eliminating the inner
> one in the #else; I agree with Andrew that we ought to go ahead and take
> that step given the patch at hand, and then figure out if there's an
> additional simplification possible.
>
If additional simplification is possible? Yeah, it's __weak which is
designed for this purpose.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, Feb 07, 2014 at 03:09:09PM -0800, David Rientjes wrote:
> On Fri, 7 Feb 2014, Andrew Morton wrote:
>
> > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > > index 1cedd00..5f8348f 100644
> > > > > > --- a/include/linux/mm.h
> > > > > > +++ b/include/linux/mm.h
> > > > > > @@ -1589,10 +1589,8 @@ static inline int
> > > > > > __early_pfn_to_nid(unsigned long pfn)
> > > > > > #else
> > > > > > /* please see mm/page_alloc.c */
> > > > > > extern int __meminit early_pfn_to_nid(unsigned long pfn);
> > > > > > -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > > > > > /* there is a per-arch backend function. */
> > > > > > extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> > > > > > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > > > > > #endif
> > > > > >
> > > > > > extern void set_dma_reserve(unsigned long new_dma_reserve);
> > > > >
> > > > > Wouldn't it be better to just declare the __early_pfn_to_nid() in
> > > > > mm/page_alloc.c to be static?
> > > >
> > > > Won't that break the ability to override that function in
> > > > architecture-specific code (as arch/ia64/mm/numa.c does)?
> > > >
> > >
> > > Why? CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID should define where this function
> > > is defined so ia64 should have it set and the definition which I'm
> > > suggesting be static is only compiled when this is undefined in
> > > mm/page_alloc.c. I'm not sure why we'd want to be messing with the
> > > declaration?
> >
> > __early_pfn_to_nid() must be global if it is implemented in arch/.
> >
>
> Why?? If CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID then, yes, we need it to be
> global. Otherwise it's perfectly fine just being static in file scope.
> This causes the compilation unit to break when you compile it, not wait
> until vmlinux and find undefined references.
>
> I see no reason it can't be done like this in mm/page_alloc.c:
>
> #ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> extern int __meminit __early_pfn_to_nid(unsigned long pfn);
No, a .c file should not have an extern declaration in it. This should
live in an appropriate header file, to be included in both page_alloc.c
and any arch file that defines an overriding function.
> Both of these options look much better than
>
> include/linux/mm.h:
>
> #if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
> !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
> static inline int __early_pfn_to_nid(unsigned long pfn)
> {
> return 0;
> }
> #else
> /* please see mm/page_alloc.c */
> extern int __meminit early_pfn_to_nid(unsigned long pfn);
> #ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> /* there is a per-arch backend function. */
> extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> #endif
>
> where all this confusion is originating from.
The proposal is to first simplify those ifdefs by eliminating the inner
one in the #else; I agree with Andrew that we ought to go ahead and take
that step given the patch at hand, and then figure out if there's an
additional simplification possible.
- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, 7 Feb 2014, Andrew Morton wrote:
> > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > index 1cedd00..5f8348f 100644
> > > > > --- a/include/linux/mm.h
> > > > > +++ b/include/linux/mm.h
> > > > > @@ -1589,10 +1589,8 @@ static inline int __early_pfn_to_nid(unsigned
> > > > > long pfn)
> > > > > #else
> > > > > /* please see mm/page_alloc.c */
> > > > > extern int __meminit early_pfn_to_nid(unsigned long pfn);
> > > > > -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > > > > /* there is a per-arch backend function. */
> > > > > extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> > > > > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > > > > #endif
> > > > >
> > > > > extern void set_dma_reserve(unsigned long new_dma_reserve);
> > > >
> > > > Wouldn't it be better to just declare the __early_pfn_to_nid() in
> > > > mm/page_alloc.c to be static?
> > >
> > > Won't that break the ability to override that function in
> > > architecture-specific code (as arch/ia64/mm/numa.c does)?
> > >
> >
> > Why? CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID should define where this function
> > is defined so ia64 should have it set and the definition which I'm
> > suggesting be static is only compiled when this is undefined in
> > mm/page_alloc.c. I'm not sure why we'd want to be messing with the
> > declaration?
>
> __early_pfn_to_nid() must be global if it is implemented in arch/.
>
Why?? If CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID then, yes, we need it to be
global. Otherwise it's perfectly fine just being static in file scope.
This causes the compilation unit to break when you compile it, not wait
until vmlinux and find undefined references.
I see no reason it can't be done like this in mm/page_alloc.c:
#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
extern int __meminit __early_pfn_to_nid(unsigned long pfn);
#else
static int __meminit __early_pfn_to_nid(unsigned long pfn)
{
...
}
or delcare __early_pfn_to_nid() to have __attribute__((weak)) and override
it when CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID (and get rid of the pointless
CONFIG option entirely at that point).
Both of these options look much better than
include/linux/mm.h:
#if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
!defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
static inline int __early_pfn_to_nid(unsigned long pfn)
{
return 0;
}
#else
/* please see mm/page_alloc.c */
extern int __meminit early_pfn_to_nid(unsigned long pfn);
#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
/* there is a per-arch backend function. */
extern int __meminit __early_pfn_to_nid(unsigned long pfn);
#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
#endif
where all this confusion is originating from.
It's obviously up to your taste in how to proceed, but the latter looks
sloppy to me and is the reason we have so many unreferenced prototypes in
header files.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, 7 Feb 2014 13:15:29 -0800 (PST) David Rientjes wrote: > On Fri, 7 Feb 2014, Josh Triplett wrote: > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > > index 1cedd00..5f8348f 100644 > > > > --- a/include/linux/mm.h > > > > +++ b/include/linux/mm.h > > > > @@ -1589,10 +1589,8 @@ static inline int __early_pfn_to_nid(unsigned > > > > long pfn) > > > > #else > > > > /* please see mm/page_alloc.c */ > > > > extern int __meminit early_pfn_to_nid(unsigned long pfn); > > > > -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID > > > > /* there is a per-arch backend function. */ > > > > extern int __meminit __early_pfn_to_nid(unsigned long pfn); > > > > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */ > > > > #endif > > > > > > > > extern void set_dma_reserve(unsigned long new_dma_reserve); > > > > > > Wouldn't it be better to just declare the __early_pfn_to_nid() in > > > mm/page_alloc.c to be static? > > > > Won't that break the ability to override that function in > > architecture-specific code (as arch/ia64/mm/numa.c does)? > > > > Why? CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID should define where this function > is defined so ia64 should have it set and the definition which I'm > suggesting be static is only compiled when this is undefined in > mm/page_alloc.c. I'm not sure why we'd want to be messing with the > declaration? __early_pfn_to_nid() must be global if it is implemented in arch/. Making it static when it is implemented in core mm makes a bit of sense, in that it cleans up the non-ia64 namespace and discourages usage from other compilation units. But it's is a bit odd and unexpected to do such a thing. I'm inclined to happily nuke the ifdef then go think about something else ;) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, 7 Feb 2014, Josh Triplett wrote: > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index 1cedd00..5f8348f 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -1589,10 +1589,8 @@ static inline int __early_pfn_to_nid(unsigned long > > > pfn) > > > #else > > > /* please see mm/page_alloc.c */ > > > extern int __meminit early_pfn_to_nid(unsigned long pfn); > > > -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID > > > /* there is a per-arch backend function. */ > > > extern int __meminit __early_pfn_to_nid(unsigned long pfn); > > > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */ > > > #endif > > > > > > extern void set_dma_reserve(unsigned long new_dma_reserve); > > > > Wouldn't it be better to just declare the __early_pfn_to_nid() in > > mm/page_alloc.c to be static? > > Won't that break the ability to override that function in > architecture-specific code (as arch/ia64/mm/numa.c does)? > Why? CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID should define where this function is defined so ia64 should have it set and the definition which I'm suggesting be static is only compiled when this is undefined in mm/page_alloc.c. I'm not sure why we'd want to be messing with the declaration? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, Feb 07, 2014 at 01:04:47PM -0800, David Rientjes wrote: > On Fri, 7 Feb 2014, Rashika Kheria wrote: > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 1cedd00..5f8348f 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1589,10 +1589,8 @@ static inline int __early_pfn_to_nid(unsigned long > > pfn) > > #else > > /* please see mm/page_alloc.c */ > > extern int __meminit early_pfn_to_nid(unsigned long pfn); > > -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID > > /* there is a per-arch backend function. */ > > extern int __meminit __early_pfn_to_nid(unsigned long pfn); > > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */ > > #endif > > > > extern void set_dma_reserve(unsigned long new_dma_reserve); > > Wouldn't it be better to just declare the __early_pfn_to_nid() in > mm/page_alloc.c to be static? Won't that break the ability to override that function in architecture-specific code (as arch/ia64/mm/numa.c does)? - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On Fri, 7 Feb 2014, Rashika Kheria wrote: > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 1cedd00..5f8348f 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1589,10 +1589,8 @@ static inline int __early_pfn_to_nid(unsigned long pfn) > #else > /* please see mm/page_alloc.c */ > extern int __meminit early_pfn_to_nid(unsigned long pfn); > -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID > /* there is a per-arch backend function. */ > extern int __meminit __early_pfn_to_nid(unsigned long pfn); > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */ > #endif > > extern void set_dma_reserve(unsigned long new_dma_reserve); Wouldn't it be better to just declare the __early_pfn_to_nid() in mm/page_alloc.c to be static? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
On 02/07/2014 07:15 AM, Rashika Kheria wrote: > The ifdef conditions in include/linux/mm.h presents three cases: > > - !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && > !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) > There is no actual definition of function but include/linux/mm.h has a > static inline stub defined. > > - defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && > !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) > linux/mm.h does not define a prototype, but mm/page_alloc.c defines > the function. > Hence, compiler reports the following warning: > mm/page_alloc.c:4300:15: warning: no previous prototype for > ‘__early_pfn_to_nid’ [-Wmissing-prototypes] > > - defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) > The architecture defines the function, and linux/mm.h has a prototype. > > Thus, join the conditions of Case 2 and 3 i.e. eliminate the ifdef > condition of CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID to eliminate the > missing prototype warning from file mm/page_alloc.c. > > Signed-off-by: Rashika Kheria > Reviewed-by: Josh Triplett Reviewed-by: Rik van Riel -- All rights reversed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h
The ifdef conditions in include/linux/mm.h presents three cases: - !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) There is no actual definition of function but include/linux/mm.h has a static inline stub defined. - defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) linux/mm.h does not define a prototype, but mm/page_alloc.c defines the function. Hence, compiler reports the following warning: mm/page_alloc.c:4300:15: warning: no previous prototype for ‘__early_pfn_to_nid’ [-Wmissing-prototypes] - defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) The architecture defines the function, and linux/mm.h has a prototype. Thus, join the conditions of Case 2 and 3 i.e. eliminate the ifdef condition of CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID to eliminate the missing prototype warning from file mm/page_alloc.c. Signed-off-by: Rashika Kheria Reviewed-by: Josh Triplett --- include/linux/mm.h |2 -- 1 file changed, 2 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index 1cedd00..5f8348f 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1589,10 +1589,8 @@ static inline int __early_pfn_to_nid(unsigned long pfn) #else /* please see mm/page_alloc.c */ extern int __meminit early_pfn_to_nid(unsigned long pfn); -#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID /* there is a per-arch backend function. */ extern int __meminit __early_pfn_to_nid(unsigned long pfn); -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */ #endif extern void set_dma_reserve(unsigned long new_dma_reserve); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
