generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Tue, Jun 14, 2011 at 08:28:05AM +0200, Michal Nazarewicz wrote:
> On Mon, 13 Jun 2011 17:35:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >We also need to a find a better way to get rid of the inclusion of C
> >source files. I understand why Dave did it, but it's been quite some
> >time since those patches were committed - e.g.
> >7e75bc0f9006e995a0fa25f0a285addc3d5fd5cb - and no progress has been made
> >to fix that up. Probably Clang's link-time optimization would help a lot
> >on that, but Clang still needs quite some work to be usable for us, so
> >we need something else. Any ideas ?
> 
> To me it always seemed like it's a bug in/lack of feature of Kbuild.
> Would making Kbuild allow building module from separate .o files be
> a problem?

maybe the guys at linux-kbuild@vger can help answering this.

> Also, at one point in time I had an idea of a framework in which each
> individual composite function would be a separate module.  I haven't
> thought it through though so it's probably not such a great idea after
> all. ;)

such a framework won't work for Certification. The original composite
framework that I implemented with the guidance of Dave was doing exactly
that. Each function driver was a module of its own and you built
composite gadgets by loading the different drivers.

That was until Dave explained to me why it wouldn't fly. You can't have
completely dynamic USB peripherals. If you go to certification with
something like that, you will be denied certification as your device can
change how it appears to the bus at any time.

So, the way we have things now, is good. We just need to figure out a
way to avoid the inclusion of C files.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Arnaud Lacombe]

Hi,

On Tue, Jun 14, 2011 at 3:32 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Jun 14, 2011 at 08:28:05AM +0200, Michal Nazarewicz wrote:
>> On Mon, 13 Jun 2011 17:35:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>> >We also need to a find a better way to get rid of the inclusion of C
>> >source files. I understand why Dave did it, but it's been quite some
>> >time since those patches were committed - e.g.
>> >7e75bc0f9006e995a0fa25f0a285addc3d5fd5cb - and no progress has been made
>> >to fix that up. Probably Clang's link-time optimization would help a lot
>> >on that, but Clang still needs quite some work to be usable for us, so
>> >we need something else. Any ideas ?
>>
>> To me it always seemed like it's a bug in/lack of feature of Kbuild.
>> Would making Kbuild allow building module from separate .o files be
>> a problem?
>
> maybe the guys at linux-kbuild@vger can help answering this.
>
What is the problem exactly ?

The following patch:

diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 4fe92b1..cc432b8 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
 # USB gadget drivers
 #
 g_zero-y                       := zero.o
+g_zero-y                       += composite.o
+g_zero-y                       += usbstring.o
+g_zero-y                       += config.o
+g_zero-y                       += epautoconf.o
+g_zero-y                       += f_sourcesink.o
+g_zero-y                       += f_loopback.o
 g_audio-y                      := audio.o
 g_ether-y                      := ether.o
 g_serial-y                     := serial.o
diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
index 6d16db9..9e74d8f 100644
--- a/drivers/usb/gadget/zero.c
+++ b/drivers/usb/gadget/zero.c
@@ -60,23 +60,6 @@

 /*-------------------------------------------------------------------------*/

-/*
- * Kbuild is not very cooperative with respect to linking separately
- * compiled library objects into one module.  So for now we won't use
- * separate compilation ... ensuring init/exit sections work to shrink
- * the runtime footprint, and giving us at least some parts of what
- * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
- */
-#include "composite.c"
-#include "usbstring.c"
-#include "config.c"
-#include "epautoconf.c"
-
-#include "f_sourcesink.c"
-#include "f_loopback.c"
-
-/*-------------------------------------------------------------------------*/
-
 #define DRIVER_VERSION         "Cinco de Mayo 2008"

 static const char longname[] = "Gadget Zero";


build fine for me with CONFIG_USB_ZERO=[ym], beside an obvious visibility issue:

