Re: [PATCH] kernel: Introduce a write lock/unlock wrapper for tasklist_lock

[Matthew Wilcox]

On Wed, Dec 27, 2023 at 07:07:27PM +0800, Hillf Danton wrote:
> Feel free to ignore the following leg works.
> 
> 	/* Set the waiting flag to notify readers that a writer is pending */
> 	atomic_or(_QW_WAITING, &lock->cnts);
> 
> 	enable irq;
> 
> 	/* When no more readers or writers, set the locked flag */
> 	do {
> 		cnts = atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING);
> 	} while (!atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED));
> 
> 	int
> 	atomic_cond_read_acquire(&lock->cnts, !(VAL & _QW_LOCKED));
> 	deadlock
> 	disable irq;

That would be a buggy implementation, and would not be what I was
thinking.

> Though the case below is safe, it looks not pretty but clumsy.
> 
> 	/* Set the waiting flag to notify readers that a writer is pending */
> 	atomic_or(_QW_WAITING, &lock->cnts);
> 
> 	/* When no more readers or writers, set the locked flag */
> 	do {
> 		enable irq;
> 
> 		cnts = atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING);
> 
> 		disable irq;
> 
> 	} while (!atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED));

Why do you think it looks clumsy?  It's more or less what I was
thinking.

-void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock)
+void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock, bool irq)
...
        do {
+               if (irq)
+                       local_irq_enable();
                cnts = atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING);
+               if (irq)
+                       local_irq_disable();
        } while (!atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED));