[RFC] Preventing race when compiling external kernel modules

Submitted by Richard Purdie on Oct. 14, 2011, 11:42 a.m.

Details

Message ID 1318592560.942.3.camel@ted
State New, archived
Headers show

Commit Message

Richard Purdie Oct. 14, 2011, 11:42 a.m.
On Fri, 2011-10-14 at 12:47 +0200, Anders Darander wrote:
> * Anders Darander <anders@chargestorm.se> [111014 09:55]:
> > In our local tree, I've circumvented this race by applying a patch like
> > [3]. (Well, we could likely have put the lock in do_make_scripts()
> > instead of module_do_compile(), as we have done currently). Obviously,
> > I'm not proposing to apply this patch, as it depends on lockfile from
> > the procmail-package (host-package).
> 
> Just to confirm, it seems (after a very few tests) that it indeed is
> enough to guard the do_make_scripts() with the lock.
> 
> However, the question on how to make the real solution remains...

I've not tested this but it might give you enough info to test
something:

(Basic idea is to promote that function to a task, then apply a lock to
it).



Cheers,

Richard

Patch hide | download patch | download mbox

diff --git a/meta/classes/module.bbclass b/meta/classes/module.bbclass
index 572df0d..5602e74 100644
--- a/meta/classes/module.bbclass
+++ b/meta/classes/module.bbclass
@@ -14,8 +14,10 @@  do_make_scripts() {
                   -C ${STAGING_KERNEL_DIR} scripts
 }
 
