lkml.org 
[lkml]   [2012]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 25/25] ipc: don't use [delayed_]work_pending()
    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@kernel.org> 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.
    --


    \
     
     \ /
      Last update: 2012-12-22 12:21    [W:4.322 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site