The future of ubi_assert()

The future of ubi_assert()

[Richard Weinberger]

Artem,

I'm not happy with ubi_assert().
Currently it only prints a warning and a stack trace but execution
continues. In production nobody will notice and while developing turning
it into a plain BUG_ON is most of the time more useful because execution stops
exactly where the boo boo happens one can analyze stack/registers.

I propose splitting ubi_assert() into two new functions.

1. ubi_bug_on()
Basically a BUG_ON(), it shall be used for assertions where execution of
UBI cannot proceed and anything we can do is crashing the machine.

2. ubi_warn_on()
This macro shall be used for assertions where further execution is possible
in read-only mode. ubi_warn_on() would be a WARN_ON() plus ubi_ro_mode().

I'm sure that the vast majority of all ubi_asserts() can be turned into a ubi_warn_on().

What do you think?

Thanks,
//richard

Re: The future of ubi_assert()

[Artem Bityutskiy]

On Wed, 2014-11-05 at 23:02 +0100, Richard Weinberger wrote:
> Artem,
> 
> I'm not happy with ubi_assert().

Good start of an e-mail, immediately sets the goal - make Richard
happy :-)


> Currently it only prints a warning and a stack trace but execution
> continues.

Yeah, that was an idea initially, when this all was in process of
creation, and we were testing it a lot, and had problems. We did wanted
to see a warning, and then let things continue, and see what happens
next. And we put really a lot of them, and often they were bogus, and
sometimes it was good for production even, because a bogus assert did
not stop the whole thing.

>  In production nobody will notice and while developing turning
> it into a plain BUG_ON is most of the time more useful because execution stops
> exactly where the boo boo happens one can analyze stack/registers.

Sure, for some of the critical ones it BUG_ON is a better answer.

> I propose splitting ubi_assert() into two new functions.
> 
> 1. ubi_bug_on()
> Basically a BUG_ON(), it shall be used for assertions where execution of
> UBI cannot proceed and anything we can do is crashing the machine.

Just use plain BUG_ON() then.

> 2. ubi_warn_on()
> This macro shall be used for assertions where further execution is possible
> in read-only mode. ubi_warn_on() would be a WARN_ON() plus ubi_ro_mode().

OK, just use plain WARN_ON(). Many asserts can be just removed even, I
think.

> I'm sure that the vast majority of all ubi_asserts() can be turned into a ubi_warn_on().

The reason we introduced macros, originally, was that we did not want
all our asserts to be compiled in. Because we put them nearly
everywhere. Wrapping allowed us to compile them off when debugging was
disabled.

But nowadays many of them can be just killed, and we can use WARN_ON() /
BUG_ON() without wrapping them.

Re: The future of ubi_assert()

[Richard Weinberger]

Am 06.11.2014 um 08:21 schrieb Artem Bityutskiy:
> On Wed, 2014-11-05 at 23:02 +0100, Richard Weinberger wrote:
>> Artem,
>>
>> I'm not happy with ubi_assert().
> 
> Good start of an e-mail, immediately sets the goal - make Richard
> happy :-)
> 
> 
>> Currently it only prints a warning and a stack trace but execution
>> continues.
> 
> Yeah, that was an idea initially, when this all was in process of
> creation, and we were testing it a lot, and had problems. We did wanted
> to see a warning, and then let things continue, and see what happens
> next. And we put really a lot of them, and often they were bogus, and
> sometimes it was good for production even, because a bogus assert did
> not stop the whole thing.
> 
>>  In production nobody will notice and while developing turning
>> it into a plain BUG_ON is most of the time more useful because execution stops
>> exactly where the boo boo happens one can analyze stack/registers.
> 
> Sure, for some of the critical ones it BUG_ON is a better answer.
> 
>> I propose splitting ubi_assert() into two new functions.
>>
>> 1. ubi_bug_on()
>> Basically a BUG_ON(), it shall be used for assertions where execution of
>> UBI cannot proceed and anything we can do is crashing the machine.
> 
> Just use plain BUG_ON() then.

Fine by me.

>> 2. ubi_warn_on()
>> This macro shall be used for assertions where further execution is possible
>> in read-only mode. ubi_warn_on() would be a WARN_ON() plus ubi_ro_mode().
> 
> OK, just use plain WARN_ON(). Many asserts can be just removed even, I
> think.

I think we can do better.
Think of wl.c, if an ubi_assert() in wl.c does not hold, we can put the UBI device
into read-only mode and continue execution.
BUG_ON() would be too much and WARN_ON() would lead to data corruption as the execution
would go on...
Something like ext4's errors=remount-ro. :)

>> I'm sure that the vast majority of all ubi_asserts() can be turned into a ubi_warn_on().
> 
> The reason we introduced macros, originally, was that we did not want
> all our asserts to be compiled in. Because we put them nearly
> everywhere. Wrapping allowed us to compile them off when debugging was
> disabled.

Makes sense.

> But nowadays many of them can be just killed, and we can use WARN_ON() /
> BUG_ON() without wrapping them.
> 

Please see my comment above on wl.c.

Thanks,
//richard

Re: The future of ubi_assert()

[Artem Bityutskiy]

On Thu, 2014-11-06 at 09:07 +0100, Richard Weinberger wrote:
> I think we can do better.
> Think of wl.c, if an ubi_assert() in wl.c does not hold, we can put the UBI device
> into read-only mode and continue execution.
> BUG_ON() would be too much and WARN_ON() would lead to data corruption as the execution
> would go on...
> Something like ext4's errors=remount-ro. :)

If you are willing to do the work, and if it is needed, and actually
tested, sure.

Re: The future of ubi_assert()

[Richard Weinberger]

Am 06.11.2014 um 09:13 schrieb Artem Bityutskiy:
> On Thu, 2014-11-06 at 09:07 +0100, Richard Weinberger wrote:
>> I think we can do better.
>> Think of wl.c, if an ubi_assert() in wl.c does not hold, we can put the UBI device
>> into read-only mode and continue execution.
>> BUG_ON() would be too much and WARN_ON() would lead to data corruption as the execution
>> would go on...
>> Something like ext4's errors=remount-ro. :)
> 
> If you are willing to do the work, and if it is needed, and actually
> tested, sure.

Of course I will.
During fastmap development I ran into basically every ubi_assert() and
had to find out whether it is useful or not.^^

Thanks,
//richard