kernel.bbclass do_sizecheck behaviour changes

kernel.bbclass do_sizecheck behaviour changes

[Mike Crowe]

I discovered today that our kernel size check has been ineffective since we
took 7384d2831c713ac5999aca83c312154dc15cec56 from 2014 which changed the
units for KERNEL_IMAGE_MAXSIZE from bytes to kilobytes.

However whilst fixing that, I also noticed that
849b67b2e4820564b5e5c9bd4bb293c44351c5f3 in 2016 changed the consequences
of exceeding the size from being fatal to merely a warning.

This second change in behaviour wasn't described in the commit message, so
I'm not sure whether it was intentional. I can believe that when generating
multiple kernel image types it may not be essential that they all fit, but
I don't really know what the use case for the feature is.

I could fix this by adding to kernel.bbclass something like:

 KERNEL_IMAGE_MAXSIZE_CONSEQUENCE ?= "warn"

and then using it when delivering the bad news so that recipes can override
it if they wish.

Alternatively, I could treat the error as being fatal if none of the kernel
images fit within the required size. This would degenerate to the old
behaviour automatically if KERNEL_IMAGETYPES contains only one entry.

Does one of these solutions appeal, or is there another even better
solution?

Thanks.

Mike.

Re: kernel.bbclass do_sizecheck behaviour changes

[Khem Raj]

On Thu, Dec 7, 2017 at 8:15 AM, Mike Crowe <mac@mcrowe.com> wrote:
> I discovered today that our kernel size check has been ineffective since we
> took 7384d2831c713ac5999aca83c312154dc15cec56 from 2014 which changed the
> units for KERNEL_IMAGE_MAXSIZE from bytes to kilobytes.
>
> However whilst fixing that, I also noticed that
> 849b67b2e4820564b5e5c9bd4bb293c44351c5f3 in 2016 changed the consequences
> of exceeding the size from being fatal to merely a warning.
>
> This second change in behaviour wasn't described in the commit message, so
> I'm not sure whether it was intentional. I can believe that when generating
> multiple kernel image types it may not be essential that they all fit, but
> I don't really know what the use case for the feature is.
>
> I could fix this by adding to kernel.bbclass something like:
>
>  KERNEL_IMAGE_MAXSIZE_CONSEQUENCE ?= "warn"
>
> and then using it when delivering the bad news so that recipes can override
> it if they wish.
>
> Alternatively, I could treat the error as being fatal if none of the kernel
> images fit within the required size. This would degenerate to the old
> behaviour automatically if KERNEL_IMAGETYPES contains only one entry.
>
> Does one of these solutions appeal, or is there another even better
> solution?
>

I think default should be a warning with a possibility to be turned
into an error
if user wishes too. I think if you are generating multiple kernel types then you
will use all of them somewhere, so I would say if the size of one of
them excedes
the set limit them break the build.

> Thanks.
>
> Mike.
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: kernel.bbclass do_sizecheck behaviour changes

[Andre McCurdy]

On Thu, Dec 7, 2017 at 11:20 AM, Khem Raj <raj.khem@gmail.com> wrote:
> On Thu, Dec 7, 2017 at 8:15 AM, Mike Crowe <mac@mcrowe.com> wrote:
>> I discovered today that our kernel size check has been ineffective since we
>> took 7384d2831c713ac5999aca83c312154dc15cec56 from 2014 which changed the
>> units for KERNEL_IMAGE_MAXSIZE from bytes to kilobytes.
>>
>> However whilst fixing that, I also noticed that
>> 849b67b2e4820564b5e5c9bd4bb293c44351c5f3 in 2016 changed the consequences
>> of exceeding the size from being fatal to merely a warning.
>>
>> This second change in behaviour wasn't described in the commit message, so
>> I'm not sure whether it was intentional. I can believe that when generating
>> multiple kernel image types it may not be essential that they all fit, but
>> I don't really know what the use case for the feature is.
>>
>> I could fix this by adding to kernel.bbclass something like:
>>
>>  KERNEL_IMAGE_MAXSIZE_CONSEQUENCE ?= "warn"
>>
>> and then using it when delivering the bad news so that recipes can override
>> it if they wish.
>>
>> Alternatively, I could treat the error as being fatal if none of the kernel
>> images fit within the required size. This would degenerate to the old
>> behaviour automatically if KERNEL_IMAGETYPES contains only one entry.

That sounds like a good solution.

