Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Dave Hansen]

On 10/11/2016 04:50 PM, Ruchi Kandoi wrote:
> Any process holding a reference to these buffers will keep the kernel from
> reclaiming its backing pages.  mm counters don't provide a complete picture of
> these allocations, since they only account for pages that are mapped into a
> process's address space.  This problem is especially bad for systems like
> Android that use dma-buf fds to share graphics and multimedia buffers between
> processes: these allocations are often large, have complex sharing patterns,
> and are rarely mapped into every process that holds a reference to them.

What do you end up _doing_ with all this new information that you have
here?  You know which processes have "pinned" these shared buffers, and
exported that information in /proc.  But, then what?

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Dave Hansen]

On 10/11/2016 04:50 PM, Ruchi Kandoi wrote:
> Any process holding a reference to these buffers will keep the kernel from
> reclaiming its backing pages.  mm counters don't provide a complete picture of
> these allocations, since they only account for pages that are mapped into a
> process's address space.  This problem is especially bad for systems like
> Android that use dma-buf fds to share graphics and multimedia buffers between
> processes: these allocations are often large, have complex sharing patterns,
> and are rarely mapped into every process that holds a reference to them.

What do you end up _doing_ with all this new information that you have
here?  You know which processes have "pinned" these shared buffers, and
exported that information in /proc.  But, then what?

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Christian König]

Am 12.10.2016 um 01:50 schrieb Ruchi Kandoi:

This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
interest in memtrack, it can be extended to other memory allocators as well,
such as GEM implementations.
We have run into similar problems before. Because of this I already 
proposed a solution for this quite a while ago, but never pushed on 
upstreaming this since it was only done for a special use case.


Instead of keeping track of how much memory a process has bound (which 
is very fragile) my solution  only added some more debugging info on a 
per fd basis (e.g. how much memory is bound to this fd).


This information was then used by the OOM killer (for example) to make a 
better decision on which process to reap.


Shouldn't be to hard to expose this through debugfs or maybe a new fcntl 
to userspace for debugging.


I haven't looked at the code in detail, but messing with the per process 
memory accounting like you did in this proposal is clearly not a good 
idea if you ask me.


Regards,
Christian.

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Christian König]

Am 12.10.2016 um 01:50 schrieb Ruchi Kandoi:

This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
interest in memtrack, it can be extended to other memory allocators as well,
such as GEM implementations.
We have run into similar problems before. Because of this I already 
proposed a solution for this quite a while ago, but never pushed on 
upstreaming this since it was only done for a special use case.


Instead of keeping track of how much memory a process has bound (which 
is very fragile) my solution  only added some more debugging info on a 
per fd basis (e.g. how much memory is bound to this fd).


This information was then used by the OOM killer (for example) to make a 
better decision on which process to reap.


Shouldn't be to hard to expose this through debugfs or maybe a new fcntl 
to userspace for debugging.


I haven't looked at the code in detail, but messing with the per process 
memory accounting like you did in this proposal is clearly not a good 
idea if you ask me.


Regards,
Christian.

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Rob Clark]

On Tue, Oct 11, 2016 at 7:50 PM, Ruchi Kandoi  wrote:
> This patchstack introduces a new "memtrack" module for tracking and accounting
> memory exported to userspace as shared buffers, like dma-buf fds or GEM 
> handles.

btw, I wouldn't care much about the non-dmabuf case.. dri2/flink is
kind of legacy and the sharing patterns there are not so complex that
we have found the need for any more elaborate debug infrastructure
than what we already have.

Between existing dmabuf debugfs, and /proc/*/maps (and /proc/*/fd?), I
wonder what is missing?  Maybe there is a less intrusive way to get at
the debugging info you want?

BR,
-R