diff --git a/drivers/usb/gadget/f_loopback.c b/drivers/usb/gadget/f_loopback.c
index b37960f..2300a3d 100644
--- a/drivers/usb/gadget/f_loopback.c
+++ b/drivers/usb/gadget/f_loopback.c
@@ -372,8 +372,8 @@ int __init loopback_add(struct usb_composite_dev
*cdev, bool autoresume)
        loopback_driver.iConfiguration = id;

        /* support autoresume for remote wakeup testing */
-       if (autoresume)
-               sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+//     if (autoresume)
+//             sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;

        /* support OTG systems */
        if (gadget_is_otg(cdev->gadget)) {


Thanks,
 - Arnaud

>> Also, at one point in time I had an idea of a framework in which each
>> individual composite function would be a separate module.  I haven't
>> thought it through though so it's probably not such a great idea after
>> all. ;)
>
> such a framework won't work for Certification. The original composite
> framework that I implemented with the guidance of Dave was doing exactly
> that. Each function driver was a module of its own and you built
> composite gadgets by loading the different drivers.
>
> That was until Dave explained to me why it wouldn't fly. You can't have
> completely dynamic USB peripherals. If you go to certification with
> something like that, you will be denied certification as your device can
> change how it appears to the bus at any time.
>
> So, the way we have things now, is good. We just need to figure out a
> way to avoid the inclusion of C files.
>
> --
> balbi
>

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

Hi,

On Tue, Jun 14, 2011 at 11:33:32AM -0400, Arnaud Lacombe wrote:
> On Tue, Jun 14, 2011 at 3:32 AM, Felipe Balbi <balbi@ti.com> wrote:
> > On Tue, Jun 14, 2011 at 08:28:05AM +0200, Michal Nazarewicz wrote:
> >> On Mon, 13 Jun 2011 17:35:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >> >We also need to a find a better way to get rid of the inclusion of C
> >> >source files. I understand why Dave did it, but it's been quite some
> >> >time since those patches were committed - e.g.
> >> >7e75bc0f9006e995a0fa25f0a285addc3d5fd5cb - and no progress has been made
> >> >to fix that up. Probably Clang's link-time optimization would help a lot
> >> >on that, but Clang still needs quite some work to be usable for us, so
> >> >we need something else. Any ideas ?
> >>
> >> To me it always seemed like it's a bug in/lack of feature of Kbuild.
> >> Would making Kbuild allow building module from separate .o files be
> >> a problem?
> >
> > maybe the guys at linux-kbuild@vger can help answering this.
> >
> What is the problem exactly ?
> 
> The following patch:
> 
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 4fe92b1..cc432b8 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
>  # USB gadget drivers
>  #
>  g_zero-y                       := zero.o
> +g_zero-y                       += composite.o
> +g_zero-y                       += usbstring.o
> +g_zero-y                       += config.o
> +g_zero-y                       += epautoconf.o
> +g_zero-y                       += f_sourcesink.o
> +g_zero-y                       += f_loopback.o

yes, you can do that. But the problem is the runtime memory footprint
that we will have with that. At least that was the reason why Greg had
asked Dave to change it to how it is done now.

Do we support gcc --combine already ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Alan Stern]

On Tue, 14 Jun 2011, Felipe Balbi wrote:

> > --- a/drivers/usb/gadget/Makefile
> > +++ b/drivers/usb/gadget/Makefile
> > @@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
> >  # USB gadget drivers
> >  #
> >  g_zero-y                       := zero.o
> > +g_zero-y                       += composite.o
> > +g_zero-y                       += usbstring.o
> > +g_zero-y                       += config.o
> > +g_zero-y                       += epautoconf.o
> > +g_zero-y                       += f_sourcesink.o
> > +g_zero-y                       += f_loopback.o
> 
> yes, you can do that. But the problem is the runtime memory footprint
> that we will have with that. At least that was the reason why Greg had
> asked Dave to change it to how it is done now.

What exactly is the runtime memory footprint problem?  I thought the
whole reason for #include'ing .c files was that back then, the kbuild
system wasn't able to do this.

