On 30/05/2018 15:28, Sebastian Andrzej Siewior wrote:
On 2018-05-30 15:08:37 [+0100], John Garry wrote:
On 30/05/2018 14:45, Sebastian Andrzej Siewior wrote:
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.
the patch does not change the locking as it exists today.
Right
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?
I don't plan to add lockdep_assert_irqs_disabled(). You should only need
to use lockdep_assert_held() to verify that the lock is held as
expected. Whether or not the interrupts need to be disabled is best
verified with lockdep. Lockdep would even complain here if you intend to
invoke spin_unlock() on a lock which was not acquired earlier (I'm just
trying to say that lockdep_assert_held() is not required here either).
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).
I would like to get rid of that local_irq_save() here because it breaks
-RT and is not the correct thing to do for !RT (but it does not break
things).
I posted v1 with just the removal of the local_irq_sav() and v2 with
TODO removed as suggested here in the thread. The interrupts still
remain disabled.
So based on that it seems that you are in favour of the initial v1.
In fact, if this functional change was accepted then the "TODO" would
need to be updated for clarity since it would be less clear in meaning
when the local_irq_save() function is removed.
However, since !RT is not broken, then I am not sure if we accept
changes to fix the RT kernel only. You will need to defer to the James
and Martin for that.
Thanks,
John
John
Sebastian
.
