diff mbox

[RFC,v3,07/13] tables.h: add linker table support

Message ID 20160812170451.GE3296@wotan.suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Luis Chamberlain Aug. 12, 2016, 5:04 p.m. UTC
On Fri, Aug 12, 2016 at 05:51:21PM +0200, Borislav Petkov wrote:
> On Fri, Aug 12, 2016 at 05:28:05PM +0200, Luis R. Rodriguez wrote:
> > Even so, you don't link the compiled extra code so the only penalty
> > here is when compiling, nothing more. And if you are compiling typically
> > the cost here is just a few seconds.
> 
> Yeah, so let's make it clear that this is similar to COMPILE_TEST and
> people with fast machines and who don't mind building a couple more
> seconds longer, should enable it.

Sure, I'll also move it under COMPILE_TEST, and default it to n.

> You don't want to be doing bit-rotting tests on small, weak machines,
> which barely get done with the build as it is. I have a 32-bit atom
> which takes hours to build a kernel. Enabling that there is not making
> the kernel build any more fun.
> 
> > > ... or you simply don't want to have stuff which is forcibly enabled on you even
> > > if you're never going to need it
> > > ...
> > 
> > Which seems to be the same as the reason I noted ?
> 
> No, the reason is we don't force stuff down people's throats just
> because we think we know better. Other people do that.
> 
> Instead, we help them make an informed decision by describing the
> feature as precisely as possible.
> 
> > I can remove the grumpy maintainer description :)
> 
> Yap :-)

Alright, how's this new description:

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Borislav Petkov Aug. 12, 2016, 5:35 p.m. UTC | #1
On Fri, Aug 12, 2016 at 07:04:52PM +0200, Luis R. Rodriguez wrote:
> Alright, how's this new description:
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index cac3f096050d..73e4890c24c4 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -53,6 +53,34 @@ config CROSS_COMPILE
>  	  need to set this unless you want the configured kernel build
>  	  directory to select the cross-compiler automatically.
>  
> +config BUILD_AVOID_BITROT
> +	bool "Always force building specially annotated targets"
> +	default n
> +	help
> +	  If enabled then the the special table-* Makefile targets will always
> +	  be forced to be compiled even if their respective CONFIG_ option has

"will always be compiled" is already absolute.

> +	  been disabled, but its objects will only be linked in if the same
> +	  respective CONFIG_ option has been enabled. This helps avoid code
> +	  bit rot issues, use for these targets should be carefully considred

s/This helps avoid code bit rot issues, u/U/

The bit-rot thing comes again below.

> +	  by maintainers. You can safely enable this option at the expense of
> +	  increasing compile time. Enabling this option helps avoid code bit
> +	  rot by taking advantage of the facilities provided and enabled by
> +	  using linker tables documented under:
> +
> +	  include/linux/tables.h
> +
> +	  The special targets supported are:
> +
> +	    o table-obj-y
> +	    o table-lib-y
> +
> +	  Say Y if you have a decent build machine and would like to help test
> +	  building code for more subsystems. Say N if you do you not have a
> +	  good build machine or only want to compile what you've enabled for
> +	  your kernel.
> +
> +	  Enabling this option never increases the size of your kernel.
> +

Other than those minor formulation nits, yeah, nice!

Thanks.
Greg Kroah-Hartman Aug. 12, 2016, 8:23 p.m. UTC | #2
On Fri, Aug 12, 2016 at 07:04:52PM +0200, Luis R. Rodriguez wrote:
> Alright, how's this new description:
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index cac3f096050d..73e4890c24c4 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -53,6 +53,34 @@ config CROSS_COMPILE
>  	  need to set this unless you want the configured kernel build
>  	  directory to select the cross-compiler automatically.
>  
> +config BUILD_AVOID_BITROT
> +	bool "Always force building specially annotated targets"
> +	default n
> +	help
> +	  If enabled then the the special table-* Makefile targets will always
> +	  be forced to be compiled even if their respective CONFIG_ option has
> +	  been disabled, but its objects will only be linked in if the same
> +	  respective CONFIG_ option has been enabled. This helps avoid code
> +	  bit rot issues, use for these targets should be carefully considred
> +	  by maintainers. You can safely enable this option at the expense of
> +	  increasing compile time. Enabling this option helps avoid code bit
> +	  rot by taking advantage of the facilities provided and enabled by
> +	  using linker tables documented under:

As a kernel developer I have _no_ idea what this is trying to say at
all, sorry.

