* [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking @ 2010-02-11 21:54 Deepak Chitriki 2010-02-11 22:20 ` Ameya Palande 0 siblings, 1 reply; 6+ messages in thread From: Deepak Chitriki @ 2010-02-11 21:54 UTC (permalink / raw) To: linux-omap Cc: Deepak Chitriki, Ameya Palande, Omar Ramirez Luna, Nishanth Menon Removed NTFY_Notify() in WMD_MSG_Get() to avoid locking contention as NTFY_Notify() is already invoked in InputMsg(). Cc: Ameya Palande <ameya.palande@nokia.com> Cc: Omar Ramirez Luna <omar.ramirez@ti.com> Cc: Nishanth Menon <nm@ti.com> Signed-off-by: Deepak Chitriki <deepak.chitriki@ti.com> --- Ref: v0:http://marc.info/?l=linux-omap&m=126584779011493&w=2 v1:http://marc.info/?l=linux-omap&m=126586224627725&w=2 v2:http://marc.info/?l=linux-omap&m=126591341909124&w=2 v3:http://marc.info/?l=linux-omap&m=126591743315572&w=2 v4:Comment fixes from v3 drivers/dsp/bridge/wmd/msg_sm.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/dsp/bridge/wmd/msg_sm.c b/drivers/dsp/bridge/wmd/msg_sm.c index 3a275f1..8475e20 100644 --- a/drivers/dsp/bridge/wmd/msg_sm.c +++ b/drivers/dsp/bridge/wmd/msg_sm.c @@ -295,11 +295,8 @@ DSP_STATUS WMD_MSG_Get(struct MSG_QUEUE *hMsgQueue, (struct list_head *)pMsgFrame); if (LST_IsEmpty(hMsgQueue->msgUsedList)) SYNC_ResetEvent(hMsgQueue->hSyncEvent); - else { - NTFY_Notify(hMsgQueue->hNtfy, - DSP_NODEMESSAGEREADY); + else SYNC_SetEvent(hMsgQueue->hSyncEvent); - } fGotMsg = true; } @@ -347,11 +344,8 @@ DSP_STATUS WMD_MSG_Get(struct MSG_QUEUE *hMsgQueue, } hMsgQueue->refCount--; /* Reset the event if there are still queued messages */ - if (!LST_IsEmpty(hMsgQueue->msgUsedList)) { - NTFY_Notify(hMsgQueue->hNtfy, - DSP_NODEMESSAGEREADY); + if (!LST_IsEmpty(hMsgQueue->msgUsedList)) SYNC_SetEvent(hMsgQueue->hSyncEvent); - } /* Exit critical section */ (void)SYNC_LeaveCS(hMsgMgr->hSyncCS); } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking 2010-02-11 21:54 [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking Deepak Chitriki @ 2010-02-11 22:20 ` Ameya Palande 2010-02-17 16:09 ` Menon, Nishanth 0 siblings, 1 reply; 6+ messages in thread From: Ameya Palande @ 2010-02-11 22:20 UTC (permalink / raw) To: ext Deepak Chitriki; +Cc: linux-omap, Omar Ramirez Luna, Nishanth Menon On Thu, 2010-02-11 at 22:54 +0100, ext Deepak Chitriki wrote: > Removed NTFY_Notify() in WMD_MSG_Get() to avoid locking contention > as NTFY_Notify() is already invoked in InputMsg(). > > Cc: Ameya Palande <ameya.palande@nokia.com> > Cc: Omar Ramirez Luna <omar.ramirez@ti.com> > Cc: Nishanth Menon <nm@ti.com> > > Signed-off-by: Deepak Chitriki <deepak.chitriki@ti.com> Acked-by: Ameya Palande <ameya.palande@nokia.com> > --- > Ref: > v0:http://marc.info/?l=linux-omap&m=126584779011493&w=2 > v1:http://marc.info/?l=linux-omap&m=126586224627725&w=2 > v2:http://marc.info/?l=linux-omap&m=126591341909124&w=2 > v3:http://marc.info/?l=linux-omap&m=126591743315572&w=2 > v4:Comment fixes from v3 > > drivers/dsp/bridge/wmd/msg_sm.c | 10 ++-------- > 1 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/dsp/bridge/wmd/msg_sm.c b/drivers/dsp/bridge/wmd/msg_sm.c > index 3a275f1..8475e20 100644 > --- a/drivers/dsp/bridge/wmd/msg_sm.c > +++ b/drivers/dsp/bridge/wmd/msg_sm.c > @@ -295,11 +295,8 @@ DSP_STATUS WMD_MSG_Get(struct MSG_QUEUE *hMsgQueue, > (struct list_head *)pMsgFrame); > if (LST_IsEmpty(hMsgQueue->msgUsedList)) > SYNC_ResetEvent(hMsgQueue->hSyncEvent); > - else { > - NTFY_Notify(hMsgQueue->hNtfy, > - DSP_NODEMESSAGEREADY); > + else > SYNC_SetEvent(hMsgQueue->hSyncEvent); > - } > > fGotMsg = true; > } > @@ -347,11 +344,8 @@ DSP_STATUS WMD_MSG_Get(struct MSG_QUEUE *hMsgQueue, > } > hMsgQueue->refCount--; > /* Reset the event if there are still queued messages */ > - if (!LST_IsEmpty(hMsgQueue->msgUsedList)) { > - NTFY_Notify(hMsgQueue->hNtfy, > - DSP_NODEMESSAGEREADY); > + if (!LST_IsEmpty(hMsgQueue->msgUsedList)) > SYNC_SetEvent(hMsgQueue->hSyncEvent); > - } > /* Exit critical section */ > (void)SYNC_LeaveCS(hMsgMgr->hSyncCS); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking 2010-02-11 22:20 ` Ameya Palande @ 2010-02-17 16:09 ` Menon, Nishanth 2010-02-18 18:26 ` Omar Ramirez Luna 0 siblings, 1 reply; 6+ messages in thread From: Menon, Nishanth @ 2010-02-17 16:09 UTC (permalink / raw) To: Ameya Palande, Chitriki Rudramuni, Deepak; +Cc: linux-omap, Ramirez Luna, Omar Hi, > From: Ameya Palande [mailto:ameya.palande@nokia.com] > Sent: Friday, February 12, 2010 12:21 AM > To: Chitriki Rudramuni, Deepak > Cc: linux-omap; Ramirez Luna, Omar; Menon, Nishanth > Subject: Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking > > On Thu, 2010-02-11 at 22:54 +0100, ext Deepak Chitriki wrote: > > Removed NTFY_Notify() in WMD_MSG_Get() to avoid locking contention > > as NTFY_Notify() is already invoked in InputMsg(). > > > > Cc: Ameya Palande <ameya.palande@nokia.com> > > Cc: Omar Ramirez Luna <omar.ramirez@ti.com> > > Cc: Nishanth Menon <nm@ti.com> > > > > Signed-off-by: Deepak Chitriki <deepak.chitriki@ti.com> > > Acked-by: Ameya Palande <ameya.palande@nokia.com> Could this patch be pushed if there are no more comments? > > > --- > > Ref: > > v0:http://marc.info/?l=linux-omap&m=126584779011493&w=2 > > v1:http://marc.info/?l=linux-omap&m=126586224627725&w=2 > > v2:http://marc.info/?l=linux-omap&m=126591341909124&w=2 > > v3:http://marc.info/?l=linux-omap&m=126591743315572&w=2 > > v4:Comment fixes from v3 > > > > drivers/dsp/bridge/wmd/msg_sm.c | 10 ++-------- > > 1 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/dsp/bridge/wmd/msg_sm.c > b/drivers/dsp/bridge/wmd/msg_sm.c > > index 3a275f1..8475e20 100644 > > --- a/drivers/dsp/bridge/wmd/msg_sm.c > > +++ b/drivers/dsp/bridge/wmd/msg_sm.c > > @@ -295,11 +295,8 @@ DSP_STATUS WMD_MSG_Get(struct MSG_QUEUE *hMsgQueue, > > (struct list_head *)pMsgFrame); > > if (LST_IsEmpty(hMsgQueue->msgUsedList)) > > SYNC_ResetEvent(hMsgQueue->hSyncEvent); > > - else { > > - NTFY_Notify(hMsgQueue->hNtfy, > > - DSP_NODEMESSAGEREADY); > > + else > > SYNC_SetEvent(hMsgQueue->hSyncEvent); > > - } > > > > fGotMsg = true; > > } > > @@ -347,11 +344,8 @@ DSP_STATUS WMD_MSG_Get(struct MSG_QUEUE *hMsgQueue, > > } > > hMsgQueue->refCount--; > > /* Reset the event if there are still queued messages */ > > - if (!LST_IsEmpty(hMsgQueue->msgUsedList)) { > > - NTFY_Notify(hMsgQueue->hNtfy, > > - DSP_NODEMESSAGEREADY); > > + if (!LST_IsEmpty(hMsgQueue->msgUsedList)) > > SYNC_SetEvent(hMsgQueue->hSyncEvent); > > - } > > /* Exit critical section */ > > (void)SYNC_LeaveCS(hMsgMgr->hSyncCS); > > } Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking 2010-02-17 16:09 ` Menon, Nishanth @ 2010-02-18 18:26 ` Omar Ramirez Luna 2010-02-18 20:36 ` Menon, Nishanth 0 siblings, 1 reply; 6+ messages in thread From: Omar Ramirez Luna @ 2010-02-18 18:26 UTC (permalink / raw) To: Menon, Nishanth Cc: Ameya Palande, Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando On 2/17/2010 10:09 AM, Menon, Nishanth wrote: > Hi, > >> From: Ameya Palande [mailto:ameya.palande@nokia.com] >> Sent: Friday, February 12, 2010 12:21 AM >> To: Chitriki Rudramuni, Deepak >> Cc: linux-omap; Ramirez Luna, Omar; Menon, Nishanth >> Subject: Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking >> >> On Thu, 2010-02-11 at 22:54 +0100, ext Deepak Chitriki wrote: >>> Removed NTFY_Notify() in WMD_MSG_Get() to avoid locking contention >>> as NTFY_Notify() is already invoked in InputMsg(). >>> >>> Cc: Ameya Palande<ameya.palande@nokia.com> >>> Cc: Omar Ramirez Luna<omar.ramirez@ti.com> >>> Cc: Nishanth Menon<nm@ti.com> >>> >>> Signed-off-by: Deepak Chitriki<deepak.chitriki@ti.com> >> >> Acked-by: Ameya Palande<ameya.palande@nokia.com> > > Could this patch be pushed if there are no more comments? Instead of pushing this patch I'll be reverting this one: http://marc.info/?l=linux-omap&m=125694410627720&w=2 Explanation: After reviewing the scenario it seems that the patch "fixing a flood of messages usecase" doesn't seem to be the proper way to fix it. If the dsp sends 10 messages in a shot for the same client and bridge process them without interrupt, if the client is using DSPManager_WaitForEvents, InputMsg function will notify of all of them but "WaitForEvents" will only catch one notification, then client will call GetMsg ioctl and pick one message from the queue leaving 9 of them still there, whenever WaitForEvents is executed again there won't be a Notification if the dsp stopped sending messages to the same queue, so those 9 messages never will be retrieved because WaitForEvents will timeout. The fix was only to re-notify the client that a message was still available in its queue and that it needs to pick it up, all of this during GetMsg, however this seems to be triggering a kernel warning because of nested spinlocks called for separate objects. The reason to avoid pushing this patch is that it is leaving a part of the code introduced by the "flooding" fix. Please let me know if anyone has comments. - omar ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking 2010-02-18 18:26 ` Omar Ramirez Luna @ 2010-02-18 20:36 ` Menon, Nishanth 2010-02-19 20:02 ` Omar Ramirez Luna 0 siblings, 1 reply; 6+ messages in thread From: Menon, Nishanth @ 2010-02-18 20:36 UTC (permalink / raw) To: Ramirez Luna, Omar Cc: Ameya Palande, Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando > -----Original Message----- > From: Ramirez Luna, Omar > Sent: Thursday, February 18, 2010 8:27 PM > To: Menon, Nishanth > Cc: Ameya Palande; Chitriki Rudramuni, Deepak; linux-omap; Guzman Lugo, > Fernando > Subject: Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking > > On 2/17/2010 10:09 AM, Menon, Nishanth wrote: > > Hi, > > > >> From: Ameya Palande [mailto:ameya.palande@nokia.com] > >> Sent: Friday, February 12, 2010 12:21 AM > >> To: Chitriki Rudramuni, Deepak > >> Cc: linux-omap; Ramirez Luna, Omar; Menon, Nishanth > >> Subject: Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive > locking > >> > >> On Thu, 2010-02-11 at 22:54 +0100, ext Deepak Chitriki wrote: > >>> Removed NTFY_Notify() in WMD_MSG_Get() to avoid locking contention > >>> as NTFY_Notify() is already invoked in InputMsg(). > >>> > >>> Cc: Ameya Palande<ameya.palande@nokia.com> > >>> Cc: Omar Ramirez Luna<omar.ramirez@ti.com> > >>> Cc: Nishanth Menon<nm@ti.com> > >>> > >>> Signed-off-by: Deepak Chitriki<deepak.chitriki@ti.com> > >> > >> Acked-by: Ameya Palande<ameya.palande@nokia.com> > > > > Could this patch be pushed if there are no more comments? > > Instead of pushing this patch I'll be reverting this one: > > http://marc.info/?l=linux-omap&m=125694410627720&w=2 > > Explanation: > > After reviewing the scenario it seems that the patch "fixing a flood of > messages usecase" doesn't seem to be the proper way to fix it. > > If the dsp sends 10 messages in a shot for the same client and bridge > process them without interrupt, if the client is using > DSPManager_WaitForEvents, InputMsg function will notify of all of them > but "WaitForEvents" will only catch one notification, then client will > call GetMsg ioctl and pick one message from the queue leaving 9 of them > still there, whenever WaitForEvents is executed again there won't be a > Notification if the dsp stopped sending messages to the same queue, so > those 9 messages never will be retrieved because WaitForEvents will > timeout. > > The fix was only to re-notify the client that a message was still > available in its queue and that it needs to pick it up, all of this > during GetMsg, however this seems to be triggering a kernel warning > because of nested spinlocks called for separate objects. > > The reason to avoid pushing this patch is that it is leaving a part of > the code introduced by the "flooding" fix. > > Please let me know if anyone has comments. So, do we have a clean complete fix? Why not leave the partial fix which we have in place and incorporate the clean fix on top of that? I would prefer to have something here instead of nothing at all.. but that is just me. > > - omar Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking 2010-02-18 20:36 ` Menon, Nishanth @ 2010-02-19 20:02 ` Omar Ramirez Luna 0 siblings, 0 replies; 6+ messages in thread From: Omar Ramirez Luna @ 2010-02-19 20:02 UTC (permalink / raw) To: Menon, Nishanth Cc: Ameya Palande, Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando On 2/18/2010 2:36 PM, Menon, Nishanth wrote: > [...] >> >> Instead of pushing this patch I'll be reverting this one: >> >> http://marc.info/?l=linux-omap&m=125694410627720&w=2 >> >> Explanation: >> >> After reviewing the scenario it seems that the patch "fixing a flood of >> messages usecase" doesn't seem to be the proper way to fix it. >> >> If the dsp sends 10 messages in a shot for the same client and bridge >> process them without interrupt, if the client is using >> DSPManager_WaitForEvents, InputMsg function will notify of all of them >> but "WaitForEvents" will only catch one notification, then client will >> call GetMsg ioctl and pick one message from the queue leaving 9 of them >> still there, whenever WaitForEvents is executed again there won't be a >> Notification if the dsp stopped sending messages to the same queue, so >> those 9 messages never will be retrieved because WaitForEvents will >> timeout. >> >> The fix was only to re-notify the client that a message was still >> available in its queue and that it needs to pick it up, all of this >> during GetMsg, however this seems to be triggering a kernel warning >> because of nested spinlocks called for separate objects. >> >> The reason to avoid pushing this patch is that it is leaving a part of >> the code introduced by the "flooding" fix. >> >> Please let me know if anyone has comments. > > So, do we have a clean complete fix? Why not leave the partial fix which we have in place and incorporate the clean fix on top of that? I would prefer to have something here instead of nothing at all.. but that is just me. > I wrongly pushed Deepak's fix when updating dspbridge, when I revert the old patch it will remove the other line too. The partial fix is solving the kernel warning but is useless for the flooding of DSP messages, because the line left which is SYNC_SetEvent needs an actual event to be waiting, so right now it is not doing anything (in the original patch wasn't doing anything either), so before we forget that the line left there is useless I'm reverting the patch. Regards, Omar ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-19 20:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-11 21:54 [PATCH v4] DSPBRIDGE: Fix to avoid possible recursive locking Deepak Chitriki 2010-02-11 22:20 ` Ameya Palande 2010-02-17 16:09 ` Menon, Nishanth 2010-02-18 18:26 ` Omar Ramirez Luna 2010-02-18 20:36 ` Menon, Nishanth 2010-02-19 20:02 ` Omar Ramirez Luna
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox