Re: [PACTH v1] mm, proc: Implement /proc/<pid>/totmaps

[Jann Horn <jann@thejh.net>]

[-- Attachment #1: Type: text/plain, Size: 3080 bytes --]

On Wed, Aug 10, 2016 at 10:23:53AM -0700, Sonny Rao wrote:
> On Tue, Aug 9, 2016 at 2:01 PM, Robert Foss <robert.foss@collabora.com> wrote:
> >
> >
> > On 2016-08-09 03:24 PM, Jann Horn wrote:
> >>
> >> On Tue, Aug 09, 2016 at 12:05:43PM -0400, robert.foss@collabora.com wrote:
> >>>
> >>> From: Sonny Rao <sonnyrao@chromium.org>
> >>>
> >>> This is based on earlier work by Thiago Goncales. It implements a new
> >>> per process proc file which summarizes the contents of the smaps file
> >>> but doesn't display any addresses.  It gives more detailed information
> >>> than statm like the PSS (proprotional set size).  It differs from the
> >>> original implementation in that it doesn't use the full blown set of
> >>> seq operations, uses a different termination condition, and doesn't
> >>> displayed "Locked" as that was broken on the original implemenation.
> >>>
> >>> This new proc file provides information faster than parsing the
> >>> potentially
> >>> huge smaps file.
> >>>
> >>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> >>>
> >>> Tested-by: Robert Foss <robert.foss@collabora.com>
> >>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >>
> >>
> >>
> >>> +static int totmaps_proc_show(struct seq_file *m, void *data)
> >>> +{
> >>> +       struct proc_maps_private *priv = m->private;
> >>> +       struct mm_struct *mm;
> >>> +       struct vm_area_struct *vma;
> >>> +       struct mem_size_stats *mss_sum = priv->mss;
> >>> +
> >>> +       /* reference to priv->task already taken */
> >>> +       /* but need to get the mm here because */
> >>> +       /* task could be in the process of exiting */
> >>
> >>
> >> Can you please elaborate on this? My understanding here is that you
> >> intend for the caller to be able to repeatedly read the same totmaps
> >> file with pread() and still see updated information after the target
> >> process has called execve() and be able to detect process death
> >> (instead of simply seeing stale values). Is that accurate?
> >>
> >> I would prefer it if you could grab a reference to the mm_struct
> >> directly at open time.
> >
> >
> > Sonny, do you know more about the above comment?
> 
> I think right now the file gets re-opened every time, but the mode
> where the file is opened once and repeatedly read is interesting
> because it avoids having to open the file again and again.
> 
> I guess you could end up with a wierd situation where you don't read
> the entire contents of the file in open call to read() and you might
> get inconsistent data across the different statistics?

If the file is read in two chunks, totmaps_proc_show is only called
once. The patch specifies seq_read as read handler. Have a look at its
definition. As long as you don't read from the same seq file in
parallel or seek around in it, simple sequential reads will not
re-invoke the show() method for data that has already been formatted.
For partially consumed data, the kernel buffers the rest until someone
reads it or seeks to another offset.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]