Re: [PATCH] Add /proc/pid/smaps_rollup

[Greg KH]

On Tue, Aug 08, 2017 at 11:51:40AM -0700, Daniel Colascione wrote:
> On Tue, Aug 08 2017, Greg KH wrote:
> > On Tue, Aug 08, 2017 at 11:22:55AM -0700, Daniel Colascione wrote:
> >> Adding more people.
> >> 
> >> On Tue, Aug 08 2017, Daniel Colascione wrote:
> >> > /proc/pid/smaps_rollup is a new proc file that improves the
> >> > performance of user programs that determine aggregate memory
> >> > statistics (e.g., total PSS) of a process.
> >
> > What about linux-mm?  Don't they care about this?
> 
> Thanks. Added.
> 
> >
> > Also, do you have a Documentation/ABI update to describe exactly the
> > output of this file so we know what it is, and know not to change it in
> > the future?  Or wherever proc files are documented, I'm not sure if ABI/
> > has all that many of them at the moment given the age of most of
> > them...
> 
> I figured I'd get feedback on the patch itself first. Does that make sense?

You need to show exactly what this new file is looking like, that's the
the best way to see if the code is doing what you say you require in the
best way.

Also, linux-api might care about that, so you might want to do a v2 and
include that list as well.

thanks,

greg k-h

Re: [PATCH] Add /proc/pid/smaps_rollup

[Greg KH]

On Tue, Aug 08, 2017 at 11:51:40AM -0700, Daniel Colascione wrote:
> On Tue, Aug 08 2017, Greg KH wrote:
> > On Tue, Aug 08, 2017 at 11:22:55AM -0700, Daniel Colascione wrote:
> >> Adding more people.
> >> 
> >> On Tue, Aug 08 2017, Daniel Colascione wrote:
> >> > /proc/pid/smaps_rollup is a new proc file that improves the
> >> > performance of user programs that determine aggregate memory
> >> > statistics (e.g., total PSS) of a process.
> >
> > What about linux-mm?  Don't they care about this?
> 
> Thanks. Added.
> 
> >
> > Also, do you have a Documentation/ABI update to describe exactly the
> > output of this file so we know what it is, and know not to change it in
> > the future?  Or wherever proc files are documented, I'm not sure if ABI/
> > has all that many of them at the moment given the age of most of
> > them...
> 
> I figured I'd get feedback on the patch itself first. Does that make sense?

You need to show exactly what this new file is looking like, that's the
the best way to see if the code is doing what you say you require in the
best way.

Also, linux-api might care about that, so you might want to do a v2 and
include that list as well.

thanks,

greg k-h

Re: [PATCH] Add /proc/pid/smaps_rollup

[Daniel Colascione]

On Tue, Aug 08 2017, Greg KH wrote:
> On Tue, Aug 08, 2017 at 11:22:55AM -0700, Daniel Colascione wrote:
>> Adding more people.
>> 
>> On Tue, Aug 08 2017, Daniel Colascione wrote:
>> > /proc/pid/smaps_rollup is a new proc file that improves the
>> > performance of user programs that determine aggregate memory
>> > statistics (e.g., total PSS) of a process.
>
> What about linux-mm?  Don't they care about this?

Thanks. Added.

>
> Also, do you have a Documentation/ABI update to describe exactly the
> output of this file so we know what it is, and know not to change it in
> the future?  Or wherever proc files are documented, I'm not sure if ABI/
> has all that many of them at the moment given the age of most of
> them...

I figured I'd get feedback on the patch itself first. Does that make sense?

>
>> > Anroid regularly "samples" the memory usage of various processes in
>
> Spell checkers are your friend :)

Egg applied to face.

Re: [PATCH] Add /proc/pid/smaps_rollup

[Daniel Colascione]