> Do we support gcc --combine already ?

As far as I know, the only advantage of gcc -combine is that it allows
the optimizer to work on more than one file at a time.  Would that
really make any significant difference for these particular source
files?

Alan Stern

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Arnaud Lacombe]

Hi,

On Tue, Jun 14, 2011 at 12:45 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Jun 14, 2011 at 11:33:32AM -0400, Arnaud Lacombe wrote:
>> On Tue, Jun 14, 2011 at 3:32 AM, Felipe Balbi <balbi@ti.com> wrote:
>> > On Tue, Jun 14, 2011 at 08:28:05AM +0200, Michal Nazarewicz wrote:
>> >> On Mon, 13 Jun 2011 17:35:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>> >> >We also need to a find a better way to get rid of the inclusion of C
>> >> >source files. I understand why Dave did it, but it's been quite some
>> >> >time since those patches were committed - e.g.
>> >> >7e75bc0f9006e995a0fa25f0a285addc3d5fd5cb - and no progress has been made
>> >> >to fix that up. Probably Clang's link-time optimization would help a lot
>> >> >on that, but Clang still needs quite some work to be usable for us, so
>> >> >we need something else. Any ideas ?
>> >>
>> >> To me it always seemed like it's a bug in/lack of feature of Kbuild.
>> >> Would making Kbuild allow building module from separate .o files be
>> >> a problem?
>> >
>> > maybe the guys at linux-kbuild@vger can help answering this.
>> >
>> What is the problem exactly ?
>>
>> The following patch:
>>
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 4fe92b1..cc432b8 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
>>  # USB gadget drivers
>>  #
>>  g_zero-y                       := zero.o
>> +g_zero-y                       += composite.o
>> +g_zero-y                       += usbstring.o
>> +g_zero-y                       += config.o
>> +g_zero-y                       += epautoconf.o
>> +g_zero-y                       += f_sourcesink.o
>> +g_zero-y                       += f_loopback.o
>
> yes, you can do that. But the problem is the runtime memory footprint
> that we will have with that. At least that was the reason why Greg had
> asked Dave to change it to how it is done now.
>
What number are we talking about ?