What is a "specially annotated target"?  Who is forcing it to be built?
What does it mean if it isn't built?

> +
> +	  include/linux/tables.h
> +
> +	  The special targets supported are:
> +
> +	    o table-obj-y
> +	    o table-lib-y

What does this mean to me as a developer?  What does it mean to a user
who wants to figure out if it should be enabled or not?

> +
> +	  Say Y if you have a decent build machine and would like to help test
> +	  building code for more subsystems. Say N if you do you not have a
> +	  good build machine or only want to compile what you've enabled for
> +	  your kernel.

How does this test different subsystems?  How does disabling it not test
them?  Why would I care either way?

> +
> +	  Enabling this option never increases the size of your kernel.

Then what does it do?  Just burn electricity for no reason?

totally confused...

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina Aug. 12, 2016, 8:46 p.m. UTC | #3
On Fri, 12 Aug 2016, Greg KH wrote:

> > +	  Enabling this option never increases the size of your kernel.
> 
> Then what does it do?  Just burn electricity for no reason?

I think that this whole thing could be without loss of generality 
reformulated in a "allows for participating in compile-testing code 
efforts, without actually having to have all the compile-tested code 
present in the resulting kernel".
Luis Chamberlain Aug. 12, 2016, 10 p.m. UTC | #4
On Fri, Aug 12, 2016 at 10:23:34PM +0200, Greg KH wrote:
> On Fri, Aug 12, 2016 at 07:04:52PM +0200, Luis R. Rodriguez wrote:
> > Alright, how's this new description:
> > 
> > diff --git a/init/Kconfig b/init/Kconfig
> > index cac3f096050d..73e4890c24c4 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -53,6 +53,34 @@ config CROSS_COMPILE
> >  	  need to set this unless you want the configured kernel build
> >  	  directory to select the cross-compiler automatically.
> >  
> > +config BUILD_AVOID_BITROT
> > +	bool "Always force building specially annotated targets"
> > +	default n
> > +	help
> > +	  If enabled then the the special table-* Makefile targets will always
> > +	  be forced to be compiled even if their respective CONFIG_ option has
> > +	  been disabled, but its objects will only be linked in if the same
> > +	  respective CONFIG_ option has been enabled. This helps avoid code
> > +	  bit rot issues, use for these targets should be carefully considred
> > +	  by maintainers. You can safely enable this option at the expense of
> > +	  increasing compile time. Enabling this option helps avoid code bit
> > +	  rot by taking advantage of the facilities provided and enabled by
> > +	  using linker tables documented under:
> 
> As a kernel developer I have _no_ idea what this is trying to say at
> all, sorry.

Hmm, wow OK, sorry, and I thought I was being too verbose...

OK so first, linker tables allow for the ability to help simplify initialization
sequences so that you no longer have to add the respective static inline in
header files to do nothing, instead you simply get your init routine for your
feature pegged into the linker table or not at link time. If enabling your
feature does not require structural changes, you could then safely enable
compiling this feature all the time, and only allow linking when the feature
was enabled. We don't have an easy way to express this in our build system,
the new targets added lets you accomplish this.

> What is a "specially annotated target"?

The ones listed below, table-obj-y and table-lib-y

> Who is forcing it to be built?

It would be up to maintainers for each subsystem/feature to decide if they want
to use the new targets or not within their subsystem.

> What does it mean if it isn't built?

If you have CONFIG_BUILD_AVOID_BITROT enabled and some code using the special
targets do not get built it means the dependencies it has were not met.

> > +
> > +	  include/linux/tables.h
> > +
> > +	  The special targets supported are:
> > +
> > +	    o table-obj-y
> > +	    o table-lib-y
> 
> What does this mean to me as a developer?

It mean you can count on a bit more build test coverage by
CONFIG_BUILD_AVOID_BITROT users. Using table-obj-y is functionally
equivalent to doing:

extra-y += foo.o
obj-y += foo.o

The above new targets are just short hand annotations for the same.  We could
actually use another shorthand prefix other than table-, however linker tables
help making more of these type of targets possible. For instance, on initialiation
sequences you no longer have to add each line for each feature onto a set routine,
rather you just get the initialization routine linked in or not. This lets us avoid
cluttering C code and header code with #idefs, and as a side consequences also
allows more targets to be compiled without implicating functionality.

As a developer you should take care to to use table-obj-y, or table-lib-y only if
you are certain the target does not require structural changes.

