From: Manivannan Sadhasivam <mani@kernel.org>
To: Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: Qiang Yu <quic_qianyu@quicinc.com>,
loic.poulain@linaro.org, mhi@lists.linux.dev,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
quic_cang@quicinc.com, mrana@quicinc.com
Subject: Re: [PATCH v4] bus: mhi: host: Disable preemption while processing data events
Date: Wed, 28 Dec 2022 22:05:26 +0530 [thread overview]
Message-ID: <20221228163526.GF30143@thinkpad> (raw)
In-Reply-To: <35752702-1ae2-126f-9237-a2f24c3bc3de@quicinc.com>
On Mon, Nov 21, 2022 at 10:48:54PM -0700, Jeffrey Hugo wrote:
> 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@quicinc.com>
>
> 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.
>
This sounds like a plausible solution.
> 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.
>
I do agree with you.
Qiang, can you please look into Jeff's suggestion on fixing this performance
issue?
Thanks,
Mani
> > ---
> > 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)
>
--
மணிவண்ணன் சதாசிவம்
next prev parent reply other threads:[~2022-12-28 16:35 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-21 9:34 [PATCH v4] bus: mhi: host: Disable preemption while processing data events Qiang Yu
2022-11-22 5:48 ` Jeffrey Hugo
2022-12-28 16:35 ` Manivannan Sadhasivam [this message]
2022-12-30 6:18 ` Qiang Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221228163526.GF30143@thinkpad \
--to=mani@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=loic.poulain@linaro.org \
--cc=mhi@lists.linux.dev \
--cc=mrana@quicinc.com \
--cc=quic_cang@quicinc.com \
--cc=quic_jhugo@quicinc.com \
--cc=quic_qianyu@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).