Re: [PATCH net] ptp: vclock: use mutex to fix "sleep on atomic" bug
From: Richard Cochran <richardcochran@gmail.com>
Date: 2023-02-16 22:06:05
On Thu, Feb 16, 2023 at 03:30:51PM +0100, Íñigo Huguet wrote:
vclocks were using spinlocks to protect access to its timecounter and cyclecounter. Access to timecounter/cyclecounter is backed by the same driver callbacks that are used for non-virtual PHCs, but the usage of the spinlock imposes a new limitation that didn't exist previously: now they're called in atomic context so they mustn't sleep. Some drivers like sfc or ice may sleep on these callbacks, causing errors like "BUG: scheduling while atomic: ptp5/25223/0x00000002" Fix it replacing the vclock's spinlock by a mutex. It fix the mentioned bug and it doesn't introduce longer delays.
Thanks for taking this up...
I've tested synchronizing various different combinations of clocks: - vclock->sysclock - sysclock->vclock - vclock->vclock - hardware PHC in different NIC -> vclock - created 4 vclocks and launch 4 parallel phc2sys processes
Could you please try it with lockdep enabled?
quoted hunk ↗ jump to hunk
@@ -43,16 +43,16 @@ static void ptp_vclock_hash_del(struct ptp_vclock *vclock) static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) { struct ptp_vclock *vclock = info_to_vclock(ptp); - unsigned long flags; s64 adj; adj = (s64)scaled_ppm << PTP_VCLOCK_FADJ_SHIFT; adj = div_s64(adj, PTP_VCLOCK_FADJ_DENOMINATOR); - spin_lock_irqsave(&vclock->lock, flags); + if (mutex_lock_interruptible(&vclock->lock) < 0) + return -EINTR;
Nit: please drop the '< 0' from the test.
quoted hunk ↗ jump to hunk
@@ -281,9 +280,10 @@ ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index) if (vclock->clock->index != vclock_index) continue; - spin_lock_irqsave(&vclock->lock, flags); + if (mutex_lock_interruptible(&vclock->lock) < 0) + break;
This is the only one that I'm not sure about. The others are all called from user context. Clean lockdep run would help. Thanks, Richard