>> Does one of these solutions appeal, or is there another even better
>> solution?
>
> I think default should be a warning with a possibility to be turned
> into an error
> if user wishes too. I think if you are generating multiple kernel types then you
> will use all of them somewhere, so I would say if the size of one of
> them excedes
> the set limit them break the build.

The size limit for an uncompressed kernel vs a compressed kernel is
going to be quite different, so defining one size limit and applying
it to all images doesn't seem logical.

Re: kernel.bbclass do_sizecheck behaviour changes

[Mike Crowe]

On Thursday 07 December 2017 at 11:44:12 -0800, Andre McCurdy wrote:
> On Thu, Dec 7, 2017 at 11:20 AM, Khem Raj <raj.khem@gmail.com> wrote:
> > On Thu, Dec 7, 2017 at 8:15 AM, Mike Crowe <mac@mcrowe.com> wrote:
> >>                             I also noticed that
> >> 849b67b2e4820564b5e5c9bd4bb293c44351c5f3 in 2016 changed the consequences
> >> of exceeding the size from being fatal to merely a warning.
> >>
> >> This second change in behaviour wasn't described in the commit message, so
> >> I'm not sure whether it was intentional. I can believe that when generating
> >> multiple kernel image types it may not be essential that they all fit, but
> >> I don't really know what the use case for the feature is.
> >>
> >> I could fix this by adding to kernel.bbclass something like:
> >>
> >>  KERNEL_IMAGE_MAXSIZE_CONSEQUENCE ?= "warn"
> >>
> >> and then using it when delivering the bad news so that recipes can override
> >> it if they wish.
> >>
> >> Alternatively, I could treat the error as being fatal if none of the kernel
> >> images fit within the required size. This would degenerate to the old
> >> behaviour automatically if KERNEL_IMAGETYPES contains only one entry.
> 
> That sounds like a good solution.

Thanks. I've implemented that and it seems to work. I shall provide a patch
shortly.

However, despite appearances it seems that
849b67b2e4820564b5e5c9bd4bb293c44351c5f3 did accidentally make exceeding
KERNEL_IMAGE_MAXSIZE fatal because there's no such function as "warn". I
think that it should have been "bbwarn":

 run.do_sizecheck.12393: line 119: warn: command not found

Mike.

Re: kernel.bbclass do_sizecheck behaviour changes

[Khem Raj]

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

On Thu, Dec 7, 2017 at 11:44 AM Andre McCurdy <armccurdy@gmail.com> wrote:

> On Thu, Dec 7, 2017 at 11:20 AM, Khem Raj <raj.khem@gmail.com> wrote:
> > On Thu, Dec 7, 2017 at 8:15 AM, Mike Crowe <mac@mcrowe.com> wrote:
> >> I discovered today that our kernel size check has been ineffective
> since we
> >> took 7384d2831c713ac5999aca83c312154dc15cec56 from 2014 which changed
> the
> >> units for KERNEL_IMAGE_MAXSIZE from bytes to kilobytes.
> >>
> >> However whilst fixing that, I also noticed that
> >> 849b67b2e4820564b5e5c9bd4bb293c44351c5f3 in 2016 changed the
> consequences
> >> of exceeding the size from being fatal to merely a warning.
> >>
> >> This second change in behaviour wasn't described in the commit message,
> so
> >> I'm not sure whether it was intentional. I can believe that when
> generating
> >> multiple kernel image types it may not be essential that they all fit,
> but
> >> I don't really know what the use case for the feature is.
> >>
> >> I could fix this by adding to kernel.bbclass something like:
> >>
> >>  KERNEL_IMAGE_MAXSIZE_CONSEQUENCE ?= "warn"
> >>
> >> and then using it when delivering the bad news so that recipes can
> override
> >> it if they wish.
> >>
> >> Alternatively, I could treat the error as being fatal if none of the
> kernel
> >> images fit within the required size. This would degenerate to the old
> >> behaviour automatically if KERNEL_IMAGETYPES contains only one entry.
>
> That sounds like a good solution.
>
> >> Does one of these solutions appeal, or is there another even better
> >> solution?
> >
> > I think default should be a warning with a possibility to be turned
> > into an error
> > if user wishes too. I think if you are generating multiple kernel types
> then you
> > will use all of them somewhere, so I would say if the size of one of
> > them excedes
> > the set limit them break the build.
>
> The size limit for an uncompressed kernel vs a compressed kernel is
> going to be quite different, so defining one size limit and applying
> it to all images doesn't seem logical.


