Re: [PATCH v4] bus: mhi: host: Disable preemption while processing data events

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

 



On 11/21/2022 2:34 AM, Qiang Yu wrote:
If data processing of an event is scheduled out because core
is busy handling multiple irqs, this can starve the processing
of MHI M0 state change event on another core. Fix this issue by
disabling irq on the core processing data events.

Signed-off-by: Qiang Yu <quic_qianyu@xxxxxxxxxxx>

I've been pondering this off and on since it's been proposed.

This solution will break the described deadlock, but I don't like it.

What I really don't like is that this is selfish. We already preempt anything else on the CPU that isn't a hard IRQ because we are using a tasklet (which is deprecated, see include/linux/interrupt.h). Now we are going to essentially preempt IRQs as well by preventing them from being serviced. So, now the CPU is essentially dedicated to processing MHI events. It seems selfish to say that MHI is the most important thing on a particular CPU.

This can have a huge effect on system behavior. If say the ssh IRQ is assigned to the same CPU, and we block that CPU long enough, then it will appear to the user as if the ssh connection has frozen. I've witnessed this occur with other drivers.

How long can we block the CPU? According to the code, pretty much for an unlimited amount of time. If the tasklet is processing mhi_process_data_event_ring(), then we can process U32_MAX events before throttling (which might as well be unlimited). If the tasklet is processing mhi_process_ctrl_ev_ring() then there is no throttling.

I'm thinking it would be better of the IRQ handling was refactored to use threaded interrupts. The thread is an actual process, so it could move to another CPU. It is also FIFO priority, so it basically will preempt everything but hard IRQs and soft IRQs (eg tasklets). The downside of a tasklet is that it is bound to the scheduling CPU, which in our case is the CPU servicing the IRQ, and more than a few systems tend to load the majority of the IRQs to CPU0.

I'm not going to go refactor the IRQ code at this time. This looks like an issue that is actually observed based on how it was reported, so it likely should be addressed. I'm not happy with this solution, but I don't have an alternative at this time.

Mani, up to you if you want to pick this up. I'm not nack'ing it. Technically I've reviewed it, but I'd say I'm "on the fence" about if this really should be accepted. I can't say there is a flaw in the logic, but I don't feel good about this.

---
v3->v4: modify the comment
v2->v3: modify the comment
v1->v2: add comments about why we disable local irq

  drivers/bus/mhi/host/main.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index f3aef77a..6c804c3 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -1029,11 +1029,17 @@ void mhi_ev_task(unsigned long data)
  {
  	struct mhi_event *mhi_event = (struct mhi_event *)data;
  	struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
+	unsigned long flags;
+ /*
+	 * When multiple IRQs arrive, the tasklet will be scheduled out with event ring lock
+	 * acquired, causing other high priority events like M0 state transition getting stuck
+	 * while trying to acquire the same event ring lock. Thus, let's disable local IRQs here.
+	 */
+	spin_lock_irqsave(&mhi_event->lock, flags);
  	/* process all pending events */
-	spin_lock_bh(&mhi_event->lock);
  	mhi_event->process_event(mhi_cntrl, mhi_event, U32_MAX);
-	spin_unlock_bh(&mhi_event->lock);
+	spin_unlock_irqrestore(&mhi_event->lock, flags);
  }
void mhi_ctrl_ev_task(unsigned long data)




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux