[RFC] Preventing race when compiling external kernel modules

[RFC] Preventing race when compiling external kernel modules

[Anders Darander]

Hi,

As I've detailed in [1] (originally in [2], but [1] shows a more typical
error), we're experiencing a race when including several external
modules in our image.

The race occurs during the compile step of the modules. Usually the
problem shows up when compiling (or calculating the dependencies) of
scripts/basic/fixdeps in $STAGING_KERNEL_DIR.

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

I've noticed that we have some bitbake functionality to implement
locking, bb.utils.lockfile() and bb.utils.unlockfile(), but I'm not sure
how to use that to guard the module_do_compile (or do_make_script)
steps, as these are shell scritp functions.

Any advice on how to make a fix that would be upstreamable to
oe-core/yocto? I'd prefer to be able to remove as much local patches as
possible from our internal build tree...

Thanks in advance!
Cheers,
Anders


[1] http://lists.linuxtogo.org/pipermail/openembedded-core/2011-September/009401.html
[2] http://lists.linuxtogo.org/pipermail/openembedded-core/2011-September/009371.html

[3]
$ cat 0001-module.bbclass-add-lock-to-prevent-error-bulding-ext.patch 
From 9e23beb2ec499cdba04a11165efe8867ad4dc42a Mon Sep 17 00:00:00 2001
From: Anders Darander <anders@chargestorm.se>
Date: Mon, 10 Oct 2011 14:26:20 +0200
Subject: [PATCH] module.bbclass: add lock to prevent error bulding ext
 modules

When external modules are built, certain files in STAGING_KERNEL_DIR
might get rebuilt.
This raises a potential race condition. Prevent this by using a lockfile
to make external
modules build in sequence.

This patch is not applicable upstream, as it requires lockfile from
procmail.

Signed-off-by: Anders Darander <anders@chargestorm.se>
---
 meta/classes/module.bbclass |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/meta/classes/module.bbclass b/meta/classes/module.bbclass
index 572df0d..46abd81 100644
--- a/meta/classes/module.bbclass
+++ b/meta/classes/module.bbclass
@@ -15,6 +15,8 @@ do_make_scripts() {
 }
 
 module_do_compile() {
+       trap "rm -f ${STAGING_KERNEL_DIR}/external_modules.lock; exit 1"
INT TERM EXIT
+       lockfile -r-1 ${STAGING_KERNEL_DIR}/external_modules.lock
        do_make_scripts
        unset CFLAGS CPPFLAGS CXXFLAGS LDFLAGS
        oe_runmake KERNEL_PATH=${STAGING_KERNEL_DIR}   \
@@ -23,6 +25,8 @@ module_do_compile() {
                   CC="${KERNEL_CC}" LD="${KERNEL_LD}" \
                   AR="${KERNEL_AR}" \
                   ${MAKE_TARGETS}
+       rm -f ${STAGING_KERNEL_DIR}/external_modules.lock
+        trap - INT TERM EXIT
 }
 
 module_do_install() {
-- 
1.7.7


-- 
Anders Darander
ChargeStorm AB / eStorm AB	

Re: [RFC] Preventing race when compiling external kernel modules

[Anders Darander]

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

Cheers,
Anders

> [3]
> $ cat 0001-module.bbclass-add-lock-to-prevent-error-bulding-ext.patch 
> From 9e23beb2ec499cdba04a11165efe8867ad4dc42a Mon Sep 17 00:00:00 2001
> From: Anders Darander <anders@chargestorm.se>
> Date: Mon, 10 Oct 2011 14:26:20 +0200
> Subject: [PATCH] module.bbclass: add lock to prevent error bulding ext
>  modules

> When external modules are built, certain files in STAGING_KERNEL_DIR
> might get rebuilt.
> This raises a potential race condition. Prevent this by using a lockfile
> to make external
> modules build in sequence.

> This patch is not applicable upstream, as it requires lockfile from
> procmail.

> Signed-off-by: Anders Darander <anders@chargestorm.se>
> ---
>  meta/classes/module.bbclass |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)

> diff --git a/meta/classes/module.bbclass b/meta/classes/module.bbclass
> index 572df0d..46abd81 100644
> --- a/meta/classes/module.bbclass
> +++ b/meta/classes/module.bbclass
> @@ -15,6 +15,8 @@ do_make_scripts() {
>  }

>  module_do_compile() {
> +       trap "rm -f ${STAGING_KERNEL_DIR}/external_modules.lock; exit 1"
> INT TERM EXIT
> +       lockfile -r-1 ${STAGING_KERNEL_DIR}/external_modules.lock
>         do_make_scripts
>         unset CFLAGS CPPFLAGS CXXFLAGS LDFLAGS
>         oe_runmake KERNEL_PATH=${STAGING_KERNEL_DIR}   \
> @@ -23,6 +25,8 @@ module_do_compile() {
>                    CC="${KERNEL_CC}" LD="${KERNEL_LD}" \
>                    AR="${KERNEL_AR}" \
>                    ${MAKE_TARGETS}
> +       rm -f ${STAGING_KERNEL_DIR}/external_modules.lock
> +        trap - INT TERM EXIT
>  }
>  module_do_install() {

-- 
Anders Darander
ChargeStorm AB / eStorm AB

Re: [RFC] Preventing race when compiling external kernel modules

[Richard Purdie]

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

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


Cheers,

Richard

Re: [RFC] Preventing race when compiling external kernel modules

[Koen Kooi]

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?

Re: [RFC] Preventing race when compiling external kernel modules

[Richard Purdie]

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

Re: [RFC] Preventing race when compiling external kernel modules

[Anders Darander]

* 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
ChargeStorm AB / eStorm AB

Re: [RFC] Preventing race when compiling external kernel modules

[Anders Darander]

* 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

-- 
Anders Darander
ChargeStorm AB / eStorm AB