On Tue, Jan 02, 2024 at 10:19:47AM +0800, Aiqun Yu (Maria) wrote:

On 12/29/2023 6:20 AM, Matthew Wilcox wrote:

quoted

On Wed, Dec 13, 2023 at 12:27:05PM -0600, Eric W. Biederman wrote:

quoted

Matthew Wilcox [off-list ref] writes:

quoted

I think the right way to fix this is to pass a boolean flag to
queued_write_lock_slowpath() to let it know whether it can re-enable
interrupts while checking whether _QW_WAITING is set.

Yes.  It seems to make sense to distinguish between write_lock_irq and
write_lock_irqsave and fix this for all of write_lock_irq.

I wasn't planning on doing anything here, but Hillf kind of pushed me into
it.  I think it needs to be something like this.  Compile tested only.
If it ends up getting used,

Happy new year!

Thank you!  I know your new year is a few weeks away still ;-)

quoted

-void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock)
+void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock, bool irq)
  {
  	int cnts;

@@ -82,7 +83,11 @@ void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock)

Also a new state showed up after the current design:
1. locked flag with _QW_WAITING, while irq enabled.
2. And this state will be only in interrupt context.
3. lock->wait_lock is hold by the write waiter.
So per my understanding, a different behavior also needed to be done in
queued_write_lock_slowpath:
  when (unlikely(in_interrupt())) , get the lock directly.

I don't think so.  Remember that write_lock_irq() can only be called in
process context, and when interrupts are enabled.

So needed to be done in release path. This is to address Hillf's concern on
possibility of deadlock.

Hillf's concern is invalid.

quoted

  	/* When no more readers or writers, set the locked flag */
  	do {
+		if (irq)
+			local_irq_enable();

I think write_lock_irqsave also needs to be take account. So
loal_irq_save(flags) should be take into account here.

If we did want to support the same kind of spinning with interrupts
enabled for write_lock_irqsave(), we'd want to pass the flags in
and do local_irq_restore(), but I don't know how we'd support
write_lock_irq() if we did that -- can we rely on passing in 0 for flags
meaning "reenable" on all architectures?  And ~0 meaning "don't
reenable" on all architectures?

That all seems complicated, so I didn't do that.