Received-SPF: pass (google.com: domain of linux-kernel+bounces-12529-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 28 Dec 2023 09:07:18 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Hillf Danton <hdanton@sina.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
	Maria Yu <quic_aiquny@quicinc.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kernel: Introduce a write lock/unlock wrapper for
 tasklist_lock
Message-ID: <ZY06xtvaQ4ZJ5dXa@casper.infradead.org>
References: <20231213101745.4526-1-quic_aiquny@quicinc.com>
 <ZXnaNSrtaWbS2ivU@casper.infradead.org>
 <20231226104652.1491-1-hdanton@sina.com>
 <20231227110727.1546-1-hdanton@sina.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231227110727.1546-1-hdanton@sina.com>

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));