ozone: drm: Add fd number to crash keys when dmabuf mmap fails.

ozone: drm: Add fd number to crash keys when dmabuf mmap fails.

When native GPU memory buffers were enabled we noticed many crashes
reporting mmap failing.

This CL adds fd number to crash report when dmabuf mmap fails, because we think
fd limit may cause this crash.
It's follow-up CL of https://codereview.chromium.org/2710183005/

BUG=629521
Review-Url: https://codereview.chromium.org/2725813005
Cr-Commit-Position: refs/heads/master@{#456154}
Committed: https://chromium.googlesource.com/chromium/src/+/7fdea0d7d730de0d5a34f6f1fb5f868fa8e40ece

[inactive_dshwang_plz_cc_intel, 2017-03-02 01:43:54]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 01:45:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2017-03-02 01:45:34]

dongseong.hwang@intel.com changed reviewers:
+ dcastagna@chromium.org, dnicoara@chromium.org, rsesek@chromium.org

[dshwang, 2017-03-02 01:45:35]

It's follow-up CL of https://codereview.chromium.org/2710183005/

dnicoara@: Please review changes in ui/ozone/
rsesek@: Please review changes in chrome/common/crash_keys.cc

[inactive_dshwang_plz_cc_intel, 2017-03-02 01:49:34]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 01:50:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniele Castagna, 2017-03-02 02:05:04]

https://codereview.chromium.org/2725813005/diff/20001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (left):

https://codereview.chromium.org/2725813005/diff/20001/chrome/common/crash_key...
chrome/common/crash_keys.cc:250: { "swdh_set_hosted_version_restart_count",
crash_keys::kSmallSize },
Last time I changed this I tried to avoid reformatting the file.

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:97:
base::ProcessMetrics::CreateCurrentProcessMetrics());
CreateCurrentProcessMetrics calls base::GetCurrentProcessHandle().
This won't probably work in the renderer since we're in a new pid namespace and
/proc is not remounted. We should access /proc/self instead of /proc/id.

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:99:
base::StringPrintf("%d", process_metrics->GetOpenFdCount());
This would report -1 in case we're out of fds since we won't be able to open
/proc/{PID}/fd. Is that what you intended?

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:112: ;
Why did you remove a useful log and just left a floating semi colon?

[commit-bot: I haz the power, 2017-03-02 03:29:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 03:29:02]

Dry run: This issue passed the CQ dry run.

[dshwang, 2017-03-02 19:59:32]

Thank you for review, could you review again?

https://codereview.chromium.org/2725813005/diff/20001/chrome/common/crash_key...
File chrome/common/crash_keys.cc (left):

https://codereview.chromium.org/2725813005/diff/20001/chrome/common/crash_key...
chrome/common/crash_keys.cc:250: { "swdh_set_hosted_version_restart_count",
crash_keys::kSmallSize },
On 2017/03/02 02:05:04, Daniele Castagna wrote:
> Last time I changed this I tried to avoid reformatting the file.

However, when I tried to avoid it, I could not submit the CL.
As you want to avoid it, I submitted the reformating CL first.
https://codereview.chromium.org/2721413004/

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:97:
base::ProcessMetrics::CreateCurrentProcessMetrics());
On 2017/03/02 02:05:04, Daniele Castagna wrote:
> CreateCurrentProcessMetrics calls base::GetCurrentProcessHandle().
> This won't probably work in the renderer since we're in a new pid namespace
and
> /proc is not remounted. We should access /proc/self instead of /proc/id.

That's good point. By the way, it's base::GetCurrentProcessHandle()
implamantation issue, rather than this CL. Furthermore, even if the impl uses
"/proc/self", renderer still cannot read because of sandbox filesystem
protection. As renderer has no way, renderer will has just "-1" for it.

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:99:
base::StringPrintf("%d", process_metrics->GetOpenFdCount());
On 2017/03/02 02:05:04, Daniele Castagna wrote:
> This would report -1 in case we're out of fds since we won't be able to open
> /proc/{PID}/fd. Is that what you intended?

OH, that's good point. I didn't think about it. Fortunately, it looks good. If
browser process report -1, we can think it reachs FD limit. so looks good to go.

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:112: ;
On 2017/03/02 02:05:04, Daniele Castagna wrote:
> Why did you remove a useful log and just left a floating semi colon?

Done.

