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

[Jann Horn]

On Wed, Aug 10, 2016 at 10:45:51AM -0700, Sonny Rao wrote:
> On Wed, Aug 10, 2016 at 10:37 AM, Jann Horn <jann@xxxxxxxxx> wrote:
> > 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@xxxxxxxxxxxxx> wrote:
> >> >
> >> >
> >> > On 2016-08-09 03:24 PM, Jann Horn wrote:
> >> >>
> >> >> On Tue, Aug 09, 2016 at 12:05:43PM -0400, robert.foss@xxxxxxxxxxxxx wrote:
> >> >>>
> >> >>> From: Sonny Rao <sonnyrao@xxxxxxxxxxxx>
> >> >>>
> >> >>> 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@xxxxxxxxxxxx>
> >> >>>
> >> >>> Tested-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> >> >>> Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> >> >>
> >> >>
> >> >>
> >> >>> +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.
> 
> Ok that's good.  If the consumer were using pread() though, would that
> look like a seek?

Only if the consumer uses pread() with an offset that is not the same as
the end offset of the previous read.

So if you tried to use the same file from multiple threads in parallel,
you might still have issues, but as long as you don't do that, it should
be fine.

I guess it might make sense to document this behavior somewhere - maybe
the proc.5 manpage?

Attachment: signature.asc
Description: Digital signature