+addtask make_scripts before compile
+do_make_scripts[lockfiles] = "${TMPDIR}/kernel-scripts.lock"
+
 module_do_compile() {
-       do_make_scripts
        unset CFLAGS CPPFLAGS CXXFLAGS LDFLAGS
        oe_runmake KERNEL_PATH=${STAGING_KERNEL_DIR}   \
                   KERNEL_SRC=${STAGING_KERNEL_DIR}    \

Comments

Koen Kooi Oct. 14, 2011, 11:49 a.m.
Op 14 okt. 2011, om 13:42 heeft Richard Purdie het volgende geschreven:

> On Fri, 2011-10-14 at 12:47 +0200, Anders Darander wrote:
>> * Anders Darander <anders@chargestorm.se> [111014 09:55]:
>>> In our local tree, I've circumvented this race by applying a patch like
>>> [3]. (Well, we could likely have put the lock in do_make_scripts()
>>> instead of module_do_compile(), as we have done currently). Obviously,
>>> I'm not proposing to apply this patch, as it depends on lockfile from
>>> the procmail-package (host-package).
>> 
>> Just to confirm, it seems (after a very few tests) that it indeed is
>> enough to guard the do_make_scripts() with the lock.
>> 
>> However, the question on how to make the real solution remains...
> 
> I've not tested this but it might give you enough info to test
> something:
> 
> (Basic idea is to promote that function to a task, then apply a lock to
> it).

Or taking a step back, deleting the scripts to save 0.002 kilobytes in sysroot was not a good idea. Shall we just stop deleting them and go back to the old way which actually worked?
Richard Purdie Oct. 14, 2011, 11:58 a.m.
On Fri, 2011-10-14 at 13:49 +0200, Koen Kooi wrote:
> Op 14 okt. 2011, om 13:42 heeft Richard Purdie het volgende geschreven:
> 
> > On Fri, 2011-10-14 at 12:47 +0200, Anders Darander wrote:
> >> * Anders Darander <anders@chargestorm.se> [111014 09:55]:
> >>> In our local tree, I've circumvented this race by applying a patch like
> >>> [3]. (Well, we could likely have put the lock in do_make_scripts()
> >>> instead of module_do_compile(), as we have done currently). Obviously,
> >>> I'm not proposing to apply this patch, as it depends on lockfile from
> >>> the procmail-package (host-package).
> >> 
> >> Just to confirm, it seems (after a very few tests) that it indeed is
> >> enough to guard the do_make_scripts() with the lock.
> >> 
> >> However, the question on how to make the real solution remains...
> > 
> > I've not tested this but it might give you enough info to test
> > something:
> > 
> > (Basic idea is to promote that function to a task, then apply a lock to
> > it).
> 
> Or taking a step back, deleting the scripts to save 0.002 kilobytes in
> sysroot was not a good idea. Shall we just stop deleting them and go
> back to the old way which actually worked?

It doesn't work, the scripts are compiled binaries and switching between
32 and 64 bit systems with sstate was a disaster. Either we need to
split the scripts off and mark them as "native" architecture or rebuild
them, we chose the latter for good reason.

Cheers,

Richard
Anders Darander Oct. 17, 2011, 11:54 a.m.
* Richard Purdie <richard.purdie@linuxfoundation.org> [111014 13:43]:
> On Fri, 2011-10-14 at 12:47 +0200, Anders Darander wrote:
> > * Anders Darander <anders@chargestorm.se> [111014 09:55]:
> > > In our local tree, I've circumvented this race by applying a patch like
> > > [3]. (Well, we could likely have put the lock in do_make_scripts()
> > > instead of module_do_compile(), as we have done currently). Obviously,
> > > I'm not proposing to apply this patch, as it depends on lockfile from
> > > the procmail-package (host-package).

> > Just to confirm, it seems (after a very few tests) that it indeed is
> > enough to guard the do_make_scripts() with the lock.

> > However, the question on how to make the real solution remains...

> I've not tested this but it might give you enough info to test
> something:

> (Basic idea is to promote that function to a task, then apply a lock to
> it).

Thanks!

I had an idea that it might be something like this, but I didn't really
know if I was completely off... I'll try this and see if I can come up
with something that works reliable for us. When I've got something
ready, I'll send a real patch.

Cheers,
Anders


> diff --git a/meta/classes/module.bbclass b/meta/classes/module.bbclass
> index 572df0d..5602e74 100644
> --- a/meta/classes/module.bbclass
> +++ b/meta/classes/module.bbclass
> @@ -14,8 +14,10 @@ do_make_scripts() {
>                    -C ${STAGING_KERNEL_DIR} scripts
>  }

> +addtask make_scripts before compile
> +do_make_scripts[lockfiles] = "${TMPDIR}/kernel-scripts.lock"
> +
>  module_do_compile() {
> -       do_make_scripts
>         unset CFLAGS CPPFLAGS CXXFLAGS LDFLAGS
>         oe_runmake KERNEL_PATH=${STAGING_KERNEL_DIR}   \
>                    KERNEL_SRC=${STAGING_KERNEL_DIR}    \
Anders Darander Oct. 18, 2011, 8:19 a.m.
* Richard Purdie <richard.purdie@linuxfoundation.org> [111014 13:43]:
> On Fri, 2011-10-14 at 12:47 +0200, Anders Darander wrote:
> > * Anders Darander <anders@chargestorm.se> [111014 09:55]:
> > > In our local tree, I've circumvented this race by applying a patch like
> > > [3]. (Well, we could likely have put the lock in do_make_scripts()
> > > instead of module_do_compile(), as we have done currently). Obviously,
> > > I'm not proposing to apply this patch, as it depends on lockfile from
> > > the procmail-package (host-package).

> > Just to confirm, it seems (after a very few tests) that it indeed is
> > enough to guard the do_make_scripts() with the lock.

> > However, the question on how to make the real solution remains...

> I've not tested this but it might give you enough info to test
> something:

> (Basic idea is to promote that function to a task, then apply a lock to
> it).

Just to give you some status feedback. 
Your code gave me enough to get my previously failed attempt at
promoting the function to a task. Which obviously showed a few new
problems, which I think I've solved. I'll just want to test it a few
more times, but expect a patch today or tomorrow.

Cheers,
Anders