I was hoping that we are talking about deployed kernels here compressed
sizes can vary widely too

So may be we can have a hook to define which image types should be
monitored and what the limits are for individual type

>
>

[-- Attachment #2: Type: text/html, Size: 3150 bytes --]

Re: kernel.bbclass do_sizecheck behaviour changes

[Andre McCurdy]

On Fri, Dec 8, 2017 at 1:34 PM, Khem Raj <raj.khem@gmail.com> wrote:
>>
>> The size limit for an uncompressed kernel vs a compressed kernel is
>> going to be quite different, so defining one size limit and applying
>> it to all images doesn't seem logical.
>
> I was hoping that we are talking about deployed kernels here compressed
> sizes can vary widely too
>
> So may be we can have a hook to define which image types should be monitored
> and what the limits are for individual type

We could do a lot of things and add a lot of complexity. However based
on the fact that the simplest test has been broken for some time and
no-one noticed, the evidence is that the kernel image size check isn't
a widely used feature (and the kernel image size check with multiple
kernel image types even less so, if at all) and so probably shouldn't
be the focus of our efforts.

Re: kernel.bbclass do_sizecheck behaviour changes

[Andrea Adami]

On Fri, Dec 8, 2017 at 11:24 PM, Andre McCurdy <armccurdy@gmail.com> wrote:
> On Fri, Dec 8, 2017 at 1:34 PM, Khem Raj <raj.khem@gmail.com> wrote:
>>>
>>> The size limit for an uncompressed kernel vs a compressed kernel is
>>> going to be quite different, so defining one size limit and applying
>>> it to all images doesn't seem logical.
>>
>> I was hoping that we are talking about deployed kernels here compressed
>> sizes can vary widely too
>>
>> So may be we can have a hook to define which image types should be monitored
>> and what the limits are for individual type
>
> We could do a lot of things and add a lot of complexity. However based
> on the fact that the simplest test has been broken for some time and
> no-one noticed, the evidence is that the kernel image size check isn't
> a widely used feature (and the kernel image size check with multiple
> kernel image types even less so, if at all) and so probably shouldn't
> be the focus of our efforts.
> --


Hi,

we use do_sizecheck since oe-classic and noticed the change thus
patched our kernel recipe back in 2014.
So no, it has not been broken for long in the actively maintained layers ;).
See
http://cgit.openembedded.org/meta-handheld/commit/recipes-kernel/linux?id=e51a0c84c65651a646ac701a411f724c0ed583a6

We do build only one type (zImage) so is preferable to have the test
fail if size exceeds, not just a warning.

Besides, I am annoyed by the related change for the creation of
different image types in case of errors: I do build ubifs images and
if the size exceeds the eraseblocks of the device and mkufbifs fails
then the other images types are not deployed (tar.gz, ext3, ...).
I plan to fix this somehow, it wasn't like that originally.

Cheers
Andrea


> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: kernel.bbclass do_sizecheck behaviour changes

[Mike Crowe]

On Friday 08 December 2017 at 14:24:17 -0800, Andre McCurdy wrote:
> On Fri, Dec 8, 2017 at 1:34 PM, Khem Raj <raj.khem@gmail.com> wrote:
> >>
> >> The size limit for an uncompressed kernel vs a compressed kernel is
> >> going to be quite different, so defining one size limit and applying
> >> it to all images doesn't seem logical.
> >
> > I was hoping that we are talking about deployed kernels here compressed
> > sizes can vary widely too
> >
> > So may be we can have a hook to define which image types should be monitored
> > and what the limits are for individual type
> 
> We could do a lot of things and add a lot of complexity. However based
> on the fact that the simplest test has been broken for some time and
> no-one noticed, the evidence is that the kernel image size check isn't
> a widely used feature (and the kernel image size check with multiple
> kernel image types even less so, if at all) and so probably shouldn't
> be the focus of our efforts.

Well, anyone who also didn't notice the change in units probably hasn't
noticed the breakage yet either. :-)

But, I mostly agree. It's clear the size check feature is used, but it's
quite possible that it isn't used in combination with multiple image type
support. So, I think something along the same lines as the patch I
submitted[1] is worth applying, even if in the longer term something better
might be desirable.

Mike.

[1] https://patchwork.openembedded.org/patch/146518/
    http://lists.openembedded.org/pipermail/openembedded-core/2017-December/145402.html