Avoid holding locks across sync IPCs in command buffer

Avoid holding locks across sync IPCs in command buffer

Pepper drops its global lock and reacquires it during synchronous IPCs, so these can turn into deadlocks.

BUG=418651
Committed: https://crrev.com/51e555de7077edc9e5f060860807bd71bbc14cd4
Cr-Commit-Position: refs/heads/master@{#309466}

[Peng, 2014-12-22 19:23:27]

penghuang@chromium.org changed reviewers:
+ piman@chromium.org

[Peng, 2014-12-22 19:26:10]

On 2014/12/22 19:23:27, Peng wrote:
> mailto:penghuang@chromium.org changed reviewers:
> + mailto:piman@chromium.org

Hi Antoine,

I updated the CL https://codereview.chromium.org/613843003/ to only include
changes in gpu/command_buffer/client/program_info_manager.cc.  Do you think it
is OK to land this part changes?

Thanks.

[piman, 2014-12-22 19:34:07]

LGTM, but it sounds like you're just masking the issue. It should really be
fixed at the source, in pepper.

[Peng, 2014-12-22 20:50:51]

On 2014/12/22 19:34:07, piman (Very slow to review) wrote:
> LGTM, but it sounds like you're just masking the issue. It should really be
> fixed at the source, in pepper.

We tried fix it by not releasing the global proxy lock during the sync IPC for
Pepper GL, but it causes other kind of dead locks. If the Pepper GL do not
release global proxy, the Pepper Socket API may acquire the proxy lock in the
plugin IO thread. It will block the plugin IO thread, so the Pepper GL call will
not get the reply msg. So that CL has been reverted.

I know the shared_group.cc may cause deadlock too, but in short term, we can
only fix part of deadlock issue in Pepper GL.

[Peng, 2014-12-22 20:50:58]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2014-12-22 20:52:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796543007/1

[piman, 2014-12-22 20:55:05]

On Mon, Dec 22, 2014 at 12:50 PM, <penghuang@chromium.org> wrote:

> On 2014/12/22 19:34:07, piman (Very slow to review) wrote:
>
>> LGTM, but it sounds like you're just masking the issue. It should really
>> be
>> fixed at the source, in pepper.
>>
>
> We tried fix it by not releasing the global proxy lock during the sync IPC
> for
> Pepper GL, but it causes other kind of dead locks.


These deadlocks need to be fixed.


> If the Pepper GL do not
> release global proxy, the Pepper Socket API may acquire the proxy lock in
> the
> plugin IO thread.


This is really wrong. The IO thread should never acquire long-lived locks.


> It will block the plugin IO thread, so the Pepper GL call will
> not get the reply msg. So that CL has been reverted.
>
> I know the shared_group.cc may cause deadlock too, but in short term, we
> can
> only fix part of deadlock issue in Pepper GL.
>
> https://codereview.chromium.org/796543007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2014-12-22 21:07:11]

penghuang@chromium.org changed reviewers:
+ dmichael@chromium.org

[Peng, 2014-12-22 21:07:35]

On 2014/12/22 20:55:05, piman (Very slow to review) wrote:
> On Mon, Dec 22, 2014 at 12:50 PM, <mailto:penghuang@chromium.org> wrote:
> 
> > On 2014/12/22 19:34:07, piman (Very slow to review) wrote:
> >
> >> LGTM, but it sounds like you're just masking the issue. It should really
> >> be
> >> fixed at the source, in pepper.
> >>
> >
> > We tried fix it by not releasing the global proxy lock during the sync IPC
> > for
> > Pepper GL, but it causes other kind of dead locks.
> 
> 
> These deadlocks need to be fixed.
> 
> 
> > If the Pepper GL do not
> > release global proxy, the Pepper Socket API may acquire the proxy lock in
> > the
> > plugin IO thread.
> 
> 
> This is really wrong. The IO thread should never acquire long-lived locks.

I don't know the detail. I think +dmicheal should know more.

[commit-bot: I haz the power, 2014-12-22 21:47:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-22 21:48:11]

Patchset 1 (id:??) landed as
https://crrev.com/51e555de7077edc9e5f060860807bd71bbc14cd4
Cr-Commit-Position: refs/heads/master@{#309466}

[dmichael (off chromium), 2015-01-08 00:11:18]

On 2014/12/22 21:07:35, Peng wrote:
> On 2014/12/22 20:55:05, piman (Very slow to review) wrote:
> > On Mon, Dec 22, 2014 at 12:50 PM, <mailto:penghuang@chromium.org> wrote:
> > 
> > > On 2014/12/22 19:34:07, piman (Very slow to review) wrote:
> > >
> > >> LGTM, but it sounds like you're just masking the issue. It should really
> > >> be
> > >> fixed at the source, in pepper.
> > >>
> > >
> > > We tried fix it by not releasing the global proxy lock during the sync IPC
> > > for
> > > Pepper GL, but it causes other kind of dead locks.
> > 
> > 
> > These deadlocks need to be fixed.
> > 
> > 
> > > If the Pepper GL do not
> > > release global proxy, the Pepper Socket API may acquire the proxy lock in
> > > the
> > > plugin IO thread.
> > 
> > 
> > This is really wrong. The IO thread should never acquire long-lived locks.
> 
> I don't know the detail. I think +dmicheal should know more.

I agree completely with piman; it's wrong that we're locking on the proxy lock
on the IO thread. See the bug I have filed:
https://code.google.com/p/chromium/issues/detail?id=439588
It's easy to fix, but the easy fix will regress performance on Pepper UDP
sockets. This is one of the first things I plan to tackle now that I'm back from
holiday/vacation.