> What does it mean to a user
> who wants to figure out if it should be enabled or not?

It depends on their build system capability and their goals. If they wish
to be able to report build bugs and have a decent build system they can
enable this. Otherwise they should disable it.

> > +	  Say Y if you have a decent build machine and would like to help test
> > +	  building code for more subsystems. Say N if you do you not have a
> > +	  good build machine or only want to compile what you've enabled for
> > +	  your kernel.
> 
> How does this test different subsystems?

By enabling this feature you compile kconfig symbols that typically are
disabled by most users and which have been identified by maintainers as
needing more build testing love. The extra kconfig symbols built are
only built if the dependencies for them are met.

Maintainers for subsystems would have to identify if they have key pieces of
software that typically get disabled, and that enabling them would not incur or
require structural changes, which can use more build test love.

> How does disabling it not test them?

By disabling this feature you only compile kconfig symbols you have
enabled for your kernel.

> Why would I care either way?

You would care if you aware of certain kconfig symbols that do not
get much build test love.

> > +
> > +	  Enabling this option never increases the size of your kernel.
> 
> Then what does it do?  Just burn electricity for no reason?

It enables maintainers to annotate through the build system certain
kconfig symbol which should be built if CONFIG_BUILD_AVOID_BITROT is
enabled and its symbol dependencies are met.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman Aug. 13, 2016, 10:46 a.m. UTC | #5
On Sat, Aug 13, 2016 at 12:00:42AM +0200, Luis R. Rodriguez wrote:
> On Fri, Aug 12, 2016 at 10:23:34PM +0200, Greg KH wrote:
> > On Fri, Aug 12, 2016 at 07:04:52PM +0200, Luis R. Rodriguez wrote:
> > > Alright, how's this new description:
> > > 
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index cac3f096050d..73e4890c24c4 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -53,6 +53,34 @@ config CROSS_COMPILE
> > >  	  need to set this unless you want the configured kernel build
> > >  	  directory to select the cross-compiler automatically.
> > >  
> > > +config BUILD_AVOID_BITROT
> > > +	bool "Always force building specially annotated targets"
> > > +	default n
> > > +	help
> > > +	  If enabled then the the special table-* Makefile targets will always
> > > +	  be forced to be compiled even if their respective CONFIG_ option has
> > > +	  been disabled, but its objects will only be linked in if the same
> > > +	  respective CONFIG_ option has been enabled. This helps avoid code
> > > +	  bit rot issues, use for these targets should be carefully considred
> > > +	  by maintainers. You can safely enable this option at the expense of
> > > +	  increasing compile time. Enabling this option helps avoid code bit
> > > +	  rot by taking advantage of the facilities provided and enabled by
> > > +	  using linker tables documented under:
> > 
> > As a kernel developer I have _no_ idea what this is trying to say at
> > all, sorry.
> 
> Hmm, wow OK, sorry, and I thought I was being too verbose...

All of your good explainations and detail need to go into the text
itself.  Responding to my questions is wonderful, but is not going to
help anyone out who sees the config option for the first time, unless
you change the text there.

> OK so first, linker tables allow for the ability to help simplify initialization
> sequences so that you no longer have to add the respective static inline in
> header files to do nothing, instead you simply get your init routine for your
> feature pegged into the linker table or not at link time. If enabling your
> feature does not require structural changes, you could then safely enable
> compiling this feature all the time, and only allow linking when the feature
> was enabled. We don't have an easy way to express this in our build system,
> the new targets added lets you accomplish this.

I still don't understand, sorry.

As a kernel developer, why do I care about this?  Who should care about
this, a driver developer, subsystem developer, someone adding a syscall,
an arch subsystem developer?

> > What is a "specially annotated target"?
> 
> The ones listed below, table-obj-y and table-lib-y

circular logic doesn't work :)

> > Who is forcing it to be built?
> 
> It would be up to maintainers for each subsystem/feature to decide if they want
> to use the new targets or not within their subsystem.

Ok, as a maintainer of a subsystem, I still have no idea what any of
this means.  But maybe I'm just don't understand kernel subsystem
development...

> > What does it mean if it isn't built?
> 
> If you have CONFIG_BUILD_AVOID_BITROT enabled and some code using the special
> targets do not get built it means the dependencies it has were not met.

What "dependencies"?