> Any process holding a reference to these buffers will keep the kernel from
> reclaiming its backing pages.  mm counters don't provide a complete picture of
> these allocations, since they only account for pages that are mapped into a
> process's address space.  This problem is especially bad for systems like
> Android that use dma-buf fds to share graphics and multimedia buffers between
> processes: these allocations are often large, have complex sharing patterns,
> and are rarely mapped into every process that holds a reference to them.
>
> memtrack maintains a per-process list of shared buffer references, which is
> exported to userspace as /proc/[pid]/memtrack.  Buffers can be optionally
> "tagged" with a short string: for example, Android userspace would use this
> tag to identify whether buffers were allocated on behalf of the camera stack,
> GL, etc.  memtrack also exports the VMAs associated with these buffers so
> that pages already included in the process's mm counters aren't 
> double-counted.
>
> Shared-buffer allocators can hook into memtrack by embedding
> struct memtrack_buffer in their buffer metadata, calling
> memtrack_buffer_{init,remove} at buffer allocation and free time, and
> memtrack_buffer_{install,uninstall} when a userspace process takes or
> drops a reference to the buffer.  For fd-backed buffers like dma-bufs, hooks 
> in
> fdtable.c and fork.c automatically notify memtrack when references are added 
> or
> removed from a process's fd table.
>
> This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
> interest in memtrack, it can be extended to other memory allocators as well,
> such as GEM implementations.
>
> Greg Hackmann (1):
>   drivers: staging: ion: add ION_IOC_TAG ioctl
>
> Ruchi Kandoi (5):
>   fs: add installed and uninstalled file_operations
>   drivers: misc: add memtrack
>   dma-buf: add memtrack support
>   memtrack: Adds the accounting to keep track of all mmaped/unmapped
> pages.
>   memtrack: Add memtrack accounting for forked processes.
>
>  drivers/android/binder.c|   4 +-
>  drivers/dma-buf/dma-buf.c   |  37 +++
>  drivers/misc/Kconfig|  16 +
>  drivers/misc/Makefile   |   1 +
>  drivers/misc/memtrack.c | 516 
> 
>  drivers/staging/android/ion/ion-ioctl.c |  17 ++
>  drivers/staging/android/ion/ion.c   |  60 +++-
>  drivers/staging/android/ion/ion_priv.h  |   2 +
>  drivers/staging/android/uapi/ion.h  |  25 ++
>  fs/file.c   |  38 ++-
>  fs/open.c   |   2 +-
>  fs/proc/base.c  |   4 +
>  include/linux/dma-buf.h |   5 +
>  include/linux/fdtable.h |   4 +-
>  include/linux/fs.h  |   2 +
>  include/linux/memtrack.h| 130 
>  include/linux/mm.h  |   3 +
>  include/linux/sched.h   |   3 +
>  kernel/fork.c   |  23 +-
>  19 files changed, 875 insertions(+), 17 deletions(-)
>  create mode 100644 drivers/misc/memtrack.c
>  create mode 100644 include/linux/memtrack.h
>
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Rob Clark]

On Tue, Oct 11, 2016 at 7:50 PM, Ruchi Kandoi  wrote:
> This patchstack introduces a new "memtrack" module for tracking and accounting
> memory exported to userspace as shared buffers, like dma-buf fds or GEM 
> handles.

btw, I wouldn't care much about the non-dmabuf case.. dri2/flink is
kind of legacy and the sharing patterns there are not so complex that
we have found the need for any more elaborate debug infrastructure
than what we already have.

Between existing dmabuf debugfs, and /proc/*/maps (and /proc/*/fd?), I
wonder what is missing?  Maybe there is a less intrusive way to get at
the debugging info you want?

BR,
-R

> Any process holding a reference to these buffers will keep the kernel from
> reclaiming its backing pages.  mm counters don't provide a complete picture of
> these allocations, since they only account for pages that are mapped into a
> process's address space.  This problem is especially bad for systems like
> Android that use dma-buf fds to share graphics and multimedia buffers between
> processes: these allocations are often large, have complex sharing patterns,
> and are rarely mapped into every process that holds a reference to them.
>
> memtrack maintains a per-process list of shared buffer references, which is
> exported to userspace as /proc/[pid]/memtrack.  Buffers can be optionally
> "tagged" with a short string: for example, Android userspace would use this
> tag to identify whether buffers were allocated on behalf of the camera stack,
> GL, etc.  memtrack also exports the VMAs associated with these buffers so
> that pages already included in the process's mm counters aren't 
> double-counted.
>
> Shared-buffer allocators can hook into memtrack by embedding
> struct memtrack_buffer in their buffer metadata, calling
> memtrack_buffer_{init,remove} at buffer allocation and free time, and
> memtrack_buffer_{install,uninstall} when a userspace process takes or
> drops a reference to the buffer.  For fd-backed buffers like dma-bufs, hooks 
> in
> fdtable.c and fork.c automatically notify memtrack when references are added 
> or
> removed from a process's fd table.
>
> This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
> interest in memtrack, it can be extended to other memory allocators as well,
> such as GEM implementations.
>
> Greg Hackmann (1):
>   drivers: staging: ion: add ION_IOC_TAG ioctl
>
> Ruchi Kandoi (5):
>   fs: add installed and uninstalled file_operations
>   drivers: misc: add memtrack
>   dma-buf: add memtrack support
>   memtrack: Adds the accounting to keep track of all mmaped/unmapped
> pages.
>   memtrack: Add memtrack accounting for forked processes.
>
>  drivers/android/binder.c|   4 +-
>  drivers/dma-buf/dma-buf.c   |  37 +++
>  drivers/misc/Kconfig|  16 +
>  drivers/misc/Makefile   |   1 +
>  drivers/misc/memtrack.c | 516 
> 
>  drivers/staging/android/ion/ion-ioctl.c |  17 ++
>  drivers/staging/android/ion/ion.c   |  60 +++-
>  drivers/staging/android/ion/ion_priv.h  |   2 +
>  drivers/staging/android/uapi/ion.h  |  25 ++
>  fs/file.c   |  38 ++-
>  fs/open.c   |   2 +-
>  fs/proc/base.c  |   4 +
>  include/linux/dma-buf.h |   5 +
>  include/linux/fdtable.h |   4 +-
>  include/linux/fs.h  |   2 +
>  include/linux/memtrack.h| 130 
>  include/linux/mm.h  |   3 +
>  include/linux/sched.h   |   3 +
>  kernel/fork.c   |  23 +-
>  19 files changed, 875 insertions(+), 17 deletions(-)
>  create mode 100644 drivers/misc/memtrack.c
>  create mode 100644 include/linux/memtrack.h
>
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Al Viro]

