xsukax on XFedi

@axboe that ‘spin_trylock_irq()’ is still potentially quite expensive in the failure case.

The trylock itself is probably fine, since it does

    int val = atomic_read(&lock->val);

    if (unlikely(val))
            return 0;

so it’s just a single read for the contended case (sill potentially causing cacheline bounces, of course, but not nearly as much as trying to get the lock). But the irq part means that we end up doing

    local_irq_disable(); \
    raw_spin_trylock(lock) ? \
    1 : ({ local_irq_enable(); 0;  }); \

so for the “I didn’t get the lock” case you end up with that extra “disable and re-enable interrupts”.

That’s all because we don’t make each spinlock variation make their own irq-safe version, so we wrap the trylock around the whole thing.

And that bfq code seems to make this worse by doing this whole thing twice, both for bfq_bic_try_lookup() and bfq_merge().

The locking around ioc_lookup_icq() in particular looks odd, in that the function itself claims to be RCU-safe, but then it also requires that q->queue_lock, making a shambles of the whole RCU thing.

Conversation