On Tue, Aug 08 2017, Greg KH wrote:
> On Tue, Aug 08, 2017 at 11:22:55AM -0700, Daniel Colascione wrote:
>> Adding more people.
>> 
>> On Tue, Aug 08 2017, Daniel Colascione wrote:
>> > /proc/pid/smaps_rollup is a new proc file that improves the
>> > performance of user programs that determine aggregate memory
>> > statistics (e.g., total PSS) of a process.
>
> What about linux-mm?  Don't they care about this?

Thanks. Added.

>
> Also, do you have a Documentation/ABI update to describe exactly the
> output of this file so we know what it is, and know not to change it in
> the future?  Or wherever proc files are documented, I'm not sure if ABI/
> has all that many of them at the moment given the age of most of
> them...

I figured I'd get feedback on the patch itself first. Does that make sense?

>
>> > Anroid regularly "samples" the memory usage of various processes in
>
> Spell checkers are your friend :)

Egg applied to face.

Re: [PATCH] Add /proc/pid/smaps_rollup

[Greg KH]

On Tue, Aug 08, 2017 at 11:22:55AM -0700, Daniel Colascione wrote:
> Adding more people.
> 
> On Tue, Aug 08 2017, Daniel Colascione wrote:
> > /proc/pid/smaps_rollup is a new proc file that improves the
> > performance of user programs that determine aggregate memory
> > statistics (e.g., total PSS) of a process.

What about linux-mm?  Don't they care about this?

Also, do you have a Documentation/ABI update to describe exactly the
output of this file so we know what it is, and know not to change it in
the future?  Or wherever proc files are documented, I'm not sure if ABI/
has all that many of them at the moment given the age of most of them...

> > Anroid regularly "samples" the memory usage of various processes in

Spell checkers are your friend :)

thanks,

greg k-h

Re: [PATCH] Add /proc/pid/smaps_rollup

[Greg KH]

On Tue, Aug 08, 2017 at 11:22:55AM -0700, Daniel Colascione wrote:
> Adding more people.
> 
> On Tue, Aug 08 2017, Daniel Colascione wrote:
> > /proc/pid/smaps_rollup is a new proc file that improves the
> > performance of user programs that determine aggregate memory
> > statistics (e.g., total PSS) of a process.

What about linux-mm?  Don't they care about this?

Also, do you have a Documentation/ABI update to describe exactly the
output of this file so we know what it is, and know not to change it in
the future?  Or wherever proc files are documented, I'm not sure if ABI/
has all that many of them at the moment given the age of most of them...

> > Anroid regularly "samples" the memory usage of various processes in

Spell checkers are your friend :)

thanks,

greg k-h

Re: [PATCH] Add /proc/pid/smaps_rollup

[Daniel Colascione]

Adding more people.

