more a philosophy question than anything but, while poking around
the percpu stuff today, i noticed in the header file linux/percpu.h
the opening snippet:
#include <linux/preempt.h>
#include <linux/slab.h> /* For kmalloc() */
#include <linux/smp.h>
#include <linux/string.h> /* For memset() */
#include <linux/cpumask.h>
...
hmmm, i thought to myself (because that's how i refer to myself), i
wonder why this header file is including headers for kmalloc() and
memset() when this header file makes no reference to those routines.
let's see what happens if i remove them and:
$ make distclean
$ make defconfig [x86]
$ make
... chug chug chug ...
CC arch/x86/kernel/nmi_32.o
arch/x86/kernel/nmi_32.c: In function ?check_nmi_watchdog?:
arch/x86/kernel/nmi_32.c:81: error: implicit declaration of function ?kmalloc?
arch/x86/kernel/nmi_32.c:81: error: ?GFP_KERNEL? undeclared (first use in this function)
arch/x86/kernel/nmi_32.c:81: error: (Each undeclared identifier is reported only once
arch/x86/kernel/nmi_32.c:81: error: for each function it appears in.)
arch/x86/kernel/nmi_32.c:81: warning: assignment makes pointer from integer without a cast
arch/x86/kernel/nmi_32.c:118: error: implicit declaration of function ?kfree?
make[1]: *** [arch/x86/kernel/nmi_32.o] Error 1
make: *** [arch/x86/kernel] Error 2
$
ok, now i know. but that means, of course, that nmi_32.c is
invoking kmalloc() without ever having included the necessary header
file for it -- it's just inheriting that from linux/percpu.h.
doesn't that (sort of) violate the kernel coding style? if a file
somewhere needs the contents of some header file, isn't it that file's
responsibility to explicitly include it, and not quietly realize it's
getting it from elsewhere?
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
On Thu, 13 Mar 2008 18:09:12 -0400 (EDT) Robert P. J. Day wrote:
>
> more a philosophy question than anything but, while poking around
> the percpu stuff today, i noticed in the header file linux/percpu.h
> the opening snippet:
>
> #include <linux/preempt.h>
> #include <linux/slab.h> /* For kmalloc() */
> #include <linux/smp.h>
> #include <linux/string.h> /* For memset() */
> #include <linux/cpumask.h>
> ...
>
> hmmm, i thought to myself (because that's how i refer to myself), i
> wonder why this header file is including headers for kmalloc() and
> memset() when this header file makes no reference to those routines.
> let's see what happens if i remove them and:
>
> $ make distclean
> $ make defconfig [x86]
> $ make
>
> ... chug chug chug ...
>
> CC arch/x86/kernel/nmi_32.o
> arch/x86/kernel/nmi_32.c: In function ‘check_nmi_watchdog’:
> arch/x86/kernel/nmi_32.c:81: error: implicit declaration of function ‘kmalloc’
> arch/x86/kernel/nmi_32.c:81: error: ‘GFP_KERNEL’ undeclared (first use in this function)
> arch/x86/kernel/nmi_32.c:81: error: (Each undeclared identifier is reported only once
> arch/x86/kernel/nmi_32.c:81: error: for each function it appears in.)
> arch/x86/kernel/nmi_32.c:81: warning: assignment makes pointer from integer without a cast
> arch/x86/kernel/nmi_32.c:118: error: implicit declaration of function ‘kfree’
> make[1]: *** [arch/x86/kernel/nmi_32.o] Error 1
> make: *** [arch/x86/kernel] Error 2
> $
>
> ok, now i know. but that means, of course, that nmi_32.c is
> invoking kmalloc() without ever having included the necessary header
> file for it -- it's just inheriting that from linux/percpu.h.
>
> doesn't that (sort of) violate the kernel coding style? if a file
> somewhere needs the contents of some header file, isn't it that file's
> responsibility to explicitly include it, and not quietly realize it's
> getting it from elsewhere?
Yes.
---
~Randy
On 13/03/2008, Robert P. J. Day <[email protected]> wrote:
>
> more a philosophy question than anything but, while poking around
> the percpu stuff today, i noticed in the header file linux/percpu.h
> the opening snippet:
>
> #include <linux/preempt.h>
> #include <linux/slab.h> /* For kmalloc() */
> #include <linux/smp.h>
> #include <linux/string.h> /* For memset() */
> #include <linux/cpumask.h>
> ...
>
> hmmm, i thought to myself (because that's how i refer to myself), i
> wonder why this header file is including headers for kmalloc() and
> memset() when this header file makes no reference to those routines.
> let's see what happens if i remove them and:
>
> $ make distclean
> $ make defconfig [x86]
> $ make
>
> ... chug chug chug ...
>
> CC arch/x86/kernel/nmi_32.o
> arch/x86/kernel/nmi_32.c: In function 'check_nmi_watchdog':
> arch/x86/kernel/nmi_32.c:81: error: implicit declaration of function 'kmalloc'
> arch/x86/kernel/nmi_32.c:81: error: 'GFP_KERNEL' undeclared (first use in this function)
> arch/x86/kernel/nmi_32.c:81: error: (Each undeclared identifier is reported only once
> arch/x86/kernel/nmi_32.c:81: error: for each function it appears in.)
> arch/x86/kernel/nmi_32.c:81: warning: assignment makes pointer from integer without a cast
> arch/x86/kernel/nmi_32.c:118: error: implicit declaration of function 'kfree'
> make[1]: *** [arch/x86/kernel/nmi_32.o] Error 1
> make: *** [arch/x86/kernel] Error 2
> $
>
> ok, now i know. but that means, of course, that nmi_32.c is
> invoking kmalloc() without ever having included the necessary header
> file for it -- it's just inheriting that from linux/percpu.h.
>
> doesn't that (sort of) violate the kernel coding style? if a file
> somewhere needs the contents of some header file, isn't it that file's
> responsibility to explicitly include it, and not quietly realize it's
> getting it from elsewhere?
>
I agree with you completely. A file should explicitly include headers
for the stuff it uses and not rely on implicit includes done
elsewhere. Cleaning that up is going to touch a lot of files though
for no real short term gain (there is a long term gain of
maintainability though), so it's going to be a loveless job :(
--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
On Fri, 14 Mar 2008, Jesper Juhl wrote:
> On 13/03/2008, Robert P. J. Day <[email protected]> wrote:
> >
> > more a philosophy question than anything but, while poking around
> > the percpu stuff today, i noticed in the header file linux/percpu.h
> > the opening snippet:
> >
> > #include <linux/preempt.h>
> > #include <linux/slab.h> /* For kmalloc() */
> > #include <linux/smp.h>
> > #include <linux/string.h> /* For memset() */
> > #include <linux/cpumask.h>
> > ...
> I agree with you completely. A file should explicitly include
> headers for the stuff it uses and not rely on implicit includes done
> elsewhere. Cleaning that up is going to touch a lot of files though
> for no real short term gain (there is a long term gain of
> maintainability though), so it's going to be a loveless job :(
i wasn't about to run off and start changing stuff, i was just curious
about the general philosophy. i *might* see the value of a patch if
it's a cleanup that affects a restricted set of files that are
logically related and can be done with a single patch. beyond that,
no.
the only reason the above example caught my eye is the insistence in
the comments as to why those includes were there, when there were no
invocations of those routines anywhere in the file. i always find
that curious.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
On Mar 14 2008 00:53, Jesper Juhl wrote:
>I agree with you completely. A file should explicitly include headers
>for the stuff it uses and not rely on implicit includes done
>elsewhere. Cleaning that up is going to touch a lot of files though
>for no real short term gain (there is a long term gain of
>maintainability though), so it's going to be a loveless job :(
But straightforward. You nuke the complete #include list of a .h/.c
file and "rebuild" it by hand, by looking at the code the .h/.c file
provides/uses and selecting appropriate #includes.
On Fri, 14 Mar 2008, Jan Engelhardt wrote:
>
> On Mar 14 2008 00:53, Jesper Juhl wrote:
> >I agree with you completely. A file should explicitly include
> >headers for the stuff it uses and not rely on implicit includes
> >done elsewhere. Cleaning that up is going to touch a lot of files
> >though for no real short term gain (there is a long term gain of
> >maintainability though), so it's going to be a loveless job :(
>
> But straightforward. You nuke the complete #include list of a .h/.c
> file and "rebuild" it by hand, by looking at the code the .h/.c file
> provides/uses and selecting appropriate #includes.
well, yes, that would certainly do it, but i'm not that ambitious.
:-) actually, what i'm testing now is deleting the two superfluous
includes from <linux/percpu.h>:
#include <linus/slab.h>
#include <linux/string.h>
doing "make allyesconfig" on x86, watching where the compile fails,
fixing that file, and noticing that errors fall into a fairly small
set of localized clumps, so i'll just collect them and submit them.
there's actually not that many. patches to follow shortly.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
On Thu, Mar 13, 2008 at 09:53:01PM -0400, Robert P. J. Day wrote:
> On Fri, 14 Mar 2008, Jan Engelhardt wrote:
>
> >
> > On Mar 14 2008 00:53, Jesper Juhl wrote:
> > >I agree with you completely. A file should explicitly include
> > >headers for the stuff it uses and not rely on implicit includes
> > >done elsewhere. Cleaning that up is going to touch a lot of files
> > >though for no real short term gain (there is a long term gain of
> > >maintainability though), so it's going to be a loveless job :(
> >
> > But straightforward. You nuke the complete #include list of a .h/.c
> > file and "rebuild" it by hand, by looking at the code the .h/.c file
> > provides/uses and selecting appropriate #includes.
Which is generally wrong thing to do because you won't notice that
moving parts to another header will make list of necessary headers much
shorter.
> well, yes, that would certainly do it, but i'm not that ambitious.
> :-) actually, what i'm testing now is deleting the two superfluous
> includes from <linux/percpu.h>:
>
> #include <linus/slab.h>
This needed for kzalloc().
> #include <linux/string.h>
That looks unnecessary, yes.
> doing "make allyesconfig" on x86, watching where the compile fails,
> fixing that file, and noticing that errors fall into a fairly small
> set of localized clumps, so i'll just collect them and submit them.
> there's actually not that many.
Sir, why on earth do you breed so many patches?
If you're going to delete string.h from percpu.h (which is good idea)
you a) delete it, and b) fix all compile breakages, and c) submit as one
patch.
By "all compile" breakages I mean cross-compilation on archs other than
i386 and on many configs on each of them. If you can't do that, sorry,
stay far away from headers, especially core.
As for many preparatory patches, a) by itself they're useless -- A is
still including B and headers are protected against double inclusion,
b) late in game you can discover that, oops, B is needed in A and all
those patches were useless, c) submitting as one patch which removes B
from A means other people can compile-test that you haven't broken their
usual config, with many small patches scattered over mailing lists they
won't be able do that. Remember, A is still including B.
I suggest to add headers when there is compilation breakage and remove
headers when it's safe thing to do.
P.S.: Some common sense is also required when touching header
dependencies. gfp.h is not included left and right by drivers,
because it comes from slab.h. Some common sense is also required
when touching header dependencies.
Jesper Juhl schrieb:
> On 13/03/2008, Robert P. J. Day <[email protected]> wrote:
>> more a philosophy question than anything but, while poking around
>> the percpu stuff today, i noticed in the header file linux/percpu.h
>> the opening snippet:
>>
>> #include <linux/preempt.h>
>> #include <linux/slab.h> /* For kmalloc() */
>> #include <linux/smp.h>
>> #include <linux/string.h> /* For memset() */
>> #include <linux/cpumask.h>
>> ...
>>
>> hmmm, i thought to myself (because that's how i refer to myself), i
>> wonder why this header file is including headers for kmalloc() and
>> memset() when this header file makes no reference to those routines.
>> let's see what happens if i remove them and:
>>
>> $ make distclean
>> $ make defconfig [x86]
>> $ make
>>
>> ... chug chug chug ...
>>
>> CC arch/x86/kernel/nmi_32.o
>> arch/x86/kernel/nmi_32.c: In function 'check_nmi_watchdog':
>> arch/x86/kernel/nmi_32.c:81: error: implicit declaration of function 'kmalloc'
>> arch/x86/kernel/nmi_32.c:81: error: 'GFP_KERNEL' undeclared (first use in this function)
>> arch/x86/kernel/nmi_32.c:81: error: (Each undeclared identifier is reported only once
>> arch/x86/kernel/nmi_32.c:81: error: for each function it appears in.)
>> arch/x86/kernel/nmi_32.c:81: warning: assignment makes pointer from integer without a cast
>> arch/x86/kernel/nmi_32.c:118: error: implicit declaration of function 'kfree'
>> make[1]: *** [arch/x86/kernel/nmi_32.o] Error 1
>> make: *** [arch/x86/kernel] Error 2
>> $
>>
>> ok, now i know. but that means, of course, that nmi_32.c is
>> invoking kmalloc() without ever having included the necessary header
>> file for it -- it's just inheriting that from linux/percpu.h.
>>
>> doesn't that (sort of) violate the kernel coding style? if a file
>> somewhere needs the contents of some header file, isn't it that file's
>> responsibility to explicitly include it, and not quietly realize it's
>> getting it from elsewhere?
>>
> I agree with you completely. A file should explicitly include headers
> for the stuff it uses and not rely on implicit includes done
> elsewhere. Cleaning that up is going to touch a lot of files though
> for no real short term gain (there is a long term gain of
> maintainability though), so it's going to be a loveless job :(
>
i do not. the question we must discuss is imho not the situation as is, but how
a header cleanup is done if functions are reworked or moved.
you will _never_ get a clear include situation, even if you remove all of the includes
and try to compile the kernel by adding neccessary header includes back.
when you include the first headers again, you will re-generate the current situation,
because you'll never notice through compile test if there is again an implicit include
due to hierachical reasons.
preventing this could only be done if you really do analyse each source/header file to
check what is needed. this will cause much more time for the preprocessor of your compiler,
thus there is no really advantage.
what i said in my first sentence is the imho proper way:
if you alter sources and includes get obsolete, remove them and fix compiling again for
files that formerly depended (implicit or explicit) on this altered sources.
not to much work for the developer himself and also not exhausting the compile times.
regards
marcel
On Fri, 14 Mar 2008, Alexey Dobriyan wrote:
> On Thu, Mar 13, 2008 at 09:53:01PM -0400, Robert P. J. Day wrote:
> > includes from <linux/percpu.h>:
> >
> > #include <linux/slab.h>
>
> This needed for kzalloc().
oh, &%^^%*(&^), you're right. i looked at the comment, where it said
it needed that include for "kmalloc", grepped, didn't see kmalloc so
thought that include was unnecessary. i didn't even notice the
"kzalloc". my bad, profuse apologies. argh.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
On Fri, 14 Mar 2008, [email protected] wrote:
> what i said in my first sentence is the imho proper way: if you
> alter sources and includes get obsolete, remove them and fix
> compiling again for files that formerly depended (implicit or
> explicit) on this altered sources. not to much work for the
> developer himself and also not exhausting the compile times.
and, in my defense, that's what i *thought* i was doing, not having
noticed that one of those "unnecessary" #includes really was
required after all. argh. that was embarrassing.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
On Fri, 14 Mar 2008, Robert P. J. Day wrote:
> On Fri, 14 Mar 2008, [email protected] wrote:
>
> > what i said in my first sentence is the imho proper way: if you
> > alter sources and includes get obsolete, remove them and fix
> > compiling again for files that formerly depended (implicit or
> > explicit) on this altered sources. not to much work for the
> > developer himself and also not exhausting the compile times.
>
> and, in my defense, that's what i *thought* i was doing, not having
> noticed that one of those "unnecessary" #includes really was
> required after all. argh. that was embarrassing.
sure, let's beat this to death.
upon reflection, i realize i was trying to deal with two different
issues. on the one hand, there are the (countless?) header files that
don't include header files they normally should. for instance,
imagine a file fubar.c that contains:
...
#include <linux/percpu.h>
...
... some reference to GFP_KERNEL ...
...
but doesn't include <linux/gfp.h>, which it should 'cuz it's referring
to GFP_KERNEL. the above will still work since percpu.h includes
slab.h, which includes gfp.h, so it all works out in the end. and
there's simply too much of that going on to do anything about.
on the other hand, something that *is* worth fixing is what's in
percpu.h right now:
...
#include <linux/string.h> /* For memset() */
...
since it's clear(?) that percpu.h has no need for string.h, it does
make sense to remove that include and see if that causes the build to
break because others have quietly been taking advantage of that
include.
IMHO, then, that second case is worth trying to fix when it comes
up. the first really isn't, but i had the two confused and thought i
had actually come across the second example. again, my stupidity.
rday
p.s. just for fun, i did remove that include of string.h from
percpu.h and i'm doing an allyesconfig x86 build off on the side just
to see the result. if it still builds, then that *would* be worth a
quick patch, just to stop including stuff you don't need. and there's
going to be *lot* less of that, i would think.
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================