[Daniele Castagna, 2017-03-02 20:08:52]

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:97:
base::ProcessMetrics::CreateCurrentProcessMetrics());
On 2017/03/02 at 19:59:31, dshwang wrote:
> On 2017/03/02 02:05:04, Daniele Castagna wrote:
> > CreateCurrentProcessMetrics calls base::GetCurrentProcessHandle().
> > This won't probably work in the renderer since we're in a new pid namespace
and
> > /proc is not remounted. We should access /proc/self instead of /proc/id.
> 
> That's good point. By the way, it's base::GetCurrentProcessHandle()
implamantation issue, rather than this CL. Furthermore, even if the impl uses
"/proc/self", renderer still cannot read because of sandbox filesystem
protection. As renderer has no way, renderer will has just "-1" for it.

Are you sure this works in the GPU process and the value you get is what you
expect?
Did you try to run this code even once in the browser/renderer/gpu process?

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:99:
base::StringPrintf("%d", process_metrics->GetOpenFdCount());
On 2017/03/02 at 19:59:31, dshwang wrote:
> On 2017/03/02 02:05:04, Daniele Castagna wrote:
> > This would report -1 in case we're out of fds since we won't be able to open
> > /proc/{PID}/fd. Is that what you intended?
> 
> OH, that's good point. I didn't think about it. Fortunately, it looks good. If
browser process report -1, we can think it reachs FD limit. so looks good to go.

We were thinking of addding a UMA stat for openfds. As rsesek suggested, we
could open the fd for /proc/self/fd early and keep it open. In that way we
wouldn't have the problem of trying to open a dir when we might have reached the
fd limit.

[dshwang, 2017-03-02 20:15:12]

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:97:
base::ProcessMetrics::CreateCurrentProcessMetrics());
On 2017/03/02 20:08:52, Daniele Castagna wrote:
> On 2017/03/02 at 19:59:31, dshwang wrote:
> > On 2017/03/02 02:05:04, Daniele Castagna wrote:
> > > CreateCurrentProcessMetrics calls base::GetCurrentProcessHandle().
> > > This won't probably work in the renderer since we're in a new pid
namespace
> and
> > > /proc is not remounted. We should access /proc/self instead of /proc/id.
> > 
> > That's good point. By the way, it's base::GetCurrentProcessHandle()
> implamantation issue, rather than this CL. Furthermore, even if the impl uses
> "/proc/self", renderer still cannot read because of sandbox filesystem
> protection. As renderer has no way, renderer will has just "-1" for it.
> 
> Are you sure this works in the GPU process and the value you get is what you
> expect?
> Did you try to run this code even once in the browser/renderer/gpu process?

I run this code in chromeos, and it works in only browser process. gpu process
never use this code, as this class is for client for gpu process.

https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:99:
base::StringPrintf("%d", process_metrics->GetOpenFdCount());
On 2017/03/02 20:08:52, Daniele Castagna wrote:
> On 2017/03/02 at 19:59:31, dshwang wrote:
> > On 2017/03/02 02:05:04, Daniele Castagna wrote:
> > > This would report -1 in case we're out of fds since we won't be able to
open
> > > /proc/{PID}/fd. Is that what you intended?
> > 
> > OH, that's good point. I didn't think about it. Fortunately, it looks good.
If
> browser process report -1, we can think it reachs FD limit. so looks good to
go.
> 
> We were thinking of addding a UMA stat for openfds. As rsesek suggested, we
> could open the fd for /proc/self/fd early and keep it open. In that way we
> wouldn't have the problem of trying to open a dir when we might have reached
the
> fd limit.

good to know. do you want to wait for UMA stat change?

[dshwang, 2017-03-07 23:07:09]

Description was changed from

==========
ozone: drm: Add fd number to crash keys when dmabuf mmap fails.

When native GPU memory buffers were enabled we noticed many crashes
reporting mmap failing.

This CL adds fd number to crash report when dmabuf mmap fails, because we think
fd limit may cause this crash.
It's follow-up CL of https://codereview.chromium.org/2710183005/

BUG=629521
==========

to

==========
ozone: drm: Add fd number to crash keys when dmabuf mmap fails.

When native GPU memory buffers were enabled we noticed many crashes
reporting mmap failing.

This CL adds fd number to crash report when dmabuf mmap fails, because we think
fd limit may cause this crash.
It's follow-up CL of https://codereview.chromium.org/2710183005/

BUG=629521
==========

[Daniele Castagna, 2017-03-09 03:58:06]

