On 30/05/2018 14:45, Sebastian Andrzej Siewior wrote:
On 2018-05-30 14:37:50 [+0100], John Garry wrote:
On 30/05/2018 12:22, Sebastian Andrzej Siewior wrote:
Yes, there are the same on vanilla and not on RT. However my point is
that the code does this instead:
local_irq_save();
spin_unlock();
Ah, I just noticed that this is spin_unlock().
So about the "TODO", which you mention "I *assumed* that the intention was
to audit the code for this
spin_unlock_irq(ap->lock);
change instead. But if this is or was never intended than I could indeed
remove the TODO comment."
As I see, we're dropping the lock but maintaining the irq posture for
holding that lock (disabled), which seems inefficient.
excellent. So no more objections from your side or is this a complaint I
didn't fully decode?
I think the original code is not great since we're dropping the lock
but maintaining the irq posture as disabled.
Do you plan to add a lockdep_assert_irqs_disabled() check in addition?
It's not needed if we work on the basis that the lock is always taken
with irqs disabled. And is this lockdep function even intended to be
used outside core kernel code?
Personally I think that solving the issue in the original code would be
better, i.e. keeping irqs disabled needlessly. Or else the "TODO" can
stay (or be amended to improve clarity).
John
John
Sebastian
.
