Re: [PATCH 2/7] genalloc: selftest
+ +/* + * In case of failure of any of these tests, memory corruption is almost + * guarranteed; allowing the boot to continue means risking to corrupt + * also any filesystem/block device accessed write mode. + * Therefore, BUG_ON() is used, when testing. + */ + + I like the explanation; good background info on why something is implemented the way it is :-). Reviewed-by: Jay Freyensee
Re: [PATCH 2/7] genalloc: selftest
On 26/02/18 21:12, Matthew Wilcox wrote: [...] > panic() halts the kernel > BUG_ON() kills the thread > WARN_ON() just prints messages > > Now, if we're at boot time and we're still executing code from the init > thread, killing init is equivalent to halting the kernel. > > The question is, what is appropriate for test modules? I would say > WARN_ON is not appropriate because people ignore warnings. BUG_ON is > reasonable for development. panic() is probably not. Ok, so I can leave WARN_ON() in the libraries, and keep the more restrictive BUG_ON() for the self test, which is optional for both genalloc and pmalloc. > Also, calling BUG_ON while holding a lock is not a good idea; if anything > needs to acquire that lock to shut down in a reasonable fashion, it's > going to hang. > > And there's no need to do something like BUG_ON(!foo); foo->wibble = 1; > Dereferencing a NULL pointer already produces a nice informative splat. > In general, we assume other parts of the kernel are sane and if they pass > us a NULL pool, it's no good returning -EINVAL, we may as well just oops > and let somebody else debug it. Great, that makes the code even simpler. -- igor
Re: [PATCH 2/7] genalloc: selftest
On Mon, Feb 26, 2018 at 08:00:26PM +0200, Igor Stoppa wrote: > On 26/02/18 19:46, J Freyensee wrote: > > That's a good question. Based upon those articles, 'yes'. But it seems > > like a 'darned-if-you-do, darned-if-you-don't' question as couldn't you > > also corrupt a mounted filesystem by crashing the kernel, yes/no? > > The idea is to do it very early in the boot phase, before early init, > when the kernel has not gotten even close to any storage device. > > > If you really want a system crash, maybe just do a panic() like > > filesystems also use? > > ok, if that's a more acceptable way to halt the kernel, I do not mind. panic() halts the kernel BUG_ON() kills the thread WARN_ON() just prints messages Now, if we're at boot time and we're still executing code from the init thread, killing init is equivalent to halting the kernel. The question is, what is appropriate for test modules? I would say WARN_ON is not appropriate because people ignore warnings. BUG_ON is reasonable for development. panic() is probably not. Also, calling BUG_ON while holding a lock is not a good idea; if anything needs to acquire that lock to shut down in a reasonable fashion, it's going to hang. And there's no need to do something like BUG_ON(!foo); foo->wibble = 1; Dereferencing a NULL pointer already produces a nice informative splat. In general, we assume other parts of the kernel are sane and if they pass us a NULL pool, it's no good returning -EINVAL, we may as well just oops and let somebody else debug it.
Re: [PATCH 2/7] genalloc: selftest
On 26/02/18 19:46, J Freyensee wrote: > > > On 2/26/18 4:11 AM, Igor Stoppa wrote: >> >> On 24/02/18 00:42, J Freyensee wrote: + locations[action->location] = gen_pool_alloc(pool, action->size); + BUG_ON(!locations[action->location]); >>> Again, I'd think it through if you really want to use BUG_ON() or not: >>> >>> https://lwn.net/Articles/13183/ >>> https://lkml.org/lkml/2016/10/4/1 >> Is it acceptable to display only a WARNing, in case of risking damaging >> a mounted filesystem? > > That's a good question. Based upon those articles, 'yes'. But it seems > like a 'darned-if-you-do, darned-if-you-don't' question as couldn't you > also corrupt a mounted filesystem by crashing the kernel, yes/no? The idea is to do it very early in the boot phase, before early init, when the kernel has not gotten even close to any storage device. > If you really want a system crash, maybe just do a panic() like > filesystems also use? ok, if that's a more acceptable way to halt the kernel, I do not mind. -- igor
Re: [PATCH 2/7] genalloc: selftest
On 2/26/18 4:11 AM, Igor Stoppa wrote: On 24/02/18 00:42, J Freyensee wrote: + locations[action->location] = gen_pool_alloc(pool, action->size); + BUG_ON(!locations[action->location]); Again, I'd think it through if you really want to use BUG_ON() or not: https://lwn.net/Articles/13183/ https://lkml.org/lkml/2016/10/4/1 Is it acceptable to display only a WARNing, in case of risking damaging a mounted filesystem? That's a good question. Based upon those articles, 'yes'. But it seems like a 'darned-if-you-do, darned-if-you-don't' question as couldn't you also corrupt a mounted filesystem by crashing the kernel, yes/no? If you really want a system crash, maybe just do a panic() like filesystems also use? -- igor
Re: [PATCH 2/7] genalloc: selftest
On 24/02/18 00:42, J Freyensee wrote: > >> +locations[action->location] = gen_pool_alloc(pool, action->size); >> +BUG_ON(!locations[action->location]); > > Again, I'd think it through if you really want to use BUG_ON() or not: > > https://lwn.net/Articles/13183/ > https://lkml.org/lkml/2016/10/4/1 Is it acceptable to display only a WARNing, in case of risking damaging a mounted filesystem? -- igor
Re: [PATCH 2/7] genalloc: selftest
+ locations[action->location] = gen_pool_alloc(pool, action->size); + BUG_ON(!locations[action->location]); Again, I'd think it through if you really want to use BUG_ON() or not: https://lwn.net/Articles/13183/ https://lkml.org/lkml/2016/10/4/1 Thanks, Jay