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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 1/2/2024 5:14 PM, Matthew Wilcox wrote:
On Tue, Jan 02, 2024 at 10:19:47AM +0800, Aiqun Yu (Maria) wrote:
On 12/29/2023 6:20 AM, Matthew Wilcox wrote:
On Wed, Dec 13, 2023 at 12:27:05PM -0600, Eric W. Biederman wrote:
Matthew Wilcox <willy@xxxxxxxxxxxxx> writes:
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 ;-)
Yeah, Chinese new year will come about 5 weeks later. :)

-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.
In current kernel drivers, I can see same lock called with write_lock_irq and write_lock_irqsave in different drivers.

And this is the scenario I am talking about:
1. cpu0 have task run and called write_lock_irq.(Not in interrupt context)
2. cpu0 hold the lock->wait_lock and re-enabled the interrupt.
* this is the new state with _QW_WAITING set, lock->wait_lock locked, interrupt enabled. *
3. cpu0 in-interrupt context and want to do write_lock_irqsave.
4. cpu0 tried to acquire lock->wait_lock again.

I was thinking to support both write_lock_irq and write_lock_irqsave with interrupt enabled together in queued_write_lock_slowpath.

That's why I am suggesting in write_lock_irqsave when (in_interrupt()), instead spin for the lock->wait_lock, spin to get the lock->cnts directly.

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

Hillf's concern is invalid.

   	/* 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?
What about for all write_lock_irq, pass the real flags from local_irq_save(flags) into the queued_write_lock_slowpath?
Arch specific valid flags won't be !0 limited then.

That all seems complicated, so I didn't do that.
This is complicated. Also need test verify to ensure.
More careful design more better.

Fixed previous wrong email address. ^-^!


--
Thx and BRs,
Aiqun(Maria) Yu




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux