Subject: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure

Although is very unlikely, it's better to make sure we're not
letting this happen.

This solves this compilation warning:

kernel/irq_work.c: In function 'irq_work_run':
kernel/irq_work.c:148: warning: value computed is not used

Signed-off-by: Sergio Aguirre <[email protected]>
Cc: Huang Ying <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Kyle McMartin <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
kernel/irq_work.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index f16763f..5da635b 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -131,7 +131,7 @@ void irq_work_run(void)

list = xchg(head, NULL);
while (list != NULL) {
- struct irq_work *entry = list;
+ struct irq_work *entry = list, *xchgres;

list = irq_work_next(list);

@@ -145,7 +145,10 @@ void irq_work_run(void)
* Clear the BUSY bit and return to the free state if
* no-one else claimed it meanwhile.
*/
- cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY), NULL);
+ xchgres = cmpxchg(&entry->next,
+ next_flags(NULL, IRQ_WORK_BUSY),
+ NULL);
+ BUG_ON(unlikely(xchgres != next_flags(NULL, IRQ_WORK_BUSY)));
}
}
EXPORT_SYMBOL_GPL(irq_work_run);
--
1.7.0.4


2010-11-16 16:44:53

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure

On Tue, 2010-11-16 at 10:32 -0600, Sergio Aguirre wrote:
> Although is very unlikely, it's better to make sure we're not
> letting this happen.
>
> This solves this compilation warning:
>
> kernel/irq_work.c: In function 'irq_work_run':
> kernel/irq_work.c:148: warning: value computed is not used
>

> @@ -145,7 +145,10 @@ void irq_work_run(void)
> * Clear the BUSY bit and return to the free state if
> * no-one else claimed it meanwhile.
> */
> - cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY), NULL);
> + xchgres = cmpxchg(&entry->next,
> + next_flags(NULL, IRQ_WORK_BUSY),
> + NULL);
> + BUG_ON(unlikely(xchgres != next_flags(NULL, IRQ_WORK_BUSY)));

simply adding (void) in front would be much easier.

Subject: RE: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure

Hi Peter,

> -----Original Message-----
> From: Peter Zijlstra [mailto:[email protected]]
> Sent: Tuesday, November 16, 2010 10:45 AM
> To: Aguirre, Sergio
> Cc: LKML; Huang Ying; Martin Schwidefsky; Ingo Molnar; Kyle McMartin
> Subject: Re: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure
>
> On Tue, 2010-11-16 at 10:32 -0600, Sergio Aguirre wrote:
> > Although is very unlikely, it's better to make sure we're not
> > letting this happen.
> >
> > This solves this compilation warning:
> >
> > kernel/irq_work.c: In function 'irq_work_run':
> > kernel/irq_work.c:148: warning: value computed is not used
> >
>
> > @@ -145,7 +145,10 @@ void irq_work_run(void)
> > * Clear the BUSY bit and return to the free state if
> > * no-one else claimed it meanwhile.
> > */
> > - cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY), NULL);
> > + xchgres = cmpxchg(&entry->next,
> > + next_flags(NULL, IRQ_WORK_BUSY),
> > + NULL);
> > + BUG_ON(unlikely(xchgres != next_flags(NULL, IRQ_WORK_BUSY)));
>
> simply adding (void) in front would be much easier.

But isn't that still leaving the remote possibility of a hidden cmpxchg
Failure open?

Regards,
Sergio

2010-11-16 17:08:40

by Peter Zijlstra

[permalink] [raw]
Subject: RE: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure

On Tue, 2010-11-16 at 10:57 -0600, Aguirre, Sergio wrote:

> > > @@ -145,7 +145,10 @@ void irq_work_run(void)
> > > * Clear the BUSY bit and return to the free state if
> > > * no-one else claimed it meanwhile.
> > > */
> > > - cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY), NULL);
> > > + xchgres = cmpxchg(&entry->next,
> > > + next_flags(NULL, IRQ_WORK_BUSY),
> > > + NULL);
> > > + BUG_ON(unlikely(xchgres != next_flags(NULL, IRQ_WORK_BUSY)));
> >
> > simply adding (void) in front would be much easier.
>
> But isn't that still leaving the remote possibility of a hidden cmpxchg
> Failure open?

No, we don't care if it fails, read the comment. All we want to know is
that if it still matched, we flipped the bit.

Subject: RE: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure



> -----Original Message-----
> From: Peter Zijlstra [mailto:[email protected]]
> Sent: Tuesday, November 16, 2010 11:09 AM
> To: Aguirre, Sergio
> Cc: LKML; Huang Ying; Martin Schwidefsky; Ingo Molnar; Kyle McMartin
> Subject: RE: [RFC][PATCH] irq_work: Don't ignore possible cmpxchg failure
>
> On Tue, 2010-11-16 at 10:57 -0600, Aguirre, Sergio wrote:
>
> > > > @@ -145,7 +145,10 @@ void irq_work_run(void)
> > > > * Clear the BUSY bit and return to the free state if
> > > > * no-one else claimed it meanwhile.
> > > > */
> > > > - cmpxchg(&entry->next, next_flags(NULL, IRQ_WORK_BUSY),
> NULL);
> > > > + xchgres = cmpxchg(&entry->next,
> > > > + next_flags(NULL, IRQ_WORK_BUSY),
> > > > + NULL);
> > > > + BUG_ON(unlikely(xchgres != next_flags(NULL,
> IRQ_WORK_BUSY)));
> > >
> > > simply adding (void) in front would be much easier.
> >
> > But isn't that still leaving the remote possibility of a hidden cmpxchg
> > Failure open?
>
> No, we don't care if it fails, read the comment. All we want to know is
> that if it still matched, we flipped the bit.

I understand. Will add just a (void) typecast, and resend then.

Regards,
Sergio