Using the same hack in `f_loopback.c'[0], here is the numbers I get on
x86-64, using gcc 4.5.1, with -O2:

combined (C):
   14017    1440     288   15745    3d81 drivers/usb/gadget/g_zero.o

not combined (P):
   13835    1472     296   15603    3cf3 drivers/usb/gadget/g_zero.o

the result at -Os is slightly worse:

(C)  11318    1344     256   12918    3276 drivers/usb/gadget/g_zero.o
(P)  11353    1360     264   12977    32b1 drivers/usb/gadget/g_zero.o

 - Arnaud

[0]: btw, this is a nice source of code duplication, as
sourcesink_add() and loopback_add() are pretty much identical. To some
extend, f_loopback.c and f_sourcesink.c seem to share very similar
code structure...

> Do we support gcc --combine already ?
>
not I know of, but my knowledge are limited.

 - Arnaud

> --
> balbi
>

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Sebastian Andrzej Siewior]

Alan Stern wrote:
>> Do we support gcc --combine already ?
> 
> As far as I know, the only advantage of gcc -combine is that it allows
> the optimizer to work on more than one file at a time.  Would that
> really make any significant difference for these particular source
> files?

It should also remove static functions which are not used. However I
really don't know the root cause of the memory footprint.
Having every file as a separate module might be a little overkill because
you need usually ~2 guard pages per module in virtual address space. Maybe
a "library" like module.

> 
> Alan Stern
> 

Sebastian

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]

On Tue, Jun 14, 2011 at 01:06:16PM -0400, Alan Stern wrote:
> On Tue, 14 Jun 2011, Felipe Balbi wrote:
> 
> > > --- a/drivers/usb/gadget/Makefile
> > > +++ b/drivers/usb/gadget/Makefile
> > > @@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
> > >  # USB gadget drivers
> > >  #
> > >  g_zero-y                       := zero.o
> > > +g_zero-y                       += composite.o
> > > +g_zero-y                       += usbstring.o
> > > +g_zero-y                       += config.o
> > > +g_zero-y                       += epautoconf.o
> > > +g_zero-y                       += f_sourcesink.o
> > > +g_zero-y                       += f_loopback.o
> > 
> > yes, you can do that. But the problem is the runtime memory footprint
> > that we will have with that. At least that was the reason why Greg had
> > asked Dave to change it to how it is done now.
> 
> What exactly is the runtime memory footprint problem?  I thought the
> whole reason for #include'ing .c files was that back then, the kbuild
> system wasn't able to do this.

not at all. Kbuild has always been able to generate one module from
several objects. But then we need to remove "static" from many
functions to achieve that.

> > Do we support gcc --combine already ?
> 
> As far as I know, the only advantage of gcc -combine is that it allows
> the optimizer to work on more than one file at a time.  Would that
> really make any significant difference for these particular source
> files?

AFAICT, gcc --combine will really combine the files as if they were one,
pretty much the same as including the entire C source.

Just look at the rationale from the commit log itself:

commit 4e9ba518ec19c6c961bf6074ec05ae1a927230bc
Author: David Brownell <dbrownell@users.sourceforge.net>
Date:   Mon Aug 18 17:41:02 2008 -0700

    usb gadget: link fixes for serial gadget
    
    Change how the serial gadget driver builds:  don't use
    separate compilation, since it works poorly when key parts
    are library code (with init sections etc).  Instead be as
    close as we can to "gcc --combine ...".
    
    Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

part of it is also in any gadget driver:

/*
 * Kbuild is not very cooperative with respect to linking separately
 * compiled library objects into one module.  So for now we won't use
 * separate compilation ... ensuring init/exit sections work to shrink
 * the runtime footprint, and giving us at least some parts of what
 * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
 */

you can find all such commits by:

$ git log --author="David Brownell" --grep="link fixes" -- drivers/usb/gadget/

And here are the original discussions:

http://marc.info/?l=linux-usb&m=121868805717787&w=2
http://marc.info/?l=linux-usb&m=121875765411492&w=2

In summary:

We don't want to have library code into their own drivers because, well,
you can only have one gadget driver at a time anyway. And we don't want
separate compilation due to having stuff out of init sections.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Arnaud Lacombe]

Hi,

On Tue, Jun 14, 2011 at 1:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> [...]
> In summary:
>
> We don't want to have library code into their own drivers because,
I am not sure to parse that correctly, could you elaborate ?

> well, you can only have one gadget driver at a time anyway. And we don't
> want separate compilation due to having stuff out of init sections.
>
on that last point, are you implying that section mapping is not kept
across the different linking steps ?

 - Arnaud

ps: about the code size issue, looking at f_acm.c, f_obex.c and
f_serial.c also highlight code duplication...

> --
> balbi
>

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

Hi,

On Tue, Jun 14, 2011 at 01:48:39PM -0400, Arnaud Lacombe wrote:
> On Tue, Jun 14, 2011 at 1:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> > [...]
> > In summary:
> >
> > We don't want to have library code into their own drivers because,
> I am not sure to parse that correctly, could you elaborate ?

u_*.c and composite.c are basically library code. We don't want to have
composite.ko and/or u_*.ko.

> > well, you can only have one gadget driver at a time anyway. And we don't
> > want separate compilation due to having stuff out of init sections.
> >
> on that last point, are you implying that section mapping is not kept
> across the different linking steps ?

could be. I don't remember all the details. That was back on 2.6.27
times. I would need to really revisit all the details on the archives.

What I remember is that Dave noted code shrunk when combining the source
files, even if he didn't mark all the other functions as "static".

Greg, do you remember why you started this discussion when you first
introduced g_utils.ko ??

> ps: about the code size issue, looking at f_acm.c, f_obex.c and
> f_serial.c also highlight code duplication...

that's USB. While the underlying connection is pretty much the same
(serial) between those, the descriptors are completely different and we
don't want to have either ifdefs nor phase out descriptors to something
else. See those as example implementations for possible real gadget
drivers.

As of today, we don't have a good solution to avoid that. In fact, a lot
of duplication has been removed when the composite gadget framework was
introduced.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Alan Stern]

On Tue, 14 Jun 2011, Felipe Balbi wrote:

> On Tue, Jun 14, 2011 at 01:06:16PM -0400, Alan Stern wrote:
> > On Tue, 14 Jun 2011, Felipe Balbi wrote:
> > 
> > > > --- a/drivers/usb/gadget/Makefile
> > > > +++ b/drivers/usb/gadget/Makefile
> > > > @@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
> > > >  # USB gadget drivers
> > > >  #
> > > >  g_zero-y                       := zero.o
> > > > +g_zero-y                       += composite.o
> > > > +g_zero-y                       += usbstring.o
> > > > +g_zero-y                       += config.o
> > > > +g_zero-y                       += epautoconf.o
> > > > +g_zero-y                       += f_sourcesink.o
> > > > +g_zero-y                       += f_loopback.o
> > > 
> > > yes, you can do that. But the problem is the runtime memory footprint
> > > that we will have with that. At least that was the reason why Greg had
> > > asked Dave to change it to how it is done now.
> > 
> > What exactly is the runtime memory footprint problem?  I thought the
> > whole reason for #include'ing .c files was that back then, the kbuild
> > system wasn't able to do this.
> 
> not at all. Kbuild has always been able to generate one module from
> several objects. But then we need to remove "static" from many
> functions to achieve that.

If you insist on building an object from multiple source files with
separate compilation, the price you pay is making symbols non-static.  
That's what we have .h files for.  And that's also why the [eou]hci-hcd
drivers use the same trick of #include'ing various .c files.  I really
don't know why people object to this idiom.  (Note that if the object 
is a module, non-static symbols don't matter.  It's an issue only when 
the object is linked into the main kernel.)

> AFAICT, gcc --combine will really combine the files as if they were one,
> pretty much the same as including the entire C source.
> 
> Just look at the rationale from the commit log itself:
> 
> commit 4e9ba518ec19c6c961bf6074ec05ae1a927230bc
> Author: David Brownell <dbrownell@users.sourceforge.net>
> Date:   Mon Aug 18 17:41:02 2008 -0700

Much as I admired David's work, he could sometimes be a little 
difficult to understand.

>     usb gadget: link fixes for serial gadget
>     
>     Change how the serial gadget driver builds:  don't use
>     separate compilation, since it works poorly when key parts
>     are library code (with init sections etc).  Instead be as

"works poorly" how?  That is, what goes wrong when you try to use 
separate compilation?

>     close as we can to "gcc --combine ...".
> 
>     Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> part of it is also in any gadget driver:
> 
> /*
>  * Kbuild is not very cooperative with respect to linking separately
>  * compiled library objects into one module.  So for now we won't use

(I assume this doesn't use the word "library" in a significant sense.  
It isn't referring to static-link .a library archives, for instance.)

In what way is kbuild uncooperative?  You said above that kbuild has 
always been able to build modules from separately-compiled objects.

>  * separate compilation ... ensuring init/exit sections work to shrink
>  * the runtime footprint, and giving us at least some parts of what
>  * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
>  */

Is the use of init sections the problem?  I don't see why it should 
be.  If you link multiple objects into a single module, all the init 
code should go into a single section.  Plenty of drivers throughout the 
kernel do this successfully.  In fact, g_serial did before David's 
patch, didn't it?

> you can find all such commits by:
> 
> $ git log --author="David Brownell" --grep="link fixes" -- drivers/usb/gadget/
> 
> And here are the original discussions:
> 
> http://marc.info/?l=linux-usb&m=121868805717787&w=2
> http://marc.info/?l=linux-usb&m=121875765411492&w=2

The problem Greg faced was that he tried to put all the library code 
into a separate module.  This clearly leads to potential problems.  
Suppose module A contains library code in an init section, which is 
used by the init code in module B.  When A is finished loading, its 
init code is jettisoned.  Then when B loads and tries to call that 
code, the system crashes.

It's not clear why single compilation saves space.  The difference
wasn't tremendous -- in David's g_serial test case
(http://marc.info/?l=linux-usb&m=121875765411492&w=2), 42 bytes were
saved out of 20000.  And the savings was almost entirely in data and
bss; you'd have to do some serious digging to figure what really was
going on.

> In summary:
> 
> We don't want to have library code into their own drivers because, well,
> you can only have one gadget driver at a time anyway.

That sentence isn't clear.  Perhaps you mean that having a single copy
of the executable library code at runtime, which could be shared among
multiple gadget drivers, doesn't help because there's only one gadget
driver present at a time.  With the new UDC framework, that won't be
true any more.

> And we don't want
> separate compilation due to having stuff out of init sections.

I still don't understand what the issue is with that.  There are plenty 
of driver modules built from multiple, separately-compiled objects 
that use init sections without trouble.

On the other hand, if you want to share a single copy of the executable
library code among multiple gadget drivers, then none of it can go in
an init section.  Since gadget drivers can be loaded and unloaded at
any time, the library code would have to remain in memory permanently 
-- it couldn't go in an init section.

Alan Stern

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6600 bytes --]

Hi,

On Tue, Jun 14, 2011 at 03:27:47PM -0400, Alan Stern wrote:
> On Tue, 14 Jun 2011, Felipe Balbi wrote:
> 
> > On Tue, Jun 14, 2011 at 01:06:16PM -0400, Alan Stern wrote:
> > > On Tue, 14 Jun 2011, Felipe Balbi wrote:
> > > 
> > > > > --- a/drivers/usb/gadget/Makefile
> > > > > +++ b/drivers/usb/gadget/Makefile
> > > > > @@ -34,6 +34,12 @@ obj-$(CONFIG_USB_FUSB300)    += fusb300_udc.o
> > > > >  # USB gadget drivers
> > > > >  #
> > > > >  g_zero-y                       := zero.o
> > > > > +g_zero-y                       += composite.o
> > > > > +g_zero-y                       += usbstring.o
> > > > > +g_zero-y                       += config.o
> > > > > +g_zero-y                       += epautoconf.o
> > > > > +g_zero-y                       += f_sourcesink.o
> > > > > +g_zero-y                       += f_loopback.o
> > > > 
> > > > yes, you can do that. But the problem is the runtime memory footprint
> > > > that we will have with that. At least that was the reason why Greg had
> > > > asked Dave to change it to how it is done now.
> > > 
> > > What exactly is the runtime memory footprint problem?  I thought the
> > > whole reason for #include'ing .c files was that back then, the kbuild
> > > system wasn't able to do this.
> > 
> > not at all. Kbuild has always been able to generate one module from
> > several objects. But then we need to remove "static" from many
> > functions to achieve that.
> 
> If you insist on building an object from multiple source files with
> separate compilation, the price you pay is making symbols non-static.  
> That's what we have .h files for.  And that's also why the [eou]hci-hcd
> drivers use the same trick of #include'ing various .c files.  I really
> don't know why people object to this idiom.  (Note that if the object 
> is a module, non-static symbols don't matter.  It's an issue only when 
> the object is linked into the main kernel.)
> 
> > AFAICT, gcc --combine will really combine the files as if they were one,
> > pretty much the same as including the entire C source.
> > 
> > Just look at the rationale from the commit log itself:
> > 
> > commit 4e9ba518ec19c6c961bf6074ec05ae1a927230bc
> > Author: David Brownell <dbrownell@users.sourceforge.net>
> > Date:   Mon Aug 18 17:41:02 2008 -0700
> 
> Much as I admired David's work, he could sometimes be a little 
> difficult to understand.
> 
> >     usb gadget: link fixes for serial gadget
> >     
> >     Change how the serial gadget driver builds:  don't use
> >     separate compilation, since it works poorly when key parts
> >     are library code (with init sections etc).  Instead be as
> 
> "works poorly" how?  That is, what goes wrong when you try to use 
> separate compilation?
> 
> >     close as we can to "gcc --combine ...".
> > 
> >     Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> >     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> > 
> > part of it is also in any gadget driver:
> > 
> > /*
> >  * Kbuild is not very cooperative with respect to linking separately
> >  * compiled library objects into one module.  So for now we won't use
> 
> (I assume this doesn't use the word "library" in a significant sense.  
> It isn't referring to static-link .a library archives, for instance.)
> 
> In what way is kbuild uncooperative?  You said above that kbuild has 
> always been able to build modules from separately-compiled objects.
> 
> >  * separate compilation ... ensuring init/exit sections work to shrink
> >  * the runtime footprint, and giving us at least some parts of what
> >  * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
> >  */
> 
> Is the use of init sections the problem?  I don't see why it should 
> be.  If you link multiple objects into a single module, all the init 
> code should go into a single section.  Plenty of drivers throughout the 
> kernel do this successfully.  In fact, g_serial did before David's 
> patch, didn't it?
> 
> > you can find all such commits by:
> > 
> > $ git log --author="David Brownell" --grep="link fixes" -- drivers/usb/gadget/
> > 
> > And here are the original discussions:
> > 
> > http://marc.info/?l=linux-usb&m=121868805717787&w=2
> > http://marc.info/?l=linux-usb&m=121875765411492&w=2
> 
> The problem Greg faced was that he tried to put all the library code 
> into a separate module.  This clearly leads to potential problems.  
> Suppose module A contains library code in an init section, which is 
> used by the init code in module B.  When A is finished loading, its 
> init code is jettisoned.  Then when B loads and tries to call that 
> code, the system crashes.
> 
> It's not clear why single compilation saves space.  The difference
> wasn't tremendous -- in David's g_serial test case
> (http://marc.info/?l=linux-usb&m=121875765411492&w=2), 42 bytes were
> saved out of 20000.  And the savings was almost entirely in data and
> bss; you'd have to do some serious digging to figure what really was
> going on.
> 
> > In summary:
> > 
> > We don't want to have library code into their own drivers because, well,
> > you can only have one gadget driver at a time anyway.
> 
> That sentence isn't clear.  Perhaps you mean that having a single copy
> of the executable library code at runtime, which could be shared among
> multiple gadget drivers, doesn't help because there's only one gadget

that's what I meant.

> driver present at a time.  With the new UDC framework, that won't be
> true any more.

Good point.

> > And we don't want
> > separate compilation due to having stuff out of init sections.
> 
> I still don't understand what the issue is with that.  There are plenty 
> of driver modules built from multiple, separately-compiled objects 
> that use init sections without trouble.

Maybe we should turn u_*.c and composite.c into their own modules now
that we're starting to allow multiple gadget controllers and thus
multiple gadget drivers loaded. I failed to see that, great catch.

> On the other hand, if you want to share a single copy of the executable
> library code among multiple gadget drivers, then none of it can go in
> an init section.  Since gadget drivers can be loaded and unloaded at
> any time, the library code would have to remain in memory permanently 
> -- it couldn't go in an init section.

very true. Let's leave this alone for now and revisit once all
controllers are converted to the ->start()/->stop() methods correctly.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Alan Stern]

On Tue, 14 Jun 2011, Felipe Balbi wrote:

> > > And we don't want
> > > separate compilation due to having stuff out of init sections.
> > 
> > I still don't understand what the issue is with that.  There are plenty 
> > of driver modules built from multiple, separately-compiled objects 
> > that use init sections without trouble.
> 
> Maybe we should turn u_*.c and composite.c into their own modules now
> that we're starting to allow multiple gadget controllers and thus
> multiple gadget drivers loaded. I failed to see that, great catch.

That's not what I meant.  I was asking for an explanation of your
statement: 

	And we don't want separate compilation due to having stuff
	out of init sections.

Why should separate compilation force you to move stuff out of init
sections?

Consider usbcore as an example.  Grepping through drivers/usb/core/*.c,
you'll see there are init routines in devio.c, inode.c, and usb.c.  
These files are all compiled separately.  Yet they are linked into a
single object module, usbcore.ko, and the init sections work out just
fine.

In short, I don't see what's wrong with separate compilation.  
Apparently the only problem David found was that it increased the size 
of the final driver by 42 bytes.  Is that really worth worrying about?

Alan Stern

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

Hi,

On Tue, Jun 14, 2011 at 04:01:29PM -0400, Alan Stern wrote:
> Consider usbcore as an example.  Grepping through drivers/usb/core/*.c,
> you'll see there are init routines in devio.c, inode.c, and usb.c.  
> These files are all compiled separately.  Yet they are linked into a
> single object module, usbcore.ko, and the init sections work out just
> fine.
> 
> In short, I don't see what's wrong with separate compilation.  
> Apparently the only problem David found was that it increased the size 
> of the final driver by 42 bytes.  Is that really worth worrying about?

probably not... But Greg initiated the discussion. It was something
related with always recompiling the same objects or something. I really
don't recall.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Greg KH]

On Tue, Jun 14, 2011 at 09:36:22PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 14, 2011 at 01:48:39PM -0400, Arnaud Lacombe wrote:
> > On Tue, Jun 14, 2011 at 1:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> > > [...]
> > > In summary:
> > >
> > > We don't want to have library code into their own drivers because,
> > I am not sure to parse that correctly, could you elaborate ?
> 
> u_*.c and composite.c are basically library code. We don't want to have
> composite.ko and/or u_*.ko.
> 
> > > well, you can only have one gadget driver at a time anyway. And we don't
> > > want separate compilation due to having stuff out of init sections.
> > >
> > on that last point, are you implying that section mapping is not kept
> > across the different linking steps ?
> 
> could be. I don't remember all the details. That was back on 2.6.27
> times. I would need to really revisit all the details on the archives.
> 
> What I remember is that Dave noted code shrunk when combining the source
> files, even if he didn't mark all the other functions as "static".
> 
> Greg, do you remember why you started this discussion when you first
> introduced g_utils.ko ??

That was a while ago.  I think I was having some build issues with the
including of the .c files due to some driver core changes I was working
on, so I tried to create that, but then ran into other issues with
#defines causing different things to be build in odd ways and breaking
the parallel build system.

I really can't remember the specifics, sorry.

If you can figure a way to clean this up, I would not object to that at
all.

Sorry I can't be of more help here.

greg k-h

Re: generate one module from multiple object files (was: Re: [PATCH 2/2] usb: gadget: convert all users to the new udc)

[Greg KH]

On Tue, Jun 14, 2011 at 11:16:33PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 14, 2011 at 04:01:29PM -0400, Alan Stern wrote:
> > Consider usbcore as an example.  Grepping through drivers/usb/core/*.c,
> > you'll see there are init routines in devio.c, inode.c, and usb.c.  
> > These files are all compiled separately.  Yet they are linked into a
> > single object module, usbcore.ko, and the init sections work out just
> > fine.
> > 
> > In short, I don't see what's wrong with separate compilation.  
> > Apparently the only problem David found was that it increased the size 
> > of the final driver by 42 bytes.  Is that really worth worrying about?
> 
> probably not... But Greg initiated the discussion. It was something
> related with always recompiling the same objects or something. I really
> don't recall.

42 bytes is not worth worrying about :)

If it can be merged into a single .ko that the others use, please do it.
But for some reason I had problems the last time I tried it, but I can't
remember what they were.

greg k-h