Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

From: Borislav Petkov
Date: Sat Dec 22 2012 - 06:09:35 EST

On Fri, Dec 21, 2012 at 06:22:10PM -0800, Tejun Heo wrote:
> Hello, Andrew.
> On Fri, Dec 21, 2012 at 06:15:23PM -0800, Andrew Morton wrote:
> > On Fri, 21 Dec 2012 17:57:15 -0800 Tejun Heo <tj@xxxxxxxxxx> wrote:
> >
> > > There's no need to test whether a (delayed) work item in pending
> > > before queueing, flushing or cancelling it. Most uses are unnecessary
> > > and quite a few of them are buggy.
> >
> > > - if (!work_pending(&ipc_memory_wq))
> > > - schedule_work(&ipc_memory_wq);
> > > + schedule_work(&ipc_memory_wq);
> >
> > Well, the new code is a ton slower than the old code if the work is
> > frequently pending, so some care is needed with such a conversion.
> Yeah, I mentioned it in the head message. it comes down to
> test_and_set_bit() vs. test_bit() and none of the current users seems
> to be hot enough for that to matter at all.
> In very hot paths, such optimization *could* be valid. The problem is
> that [delayed_]work_pending() seem to be abused much more than they
> are put to any actual usefulness. Maybe we should rename them to
> something really ugly. I don't know.

Hmm, we're also disabling local interrupts for no reason, if there's no
work pending (this is queue_work_on()):

2d1a: 9c pushfq
2d1b: 41 5c pop %r12
2d1d: fa cli
2d1e: e8 00 00 00 00 callq 2d23 <queue_work_on+0x33>
2d23: f0 0f ba 2b 00 lock btsl $0x0,(%rbx)

so there's IRQ disable + locked operation in schedule_work vs a simple
test_bit which doesn't even require the LOCK prefix.

Now you say those paths are not fast paths, but the reverse of
this optimization is also true: what happens if people start using
schedule_work() in fast paths without checking whether work is pending?
A useless IRQ disable + locked operation + IRQ enable.

I don't know but this could hurt in some situations, I'm thinking of RT
folk especially here.



Sent from a fat crate under my desk. Formatting is fine.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at