> > > +
> > > +	  include/linux/tables.h
> > > +
> > > +	  The special targets supported are:
> > > +
> > > +	    o table-obj-y
> > > +	    o table-lib-y
> > 
> > What does this mean to me as a developer?
> 
> It mean you can count on a bit more build test coverage by
> CONFIG_BUILD_AVOID_BITROT users. Using table-obj-y is functionally
> equivalent to doing:
> 
> extra-y += foo.o
> obj-y += foo.o

Ok, but why change this?  What's the advantage?  Are you going to now go
and change all locations of the above in the tree to the table-
versions?  If not, why is this needed?

> The above new targets are just short hand annotations for the same.  We could
> actually use another shorthand prefix other than table-, however linker tables
> help making more of these type of targets possible. For instance, on initialiation
> sequences you no longer have to add each line for each feature onto a set routine,
> rather you just get the initialization routine linked in or not. This lets us avoid
> cluttering C code and header code with #idefs, and as a side consequences also
> allows more targets to be compiled without implicating functionality.
> 
> As a developer you should take care to to use table-obj-y, or table-lib-y only if
> you are certain the target does not require structural changes.

How am I supposed to be "certain"?

> > What does it mean to a user
> > who wants to figure out if it should be enabled or not?
> 
> It depends on their build system capability and their goals. If they wish
> to be able to report build bugs and have a decent build system they can
> enable this. Otherwise they should disable it.