On Tue, Oct 11, 2016 at 04:50:04PM -0700, Ruchi Kandoi wrote:

> memtrack maintains a per-process list of shared buffer references, which is
> exported to userspace as /proc/[pid]/memtrack.  Buffers can be optionally
> "tagged" with a short string: for example, Android userspace would use this
> tag to identify whether buffers were allocated on behalf of the camera stack,
> GL, etc.  memtrack also exports the VMAs associated with these buffers so
> that pages already included in the process's mm counters aren't 
> double-counted.
> 
> Shared-buffer allocators can hook into memtrack by embedding
> struct memtrack_buffer in their buffer metadata, calling
> memtrack_buffer_{init,remove} at buffer allocation and free time, and
> memtrack_buffer_{install,uninstall} when a userspace process takes or
> drops a reference to the buffer.  For fd-backed buffers like dma-bufs, hooks 
> in
> fdtable.c and fork.c automatically notify memtrack when references are added 
> or
> removed from a process's fd table.
> 
> This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
> interest in memtrack, it can be extended to other memory allocators as well,
> such as GEM implementations.

No, with a side of Hell, No.  Not to mention anything else,
* descriptor tables do not belong to any specific task_struct and
actions done by one show up in all who share that thing.
* shared descriptor table does not imply belonging to the same
group.
* shared descriptor table can become unshared at any point, invisibly
for that Fine Piece Of Software.
* while we are at it, blocking allocation under several spinlocks
(and with interrupts disabled, for good measure) is generally considered
a bloody bad idea.

That - just from the quick look through that patchset.  Bringing task_struct
into the API is already sufficient for a NAK.

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Al Viro]

On Tue, Oct 11, 2016 at 04:50:04PM -0700, Ruchi Kandoi wrote:

> memtrack maintains a per-process list of shared buffer references, which is
> exported to userspace as /proc/[pid]/memtrack.  Buffers can be optionally
> "tagged" with a short string: for example, Android userspace would use this
> tag to identify whether buffers were allocated on behalf of the camera stack,
> GL, etc.  memtrack also exports the VMAs associated with these buffers so
> that pages already included in the process's mm counters aren't 
> double-counted.
> 
> Shared-buffer allocators can hook into memtrack by embedding
> struct memtrack_buffer in their buffer metadata, calling
> memtrack_buffer_{init,remove} at buffer allocation and free time, and
> memtrack_buffer_{install,uninstall} when a userspace process takes or
> drops a reference to the buffer.  For fd-backed buffers like dma-bufs, hooks 
> in
> fdtable.c and fork.c automatically notify memtrack when references are added 
> or
> removed from a process's fd table.
> 
> This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
> interest in memtrack, it can be extended to other memory allocators as well,
> such as GEM implementations.

No, with a side of Hell, No.  Not to mention anything else,
* descriptor tables do not belong to any specific task_struct and
actions done by one show up in all who share that thing.
* shared descriptor table does not imply belonging to the same
group.
* shared descriptor table can become unshared at any point, invisibly
for that Fine Piece Of Software.
* while we are at it, blocking allocation under several spinlocks
(and with interrupts disabled, for good measure) is generally considered
a bloody bad idea.

That - just from the quick look through that patchset.  Bringing task_struct
into the API is already sufficient for a NAK.