Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1663533005:
[tracing] Support shared and private permission. (missing perm flag) (Closed)
|
Created:
4 years, 10 months ago by Youngmin Yoo Modified:
4 years, 10 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Support shared and private permission. (missing perm flag)
following /proc/[pid]/maps perm field:
r = read
w = write
x = execute
s = shared
p = private (copy on write)
BUG=584161
Committed: https://crrev.com/7a187c6baf50ff6819ff73b3c094f2affd2f233d
Cr-Commit-Position: refs/heads/master@{#375807}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update Protection flag name #Patch Set 3 : Add related patch url #
Messages
Total messages: 31 (12 generated)
Description was changed from ========== [tracing] Support shared and private permission. (missing perm flag) following /proc/[pid]/maps perm field: r = read w = write x = execute s = shared p = private (copy on write) BUG=584161 ========== to ========== [tracing] Support shared and private permission. (missing perm flag) following /proc/[pid]/maps perm field: r = read w = write x = execute s = shared p = private (copy on write) BUG=584161 ==========
youngmin.yoo@samsung.com changed reviewers: + nduca@chromium.org
youngmin.yoo@samsung.com changed reviewers: + dsinclair@chromium.org, jbauman@chromium.org, oysteine@chromium.org, simonhatch@chromium.org
Hi, this is Youngmin. I made some patch for tracing. Could you please review it? Thanks.
primiano@chromium.org changed reviewers: + primiano@chromium.org
The patch makes sense (% the 80, which is not really a power of two). I am just curious though, what is the motivation of this? This information is not hooked in the UI, is it? Are you using tracing but consuming the JSON yourself? From my viewpoint this patch can fly regardless once we fix the 80, but I'd be interested to learn how this is going to be used https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.cc (right): https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.cc:18: const uint32_t ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; 80 ?? I guess you meant 8
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.cc (right): https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.cc:18: const uint32_t ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; nit: Is there a reason why these are not sorted in either order (8 → 1 or 1 → 8)?
https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.cc (right): https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.cc:18: const uint32_t ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; Also, could you please provide a link to some document explaining the meaning of this flag? Is the name really "may share"? Shouldn't it be "shareable"?
On 2016/02/05 11:21:34, petrcermak wrote: > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > File base/trace_event/process_memory_maps.cc (right): > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > base/trace_event/process_memory_maps.cc:18: const uint32_t > ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; > Also, could you please provide a link to some document explaining the meaning of > this flag? Is the name really "may share"? Shouldn't it be "shareable"? Please see the documents. there are use "may share". how do you think? Thanks. http://lxr.free-electrons.com/source/include/linux/mm.h#L125 #define VM_MAYSHARE 0x00000080 http://lxr.free-electrons.com/source/fs/proc/task_mmu.c#L298 299 seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ", 300 start, 301 end, 302 flags & VM_READ ? 'r' : '-', 303 flags & VM_WRITE ? 'w' : '-', 304 flags & VM_EXEC ? 'x' : '-', 305 flags & VM_MAYSHARE ? 's' : 'p', 306 pgoff, 307 MAJOR(dev), MINOR(dev), ino);
On 2016/02/05 09:32:39, Primiano Tucci wrote: > The patch makes sense (% the 80, which is not really a power of two). > I am just curious though, what is the motivation of this? This information is > not hooked in the UI, is it? > Are you using tracing but consuming the JSON yourself? > > From my viewpoint this patch can fly regardless once we fix the 80, but I'd be > interested to learn how this is going to be used > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > File base/trace_event/process_memory_maps.cc (right): > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > base/trace_event/process_memory_maps.cc:18: const uint32_t > ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; > 80 ?? > I guess you meant 8 I followed by linux/mm.h file. there are define VM flags. http://lxr.free-electrons.com/source/include/linux/mm.h#L125 #define VM_MAYSHARE 0x00000080
On 2016/02/11 11:37:54, youngmin.yoo wrote: > On 2016/02/05 09:32:39, Primiano Tucci wrote: > > The patch makes sense (% the 80, which is not really a power of two). > > I am just curious though, what is the motivation of this? This information is > > not hooked in the UI, is it? > > Are you using tracing but consuming the JSON yourself? > > > > From my viewpoint this patch can fly regardless once we fix the 80, but I'd be > > interested to learn how this is going to be used > > > > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > > File base/trace_event/process_memory_maps.cc (right): > > > > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > > base/trace_event/process_memory_maps.cc:18: const uint32_t > > ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; > > 80 ?? > > I guess you meant 8 > > I followed by linux/mm.h file. there are define VM flags. > > http://lxr.free-electrons.com/source/include/linux/mm.h#L125 > #define VM_MAYSHARE 0x00000080 0x00000080 is in hexadecimal, so it should be: ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 128;
On 2016/02/11 11:37:24, youngmin.yoo wrote: > On 2016/02/05 11:21:34, petrcermak wrote: > > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > > File base/trace_event/process_memory_maps.cc (right): > > > > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > > base/trace_event/process_memory_maps.cc:18: const uint32_t > > ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; > > Also, could you please provide a link to some document explaining the meaning > of > > this flag? Is the name really "may share"? Shouldn't it be "shareable"? > > Please see the documents. there are use "may share". how do you think? Thanks. > > http://lxr.free-electrons.com/source/include/linux/mm.h#L125 > #define VM_MAYSHARE 0x00000080 > > http://lxr.free-electrons.com/source/fs/proc/task_mmu.c#L298 > 299 seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ", > 300 start, > 301 end, > 302 flags & VM_READ ? 'r' : '-', > 303 flags & VM_WRITE ? 'w' : '-', > 304 flags & VM_EXEC ? 'x' : '-', > 305 flags & VM_MAYSHARE ? 's' : 'p', > 306 pgoff, > 307 MAJOR(dev), MINOR(dev), ino); In this case, I think that the camel-case version should be "Mayshare" (since it's one word).
Mind hex vs dec numbers plz. Also, as a general pattern, you shouldn't add ALL the owners when you make a change, just one of them. The purpose of multiple owners is round-robin, not review of CLs in RAID-1 :-) https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.cc (right): https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.cc:18: const uint32_t ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; yeah, this is either 0x80 or 128 but definitely not 80. https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.h (right): https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.h:28: static const uint32_t kProtectionFlagsMayShare; ok, MayShare is good as a name as is less strong than "shared" and reflects the kernel.
On 2016/02/11 13:51:48, Primiano Tucci wrote: > Mind hex vs dec numbers plz. > Also, as a general pattern, you shouldn't add ALL the owners when you make a > change, just one of them. > The purpose of multiple owners is round-robin, not review of CLs in RAID-1 :-) > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > File base/trace_event/process_memory_maps.cc (right): > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > base/trace_event/process_memory_maps.cc:18: const uint32_t > ProcessMemoryMaps::VMRegion::kProtectionFlagsMayShare = 80; > yeah, this is either 0x80 or 128 but definitely not 80. > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > File base/trace_event/process_memory_maps.h (right): > > https://codereview.chromium.org/1663533005/diff/1/base/trace_event/process_me... > base/trace_event/process_memory_maps.h:28: static const uint32_t > kProtectionFlagsMayShare; > ok, MayShare is good as a name as is less strong than "shared" and reflects the > kernel. Thanks for your reviews @petrcermak, @Primiano :) I pushed patche set.
lgtm
The CQ bit was checked by youngmin.yoo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1663533005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1663533005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/02/15 00:45:02, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Hi, i got some issue. how can i get LGTM from OWNERS? ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: components/tracing/process_metrics_memory_dump_provider.cc Thanks.
primiano@chromium.org changed reviewers: - dsinclair@chromium.org, jbauman@chromium.org, nduca@chromium.org, simonhatch@chromium.org
+oysteine for components/ OWNERS
lgtm
The CQ bit was checked by youngmin.yoo@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/1663533005/#ps40001 (title: "Add related patch url")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1663533005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1663533005/40001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Support shared and private permission. (missing perm flag) following /proc/[pid]/maps perm field: r = read w = write x = execute s = shared p = private (copy on write) BUG=584161 ========== to ========== [tracing] Support shared and private permission. (missing perm flag) following /proc/[pid]/maps perm field: r = read w = write x = execute s = shared p = private (copy on write) BUG=584161 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Support shared and private permission. (missing perm flag) following /proc/[pid]/maps perm field: r = read w = write x = execute s = shared p = private (copy on write) BUG=584161 ========== to ========== [tracing] Support shared and private permission. (missing perm flag) following /proc/[pid]/maps perm field: r = read w = write x = execute s = shared p = private (copy on write) BUG=584161 Committed: https://crrev.com/7a187c6baf50ff6819ff73b3c094f2affd2f233d Cr-Commit-Position: refs/heads/master@{#375807} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7a187c6baf50ff6819ff73b3c094f2affd2f233d Cr-Commit-Position: refs/heads/master@{#375807} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
