| Message ID | 20210701125046.43018-1-wangborong@cdjrlc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sched: Use WARN_ON | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (086d9878e1092e7e69a69676ee9ec792690abb1d) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Le 01/07/2021 à 14:50, Jason Wang a écrit : > The BUG_ON macro simplifies the if condition followed by BUG, but it > will lead to the kernel crashing. Therefore, we can try using WARN_ON > instead of if condition followed by BUG. But are you sure it is ok to continue if spu_acquire(ctx) returned false ? Shouldn't there be at least for fallback handling ? Something like: if (WARN_ON(spu_acquire(ctx))) return; Christophe > > Signed-off-by: Jason Wang <wangborong@cdjrlc.com> > --- > arch/powerpc/platforms/cell/spufs/sched.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/platforms/cell/spufs/sched.c b/arch/powerpc/platforms/cell/spufs/sched.c > index 369206489895..0f218d9e5733 100644 > --- a/arch/powerpc/platforms/cell/spufs/sched.c > +++ b/arch/powerpc/platforms/cell/spufs/sched.c > @@ -904,8 +904,8 @@ static noinline void spusched_tick(struct spu_context *ctx) > struct spu_context *new = NULL; > struct spu *spu = NULL; > > - if (spu_acquire(ctx)) > - BUG(); /* a kernel thread never has signals pending */ > + /* a kernel thread never has signals pending */ > + WARN_ON(spu_acquire(ctx)); > > if (ctx->state != SPU_STATE_RUNNABLE) > goto out; >
On Thu, Jul 1, 2021 at 2:57 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 01/07/2021 à 14:50, Jason Wang a écrit : > > The BUG_ON macro simplifies the if condition followed by BUG, but it > > will lead to the kernel crashing. Therefore, we can try using WARN_ON > > instead of if condition followed by BUG. > > But are you sure it is ok to continue if spu_acquire(ctx) returned false ? > Shouldn't there be at least for fallback handling ? > > Something like: > > if (WARN_ON(spu_acquire(ctx))) > return; I think you get a crash in either case: - with the existing BUG_ON() there is an immediate backtrace and it stops there - with WARN_ON() and continuing, you operate on a context that is not valid - with the 'return', you get an endless loop, as it keeps calling spusched_tick() without sleeping. Out of those options, the existing BUG_ON() seems best. Arnd
diff --git a/arch/powerpc/platforms/cell/spufs/sched.c b/arch/powerpc/platforms/cell/spufs/sched.c index 369206489895..0f218d9e5733 100644 --- a/arch/powerpc/platforms/cell/spufs/sched.c +++ b/arch/powerpc/platforms/cell/spufs/sched.c @@ -904,8 +904,8 @@ static noinline void spusched_tick(struct spu_context *ctx) struct spu_context *new = NULL; struct spu *spu = NULL; - if (spu_acquire(ctx)) - BUG(); /* a kernel thread never has signals pending */ + /* a kernel thread never has signals pending */ + WARN_ON(spu_acquire(ctx)); if (ctx->state != SPU_STATE_RUNNABLE) goto out;
The BUG_ON macro simplifies the if condition followed by BUG, but it will lead to the kernel crashing. Therefore, we can try using WARN_ON instead of if condition followed by BUG. Signed-off-by: Jason Wang <wangborong@cdjrlc.com> --- arch/powerpc/platforms/cell/spufs/sched.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)