On 9/3/26 4:39 PM, Oleksii Kurochko wrote:
+ do { smp_rmb(); } while ( test_bit(_IRQ_INPROGRESS, &desc-
>status) );
I am thinking if a barrier is in correct place or needed at all.
Considering that desc->status is updated under spinlock() which uses
full barrier the result here should be already observable without
smp_rmb() inside do {} while ().
Probably we want to have load->load between test_bit() and a read of
action->free_on_release in if () below but I don't see what could go
wrong if this read will happen before do {} while ().
xvfree() (stores inside it) can't be executed ealier because of control
dependency [Rule 11: b (xfree) is a (action->free_on_release) store, and
b has a syntactic control dependency on a] so again it looks like a
barrier isn't needed here.
So considering what kind of barrier is used inside spinlock + Rule 11 we
can just move smp_rmb() after the cycle (just in case) and it looks like
smp_rmb() is only here just to force compiler not to order the things
considering how action->free_on_release is used:
static void irq_release_action(const struct irq_desc *desc,
struct irqaction *action)
{
/*
* Wait to make sure it's not being used on another CPU.
*
* desc->status is cleared in do_IRQ() under desc->lock, whose
* acquire/release barriers are a full smp_mb() on this arch, so the
* handler's writes are already ordered before the clear is visible.
* On this side, xvfree() is control-dependent on the final test_bit()
* load, so Rule 11 (RVWMO) already orders it after the wait with no
* barrier. smp_rmb() below adds real read->read ordering, but nothing
* after the loop depends on it (action->free_on_release isn't racy);
* it's kept as a guard against the compiler breaking the control
* dependency the ordering actually relies on.
*/
while ( test_bit(_IRQ_INPROGRESS, &desc->status) )
cpu_relax();
smp_rmb();
if ( action->free_on_release )
xvfree(action);
Am I missing something?
It could be option just to skip smp_rmb() here at all:
/*
* Wait for a handler still running on another CPU to complete:
do_IRQ()
* clears IRQ_INPROGRESS only after the handler has returned.
*
* No barrier is needed here: nothing below reads data written by the
* handler (action->free_on_release is set up once, before the
action is
* ever registered), and the stores done by xvfree() are ordered after
* the loop's load of desc->status by the control dependency alone
* (RVWMO ppo rule 11).
*/
while ( test_bit(_IRQ_INPROGRESS, &desc->status) )
cpu_relax();
if ( action->free_on_release )
xvfree(action);
But probably just to be sure that if ->free_on_release will one day
somewhere else set except the mentioned case it makes sense to have
smp_rmb() or even smp_mb() (depsite of the fact smp_rmb() looks more
then enough).
Does it make sense?
Please split this across three lines, to conform to style. (Also same nit
as above.)
+ if ( action->free_on_release )
+ xvfree(action);
