[PATCH RFC 0/2] hyperv: Use raw_spinlock_t when not sleepable

[PATCH RFC 0/2] hyperv: Use raw_spinlock_t when not sleepable

[Mitchell Levy via B4 Relay]

When compiled with PREEMPT_RT, spinlock_t is sleepable. While I did not
observe this causing any lockups on my system, it did cause warnings to
be emitted when compiled with lock debugging. This series fixes some
instances where spinlock_t is used in non-sleepable contexts, though it
almost certainly does not find every such instance.

An example of the warning raised by lockdep:
=============================
[BUG: Invalid wait context ]
6.5.0-rc1+ #16 Not tainted
-----------------------------
swapper/0/1 is trying to lock:
ffffa05a014d64c0 (&channel→sched_lock) {...}-{3:3}, at: vmbus_isr+0x179/0x320
other info that might help us debug this:
context-{2:2}
2 locks held by swapper/0/1:
 #0: ffffffff909a9c70 (misc_mtx){+.+.}-{4:4}, at: misc_register+0x34/0x180
 #1: ffffffff9079b4c8 (rcu_read_lock) {...}-{1:3}, at: rcu_lock_acquire+0x0/0x40
stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.5.0-rc1+ #16
Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 05/09/2022

Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
---
Mitchell Levy (2):
      Use raw_spinlock_t for vmbus_channel.sched_lock
      Use raw_spinlock_t in vmbus_requestor

 drivers/hv/channel.c                | 6 +++---
 drivers/hv/channel_mgmt.c           | 2 +-
 drivers/hv/vmbus_drv.c              | 4 ++--
 drivers/pci/controller/pci-hyperv.c | 6 +++---
 include/linux/hyperv.h              | 8 ++++----
 5 files changed, 13 insertions(+), 13 deletions(-)
---
base-commit: 14f9643dc90adea074a0ffb7a17d337eafc6a5cc
change-id: 20230807-b4-rt_preempt-fix-35a65c90c6c9

Best regards,
-- 
Mitchell Levy <levymitchell0@gmail.com>

[PATCH RFC 1/2] Use raw_spinlock_t for vmbus_channel.sched_lock

[Mitchell Levy via B4 Relay]

From: Mitchell Levy <levymitchell0@gmail.com>

In vmbus_drv.c, vmbus_channel.sched_lock is acquired while holding an
rcu_read_lock. Since this is not a sleepable context, change sched_lock
to be a raw_spinlock_t to avoid sleeping when PREEMPT_RT is enabled.
---
 drivers/hv/channel.c                | 4 ++--
 drivers/hv/channel_mgmt.c           | 2 +-
 drivers/hv/vmbus_drv.c              | 4 ++--
 drivers/pci/controller/pci-hyperv.c | 6 +++---
 include/linux/hyperv.h              | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 56f7e06c673e..00d73ec060ed 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -910,9 +910,9 @@ void vmbus_reset_channel_cb(struct vmbus_channel *channel)
 	tasklet_disable(&channel->callback_event);
 
 	/* See the inline comments in vmbus_chan_sched(). */
-	spin_lock_irqsave(&channel->sched_lock, flags);
+	raw_spin_lock_irqsave(&channel->sched_lock, flags);
 	channel->onchannel_callback = NULL;
-	spin_unlock_irqrestore(&channel->sched_lock, flags);
+	raw_spin_unlock_irqrestore(&channel->sched_lock, flags);
 
 	channel->sc_creation_callback = NULL;
 
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 2f4d09ce027a..7679e6a3a645 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -348,7 +348,7 @@ static struct vmbus_channel *alloc_channel(void)
 	if (!channel)
 		return NULL;
 
-	spin_lock_init(&channel->sched_lock);
+	raw_spin_lock_init(&channel->sched_lock);
 	init_completion(&channel->rescind_event);
 
 	INIT_LIST_HEAD(&channel->sc_list);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 67f95a29aeca..b865d00c46dc 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1257,7 +1257,7 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
 		 * sched_lock critical section.  See also the inline comments
 		 * in vmbus_reset_channel_cb().
 		 */