Why is this differnent from CONFIG_TEST build option we have today (or
whatever it's called...)

Anyway, again, I have no idea what this is, nor how I should use it.
And if I am the audience for this type of config option, well, perhaps
it needs to be rethought...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain Aug. 13, 2016, 5:54 p.m. UTC | #6
On Sat, Aug 13, 2016 at 12:46:16PM +0200, Greg KH wrote:
> On Sat, Aug 13, 2016 at 12:00:42AM +0200, Luis R. Rodriguez wrote:
> > On Fri, Aug 12, 2016 at 10:23:34PM +0200, Greg KH wrote:
> > > On Fri, Aug 12, 2016 at 07:04:52PM +0200, Luis R. Rodriguez wrote:
> > > > Alright, how's this new description:
> > > > 
> > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > index cac3f096050d..73e4890c24c4 100644
> > > > --- a/init/Kconfig
> > > > +++ b/init/Kconfig
> > > > @@ -53,6 +53,34 @@ config CROSS_COMPILE
> > > >  	  need to set this unless you want the configured kernel build
> > > >  	  directory to select the cross-compiler automatically.
> > > >  
> > > > +config BUILD_AVOID_BITROT
> > > > +	bool "Always force building specially annotated targets"
> > > > +	default n
> > > > +	help
> > > > +	  If enabled then the the special table-* Makefile targets will always
> > > > +	  be forced to be compiled even if their respective CONFIG_ option has
> > > > +	  been disabled, but its objects will only be linked in if the same
> > > > +	  respective CONFIG_ option has been enabled. This helps avoid code
> > > > +	  bit rot issues, use for these targets should be carefully considred
> > > > +	  by maintainers. You can safely enable this option at the expense of
> > > > +	  increasing compile time. Enabling this option helps avoid code bit
> > > > +	  rot by taking advantage of the facilities provided and enabled by
> > > > +	  using linker tables documented under:
> > > 
> > > As a kernel developer I have _no_ idea what this is trying to say at
> > > all, sorry.
> > 
> > Hmm, wow OK, sorry, and I thought I was being too verbose...
> 
> All of your good explainations and detail need to go into the text
> itself.  Responding to my questions is wonderful, but is not going to
> help anyone out who sees the config option for the first time, unless
> you change the text there.

Sure.

> > OK so first, linker tables allow for the ability to help simplify initialization
> > sequences so that you no longer have to add the respective static inline in
> > header files to do nothing, instead you simply get your init routine for your
> > feature pegged into the linker table or not at link time. If enabling your
> > feature does not require structural changes, you could then safely enable
> > compiling this feature all the time, and only allow linking when the feature
> > was enabled. We don't have an easy way to express this in our build system,
> > the new targets added lets you accomplish this.
> 
> I still don't understand, sorry.
> 
> As a kernel developer, why do I care about this?  Who should care about
> this, a driver developer, subsystem developer, someone adding a syscall,
> an arch subsystem developer?

I'll try to explain this more.

> > > What is a "specially annotated target"?
> > 
> > The ones listed below, table-obj-y and table-lib-y
> 
> circular logic doesn't work :)
> 
> > > Who is forcing it to be built?
> > 
> > It would be up to maintainers for each subsystem/feature to decide if they want
> > to use the new targets or not within their subsystem.
> 
> Ok, as a maintainer of a subsystem, I still have no idea what any of
> this means.  But maybe I'm just don't understand kernel subsystem
> development...

Jeesh, OK, will give it one more go.

> > > What does it mean if it isn't built?
> > 
> > If you have CONFIG_BUILD_AVOID_BITROT enabled and some code using the special
> > targets do not get built it means the dependencies it has were not met.
> 
> What "dependencies"?

Will elaborate.

> > > > +
> > > > +	  include/linux/tables.h
> > > > +
> > > > +	  The special targets supported are:
> > > > +
> > > > +	    o table-obj-y
> > > > +	    o table-lib-y
> > > 
> > > What does this mean to me as a developer?
> > 
> > It mean you can count on a bit more build test coverage by
> > CONFIG_BUILD_AVOID_BITROT users. Using table-obj-y is functionally
> > equivalent to doing:
> > 
> > extra-y += foo.o
> > obj-y += foo.o
> 
> Ok, but why change this?  What's the advantage?  Are you going to now go
> and change all locations of the above in the tree to the table-
> versions?  If not, why is this needed?

Hell no, the gain here is clear annotation of when this practice is used.
Linker table use could make this practice more prevalent.

> > The above new targets are just short hand annotations for the same.  We could
> > actually use another shorthand prefix other than table-, however linker tables
> > help making more of these type of targets possible. For instance, on initialiation
> > sequences you no longer have to add each line for each feature onto a set routine,
> > rather you just get the initialization routine linked in or not. This lets us avoid
> > cluttering C code and header code with #idefs, and as a side consequences also
> > allows more targets to be compiled without implicating functionality.
> > 
> > As a developer you should take care to to use table-obj-y, or table-lib-y only if
> > you are certain the target does not require structural changes.
> 
> How am I supposed to be "certain"?

Compile testing is one easy way, but better yet understanding the code is
best. The structural change complication was one consideration that came
to mind.

> > > What does it mean to a user
> > > who wants to figure out if it should be enabled or not?
> > 
> > It depends on their build system capability and their goals. If they wish
> > to be able to report build bugs and have a decent build system they can
> > enable this. Otherwise they should disable it.
> 
> Why is this differnent from CONFIG_TEST build option we have today (or
> whatever it's called...)

We could fold it under that, sure.

> Anyway, again, I have no idea what this is, nor how I should use it.
> And if I am the audience for this type of config option, well, perhaps
> it needs to be rethought...

Alright, I'll rip all this code bit rot things out into its own separate
patch for Kbuild changes, documentation, etc, and see if this helps. Otherwise
we can evaluate if we should just drop it. Keep in mind that iPXE actually
used it for this purpose and enabled it tree wide, since I knew it would
likely not be a popular option for some I figured an optional build target
would be better. The rest is up to me to try to explain then how and why some
folks might want this. Linker tables just make it an easier practice to
use due to the simplication of code from #ifdef statements on Kconfig
symbols.

I'll address all this then separately in my next respin. It will be its own
separate patch.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/init/Kconfig b/init/Kconfig
index cac3f096050d..73e4890c24c4 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -53,6 +53,34 @@  config CROSS_COMPILE
 	  need to set this unless you want the configured kernel build
 	  directory to select the cross-compiler automatically.
 
+config BUILD_AVOID_BITROT
+	bool "Always force building specially annotated targets"
+	default n
+	help
+	  If enabled then the the special table-* Makefile targets will always
+	  be forced to be compiled even if their respective CONFIG_ option has
+	  been disabled, but its objects will only be linked in if the same
+	  respective CONFIG_ option has been enabled. This helps avoid code
+	  bit rot issues, use for these targets should be carefully considred
+	  by maintainers. You can safely enable this option at the expense of
+	  increasing compile time. Enabling this option helps avoid code bit
+	  rot by taking advantage of the facilities provided and enabled by
+	  using linker tables documented under:
+
+	  include/linux/tables.h
+
+	  The special targets supported are:
+
+	    o table-obj-y
+	    o table-lib-y
+
+	  Say Y if you have a decent build machine and would like to help test
+	  building code for more subsystems. Say N if you do you not have a
+	  good build machine or only want to compile what you've enabled for
+	  your kernel.
+
+	  Enabling this option never increases the size of your kernel.
+
 config COMPILE_TEST
 	bool "Compile also drivers which will not load"
 	depends on !UML