On 2017/03/02 at 20:15:12, dongseong.hwang wrote:
>
https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
> File ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc (right):
> 
>
https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
> ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:97:
base::ProcessMetrics::CreateCurrentProcessMetrics());
> On 2017/03/02 20:08:52, Daniele Castagna wrote:
> > On 2017/03/02 at 19:59:31, dshwang wrote:
> > > On 2017/03/02 02:05:04, Daniele Castagna wrote:
> > > > CreateCurrentProcessMetrics calls base::GetCurrentProcessHandle().
> > > > This won't probably work in the renderer since we're in a new pid
namespace
> > and
> > > > /proc is not remounted. We should access /proc/self instead of /proc/id.
> > > 
> > > That's good point. By the way, it's base::GetCurrentProcessHandle()
> > implamantation issue, rather than this CL. Furthermore, even if the impl
uses
> > "/proc/self", renderer still cannot read because of sandbox filesystem
> > protection. As renderer has no way, renderer will has just "-1" for it.
> > 
> > Are you sure this works in the GPU process and the value you get is what you
> > expect?
> > Did you try to run this code even once in the browser/renderer/gpu process?
> 
> I run this code in chromeos, and it works in only browser process. gpu process
never use this code, as this class is for client for gpu process.
> 
>
https://codereview.chromium.org/2725813005/diff/20001/ui/ozone/platform/drm/c...
> ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc:99:
base::StringPrintf("%d", process_metrics->GetOpenFdCount());
> On 2017/03/02 20:08:52, Daniele Castagna wrote:
> > On 2017/03/02 at 19:59:31, dshwang wrote:
> > > On 2017/03/02 02:05:04, Daniele Castagna wrote:
> > > > This would report -1 in case we're out of fds since we won't be able to
open
> > > > /proc/{PID}/fd. Is that what you intended?
> > > 
> > > OH, that's good point. I didn't think about it. Fortunately, it looks
good. If
> > browser process report -1, we can think it reachs FD limit. so looks good to
go.
> > 
> > We were thinking of addding a UMA stat for openfds. As rsesek suggested, we
> > could open the fd for /proc/self/fd early and keep it open. In that way we
> > wouldn't have the problem of trying to open a dir when we might have reached
the
> > fd limit.
> 
> good to know. do you want to wait for UMA stat change?


I think we can land this independently from crrev.com/2733313003
It's convenient to have the log too.

LGTM.

[dshwang, 2017-03-09 21:10:17]

On 2017/03/09 03:58:06, Daniele Castagna wrote:
> I think we can land this independently from crrev.com/2733313003
> It's convenient to have the log too.
> 
> LGTM.

Thank you for review.

Robert, could you review as owner? Thanks

[Robert Sesek, 2017-03-09 21:51:13]

lgtm

[dshwang, 2017-03-10 18:57:30]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2017-03-10 18:58:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:06:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:06:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dshwang, 2017-03-10 19:11:51]

dnicoara, could you review
ui/ozone/platform/drm/common/client_native_pixmap_dmabuf.cc? Thanks

[dnicoara, 2017-03-10 19:35:27]

ui/ozone LGTM

[dshwang, 2017-03-10 20:49:28]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2017-03-10 20:49:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 20:54:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489178968634850,
"parent_rev": "31ec638091fc05bd13c41f8cd19cae7479cd5a1c", "commit_rev":
"7fdea0d7d730de0d5a34f6f1fb5f868fa8e40ece"}

[commit-bot: I haz the power, 2017-03-10 20:55:27]

Description was changed from

==========
ozone: drm: Add fd number to crash keys when dmabuf mmap fails.

When native GPU memory buffers were enabled we noticed many crashes
reporting mmap failing.

This CL adds fd number to crash report when dmabuf mmap fails, because we think
fd limit may cause this crash.
It's follow-up CL of https://codereview.chromium.org/2710183005/

BUG=629521
==========

to

==========
ozone: drm: Add fd number to crash keys when dmabuf mmap fails.

When native GPU memory buffers were enabled we noticed many crashes
reporting mmap failing.

This CL adds fd number to crash report when dmabuf mmap fails, because we think
fd limit may cause this crash.
It's follow-up CL of https://codereview.chromium.org/2710183005/

BUG=629521

Review-Url: https://codereview.chromium.org/2725813005
Cr-Commit-Position: refs/heads/master@{#456154}
Committed:
https://chromium.googlesource.com/chromium/src/+/7fdea0d7d730de0d5a34f6f1fb5f...
==========

[commit-bot: I haz the power, 2017-03-10 20:55:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7fdea0d7d730de0d5a34f6f1fb5f...