-		spin_lock(&channel->sched_lock);
+		raw_spin_lock(&channel->sched_lock);
 
 		callback_fn = channel->onchannel_callback;
 		if (unlikely(callback_fn == NULL))
@@ -1280,7 +1280,7 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
 		}
 
 sched_unlock:
-		spin_unlock(&channel->sched_lock);
+		raw_spin_unlock(&channel->sched_lock);
 sched_unlock_rcu:
 		rcu_read_unlock();
 	}
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 2d93d0c4f10d..9ede3be05782 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1995,13 +1995,13 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 		 * sched_lock critical section.  See also the inline comments
 		 * in vmbus_reset_channel_cb().
 		 */
-		spin_lock_irqsave(&channel->sched_lock, flags);
+		raw_spin_lock_irqsave(&channel->sched_lock, flags);
 		if (unlikely(channel->onchannel_callback == NULL)) {
-			spin_unlock_irqrestore(&channel->sched_lock, flags);
+			raw_spin_unlock_irqrestore(&channel->sched_lock, flags);
 			goto enable_tasklet;
 		}
 		hv_pci_onchannelcallback(hbus);
-		spin_unlock_irqrestore(&channel->sched_lock, flags);
+		raw_spin_unlock_irqrestore(&channel->sched_lock, flags);
 
 		udelay(100);
 	}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 3ac3974b3c78..56a1fb1647a4 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -873,7 +873,7 @@ struct vmbus_channel {
 	 * Synchronize channel scheduling and channel removal; see the inline
 	 * comments in vmbus_chan_sched() and vmbus_reset_channel_cb().
 	 */
-	spinlock_t sched_lock;
+	raw_spinlock_t sched_lock;
 
 	/*
 	 * A channel can be marked for one of three modes of reading:

-- 
2.25.1

[PATCH RFC 2/2] Use raw_spinlock_t in vmbus_requestor

[Mitchell Levy via B4 Relay]

From: Mitchell Levy <levymitchell0@gmail.com>

Because hv_pci_onchannelcallback is called with irq disabled in
hv_compose_msi_msg, any locks held in that function must not be
sleepable. Therefore, change vmbus_requestor to use raw_spinlock_t.
---
 drivers/hv/channel.c   | 2 +-
 include/linux/hyperv.h | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 00d73ec060ed..08e6bd4a056d 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -635,7 +635,7 @@ static int vmbus_alloc_requestor(struct vmbus_requestor *rqstor, u32 size)
 	rqstor->req_bitmap = bitmap;
 	rqstor->size = size;
 	rqstor->next_request_id = 0;
-	spin_lock_init(&rqstor->req_lock);
+	raw_spin_lock_init(&rqstor->req_lock);
 
 	return 0;
 }
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 56a1fb1647a4..664c03a78e11 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -787,7 +787,7 @@ struct vmbus_requestor {
 	unsigned long *req_bitmap; /* is a given slot available? */
 	u32 size;
 	u64 next_request_id;
-	spinlock_t req_lock; /* provides atomicity */
+	raw_spinlock_t req_lock; /* provides atomicity */
 };
 
 #define VMBUS_NO_RQSTOR U64_MAX
