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

[Borislav Petkov]

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.

Thanks.

-- 
Regards/Gruss,
    Boris.

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  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/