On Tue, Aug 08 2017, Daniel Colascione wrote:
> /proc/pid/smaps_rollup is a new proc file that improves the
> performance of user programs that determine aggregate memory
> statistics (e.g., total PSS) of a process.
>
> Anroid regularly "samples" the memory usage of various processes in
> order to blance its memory pool sizes. This sampling process involves
> opening /proc/pid/smaps and summing certain fields. For very large
> processes, sampling memory use this way can take several hundred
> milliseconds, due mostly to the overhead of the seq_printf calls in
> task_mmu.c.
>
> smaps_rollup improves the situation. It contains most of the fields of
> /proc/pid/smaps, but instead of a set of fields for each VMA,
> smaps_rollup instead contains one synthetic smaps-format entry
> representing the whole process. In the single smaps_rollup synthetic
> entry, each field is the summation of the corresponding field in all
> of the real-smaps VMAs. Using a common format for smaps_rollup and
> smaps allows userspace parsers to repurpose parsers meant for use with
> non-rollup smaps for smaps_rollup, and it allows userspace to switch
> between smaps_rollup and smaps at runtime (say, based on the
> availablity of smaps_rollup in a given kernel) with minimal fuss.
>
> By using smaps_rollup instead of smaps, a caller can avoid the
> significant overhead of formatting, reading, and parsing each of a
> large process's potentially very numerous memory mappings. For
> sampling system_server's PSS in Android, we measured a 12x speedup,
> representing a savings of several hundred milliseconds.
>
> One alternative to a new per-process proc file would have been
> including PSS information in /proc/pid/status. We considered this
> option but thought that PSS would be too expensive (by a few orders of
> magnitude) to collect relative to what's already emitted as part of
> /proc/pid/status, and slowing every user of /proc/pid/status for the
> sake of readers that happen to want PSS feels wrong.
>
> The code itself works by reusing the existing VMA-walking framework we
> use for regular smaps generation and keeping the mem_size_stats
> structure around between VMA walks instead of using a fresh one for
> each VMA.  In this way, summation happens automatically.  We let
> seq_file walk over the VMAs just as it does for regular smaps and just
> emit nothing to the seq_file until we hit the last VMA.
>
> Signed-off-by: Daniel Colascione 
> ---
>  fs/proc/base.c |   2 +
>  fs/proc/internal.h |   3 +
>  fs/proc/task_mmu.c | 196 
> -
>  3 files changed, 139 insertions(+), 62 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 719c2e943ea1..a9587b9cace5 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2930,6 +2930,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>  #ifdef CONFIG_PROC_PAGE_MONITOR
>   REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
>   REG("smaps",  S_IRUGO, proc_pid_smaps_operations),
> + REG("smaps_rollup", S_IRUGO, proc_pid_smaps_rollup_operations),
>   REG("pagemap",S_IRUSR, proc_pagemap_operations),
>  #endif
>  #ifdef CONFIG_SECURITY
> @@ -3323,6 +3324,7 @@ static const struct pid_entry tid_base_stuff[] = {
>  #ifdef CONFIG_PROC_PAGE_MONITOR
>   REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
>   REG("smaps", S_IRUGO, proc_tid_smaps_operations),
> + REG("smaps_rollup", S_IRUGO, proc_pid_smaps_rollup_operations),
>   REG("pagemap",S_IRUSR, proc_pagemap_operations),
>  #endif
>  #ifdef CONFIG_SECURITY
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index aa2b89071630..2cbfcd32e884 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -269,10 +269,12 @@ extern int proc_remount(struct super_block *, int *, 
> char *);
>  /*
>   * task_[no]mmu.c
>   */
> +struct mem_size_stats;
>  struct proc_maps_private {
>   struct inode *inode;
>   struct task_struct *task;
>   struct mm_struct *mm;
> + struct mem_size_stats *rollup;
>  #ifdef CONFIG_MMU
>   struct vm_area_struct *tail_vma;
>  #endif
> @@ -288,6 +290,7 @@ extern const struct file_operations 
> proc_tid_maps_operations;
>  extern const struct file_operations proc_pid_numa_maps_operations;
>  extern const struct file_operations proc_tid_numa_maps_operations;
>  extern const struct file_operations proc_pid_smaps_operations;
> +extern const struct file_operations proc_pid_smaps_rollup_operations;
>  extern const struct file_operations proc_tid_smaps_operations;
>  extern const struct file_operations proc_clear_refs_operations;
>  extern const struct file_operations proc_pagemap_operations;
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index b836fd61ed87..02b55df7291c 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -252,6 +252,7 @@ static int proc_map_release(struct inode *inode, struct 
> file *file)
>   if 

Re: [PATCH] Add /proc/pid/smaps_rollup

[Daniel Colascione]

Adding more people.

On Tue, Aug 08 2017, Daniel Colascione wrote:
> /proc/pid/smaps_rollup is a new proc file that improves the
> performance of user programs that determine aggregate memory
> statistics (e.g., total PSS) of a process.
>
> Anroid regularly "samples" the memory usage of various processes in
> order to blance its memory pool sizes. This sampling process involves
> opening /proc/pid/smaps and summing certain fields. For very large
> processes, sampling memory use this way can take several hundred
> milliseconds, due mostly to the overhead of the seq_printf calls in
> task_mmu.c.
>
> smaps_rollup improves the situation. It contains most of the fields of
> /proc/pid/smaps, but instead of a set of fields for each VMA,
> smaps_rollup instead contains one synthetic smaps-format entry
> representing the whole process. In the single smaps_rollup synthetic
> entry, each field is the summation of the corresponding field in all
> of the real-smaps VMAs. Using a common format for smaps_rollup and
> smaps allows userspace parsers to repurpose parsers meant for use with
> non-rollup smaps for smaps_rollup, and it allows userspace to switch
> between smaps_rollup and smaps at runtime (say, based on the
> availablity of smaps_rollup in a given kernel) with minimal fuss.
>
> By using smaps_rollup instead of smaps, a caller can avoid the
> significant overhead of formatting, reading, and parsing each of a
> large process's potentially very numerous memory mappings. For
> sampling system_server's PSS in Android, we measured a 12x speedup,
> representing a savings of several hundred milliseconds.
>
> One alternative to a new per-process proc file would have been
> including PSS information in /proc/pid/status. We considered this
> option but thought that PSS would be too expensive (by a few orders of
> magnitude) to collect relative to what's already emitted as part of
> /proc/pid/status, and slowing every user of /proc/pid/status for the
> sake of readers that happen to want PSS feels wrong.
>
> The code itself works by reusing the existing VMA-walking framework we
> use for regular smaps generation and keeping the mem_size_stats
> structure around between VMA walks instead of using a fresh one for
> each VMA.  In this way, summation happens automatically.  We let
> seq_file walk over the VMAs just as it does for regular smaps and just
> emit nothing to the seq_file until we hit the last VMA.
>
> Signed-off-by: Daniel Colascione 
> ---
>  fs/proc/base.c |   2 +
>  fs/proc/internal.h |   3 +
>  fs/proc/task_mmu.c | 196 
> -
>  3 files changed, 139 insertions(+), 62 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 719c2e943ea1..a9587b9cace5 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2930,6 +2930,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>  #ifdef CONFIG_PROC_PAGE_MONITOR
>   REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
>   REG("smaps",  S_IRUGO, proc_pid_smaps_operations),
> + REG("smaps_rollup", S_IRUGO, proc_pid_smaps_rollup_operations),
>   REG("pagemap",S_IRUSR, proc_pagemap_operations),
>  #endif
>  #ifdef CONFIG_SECURITY
> @@ -3323,6 +3324,7 @@ static const struct pid_entry tid_base_stuff[] = {
>  #ifdef CONFIG_PROC_PAGE_MONITOR
>   REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
>   REG("smaps", S_IRUGO, proc_tid_smaps_operations),
> + REG("smaps_rollup", S_IRUGO, proc_pid_smaps_rollup_operations),
>   REG("pagemap",S_IRUSR, proc_pagemap_operations),
>  #endif
>  #ifdef CONFIG_SECURITY
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index aa2b89071630..2cbfcd32e884 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -269,10 +269,12 @@ extern int proc_remount(struct super_block *, int *, 
> char *);
>  /*
>   * task_[no]mmu.c
>   */
> +struct mem_size_stats;
>  struct proc_maps_private {
>   struct inode *inode;
>   struct task_struct *task;
>   struct mm_struct *mm;
> + struct mem_size_stats *rollup;
>  #ifdef CONFIG_MMU
>   struct vm_area_struct *tail_vma;
>  #endif
> @@ -288,6 +290,7 @@ extern const struct file_operations 
> proc_tid_maps_operations;
>  extern const struct file_operations proc_pid_numa_maps_operations;
>  extern const struct file_operations proc_tid_numa_maps_operations;
>  extern const struct file_operations proc_pid_smaps_operations;
> +extern const struct file_operations proc_pid_smaps_rollup_operations;
>  extern const struct file_operations proc_tid_smaps_operations;
>  extern const struct file_operations proc_clear_refs_operations;
>  extern const struct file_operations proc_pagemap_operations;
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index b836fd61ed87..02b55df7291c 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -252,6 +252,7 @@ static int proc_map_release(struct inode *inode, struct 
> file *file)
>   if (priv->mm)
>