@@ -1050,7 +1050,7 @@ struct vmbus_channel {
 do {									\
 	struct vmbus_requestor *rqstor = &(channel)->requestor;		\
 									\
-	spin_lock_irqsave(&rqstor->req_lock, flags);			\
+	raw_spin_lock_irqsave(&rqstor->req_lock, flags);		\
 } while (0)
 
 static __always_inline void unlock_requestor(struct vmbus_channel *channel,
@@ -1058,7 +1058,7 @@ static __always_inline void unlock_requestor(struct vmbus_channel *channel,
 {
 	struct vmbus_requestor *rqstor = &channel->requestor;
 
-	spin_unlock_irqrestore(&rqstor->req_lock, flags);
+	raw_spin_unlock_irqrestore(&rqstor->req_lock, flags);
 }
 
 u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr);

-- 
2.25.1

RE: [PATCH RFC 0/2] hyperv: Use raw_spinlock_t when not sleepable

[Michael Kelley (LINUX)]

From: Mitchell Levy via B4 Relay <devnull+levymitchell0.gmail.com@kernel.org> Sent: Wednesday, August 9, 2023 3:37 PM
> 
> When compiled with PREEMPT_RT, spinlock_t is sleepable. While I did not
> observe this causing any lockups on my system, it did cause warnings to
> be emitted when compiled with lock debugging. This series fixes some
> instances where spinlock_t is used in non-sleepable contexts, though it
> almost certainly does not find every such instance.
> 
> An example of the warning raised by lockdep:
> =============================
> [BUG: Invalid wait context ]
> 6.5.0-rc1+ #16 Not tainted
> -----------------------------
> swapper/0/1 is trying to lock:
> ffffa05a014d64c0 (&channel→sched_lock) {...}-{3:3}, at: vmbus_isr+0x179/0x320
> other info that might help us debug this:
> context-{2:2}
> 2 locks held by swapper/0/1:
>  #0: ffffffff909a9c70 (misc_mtx){+.+.}-{4:4}, at: misc_register+0x34/0x180
>  #1: ffffffff9079b4c8 (rcu_read_lock) {...}-{1:3}, at: rcu_lock_acquire+0x0/0x40
> stack backtrace:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.5.0-rc1+ #16
> Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V
> UEFI Release v4.1 05/09/2022
> 
> Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>

Getting the Hyper-V specific Linux guest code being able to work correctly
with PREEMPT_RT is an interesting requirement that we obviously haven't
dealt with yet.  Unfortunately, converting the channel->sched_lock to
a raw spin lock doesn't fully solve the problem.  Once that conversion is
done, there are multiple places where kmalloc() or a relative could be
called with GFP_ATOMIC while holding the sched_lock.  While such
allocations are allowed in a normal kernel, they are not allowed in a 
PREEMPT_RT kernel.

One such place is in hv_compose_msi_msg(), where
hv_pci_onchannelcallback() is invoked while holding the sched_lock.
hv_pci_onchannelcallback() does memory allocations.

Another place is in vmbus_chan_sched() where the onchannel_callback
function is directly invoked while holding the lock if HV_CALL_ISR is set.
HV_CALL_ISR is set for the uio_hv_generic.c driver, and for the netvsc driver.
I didn't follow all the code paths in the netvsc driver, but I suspect there
are places where the netvsc driver callback function does memory 
allocations.  I did look at the hv_uio_generic.c driver, and I'm pretty
sure a memory allocation could be done by uio_event_notify() when
sending a signal to the user space process.

Unfortunately, none of these places have easy fixes for the memory
allocation requirement.  Getting the Hyper-V specific code to work
with PREEMPT_RT will likely require some non-trivial redesign.

Given these limitations, I'm not sure if making this change is
worthwhile.   I'm not 100% clear on your goals.  If it is simply to
eliminate the lockdep warnings, then perhaps there's an
argument to be made in favor of the changes.  I'm open to
further discussion on the topic.

Michael

> ---
> Mitchell Levy (2):
>       Use raw_spinlock_t for vmbus_channel.sched_lock
>       Use raw_spinlock_t in vmbus_requestor
> 
>  drivers/hv/channel.c                | 6 +++---
>  drivers/hv/channel_mgmt.c           | 2 +-
>  drivers/hv/vmbus_drv.c              | 4 ++--
>  drivers/pci/controller/pci-hyperv.c | 6 +++---
>  include/linux/hyperv.h              | 8 ++++----
>  5 files changed, 13 insertions(+), 13 deletions(-)
> ---
> base-commit: 14f9643dc90adea074a0ffb7a17d337eafc6a5cc
> change-id: 20230807-b4-rt_preempt-fix-35a65c90c6c9
> 
> Best regards,
> --
> Mitchell Levy <levymitchell0@gmail.com>