| Message ID | 1669023264-84125-1-git-send-email-quic_qianyu@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v4] bus: mhi: host: Disable preemption while processing data events | expand |
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. 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)
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) >
On 12/29/2022 12:35 AM, Manivannan Sadhasivam wrote: > 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 Jeff's suggestion is reasonable. I have no reasons to insist that the patch should be accepted. Thanks, Qiang >>> --- >>> 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)
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)
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> --- 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(-)