Patchwork [RFC] Preventing race when compiling external kernel modules

login
mail settings
Submitter Richard Purdie
Date Oct. 14, 2011, 11:42 a.m.
Message ID <1318592560.942.3.camel@ted>
Download mbox | patch
Permalink /patch/13229/
State New, archived
Headers show

Comments

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
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

Patch

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}    \