* [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