Message-ID: <3365cd1d750e84fedc8e75d646a77ffd85619d35.camel@ndufresne.ca>
Date:   Wed, 11 May 2022 09:21:20 -0400
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     "T.J. Mercier" <tjmercier@...gle.com>, Tejun Heo <tj@...nel.org>,
        Zefan Li <lizefan.x@...edance.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Hridya Valsaraju <hridya@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        Liam Mark <lmark@...eaurora.org>,
        Laura Abbott <labbott@...hat.com>,
        Brian Starkey <Brian.Starkey@....com>,
        John Stultz <john.stultz@...aro.org>,
        Shuah Khan <shuah@...nel.org>
Cc:     daniel@...ll.ch, jstultz@...gle.com, cmllamas@...gle.com,
        kaleshsingh@...gle.com, Kenny.Ho@....com, mkoutny@...e.com,
        skhan@...uxfoundation.org, kernel-team@...roid.com,
        cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v7 0/6] Proposal for a GPU cgroup controller

Hi,

Le mardi 10 mai 2022 à 23:56 +0000, T.J. Mercier a écrit :
> This patch series revisits the proposal for a GPU cgroup controller to
> track and limit memory allocations by various device/allocator
> subsystems. The patch series also contains a simple prototype to
> illustrate how Android intends to implement DMA-BUF allocator
> attribution using the GPU cgroup controller. The prototype does not
> include resource limit enforcements.

I'm sorry, since I'm not in-depth technically involve. But from reading the
topic I don't understand the bound this creates between DMABuf Heaps and GPU. Is
this an attempt to really track the DMABuf allocated by userland, or just
something for GPU ? What about V4L2 devices ? Any way this can be clarified,
specially what would other subsystem needs to have cgroup DMABuf allocation
controller support ?

> 
> Changelog:
> v7:
> Hide gpucg and gpucg_bucket struct definitions per Michal Koutný.
> This means gpucg_register_bucket now returns an internally allocated
> struct gpucg_bucket.
> 
> Move all public function documentation to the cgroup_gpu.h header.
> 
> Remove comment in documentation about duplicate name rejection which
> is not relevant to cgroups users per Michal Koutný.
> 
> v6:
> Move documentation into cgroup-v2.rst per Tejun Heo.
> 
> Rename BINDER_FD{A}_FLAG_SENDER_NO_NEED ->
> BINDER_FD{A}_FLAG_XFER_CHARGE per Carlos Llamas.
> 
> Return error on transfer failure per Carlos Llamas.
> 
> v5:
> Rebase on top of v5.18-rc3
> 
> Drop the global GPU cgroup "total" (sum of all device totals) portion
> of the design since there is no currently known use for this per
> Tejun Heo.
> 
> Fix commit message which still contained the old name for
> dma_buf_transfer_charge per Michal Koutný.
> 
> Remove all GPU cgroup code except what's necessary to support charge transfer
> from dma_buf. Previously charging was done in export, but for non-Android
> graphics use-cases this is not ideal since there may be a delay between
> allocation and export, during which time there is no accounting.
> 
> Merge dmabuf: Use the GPU cgroup charge/uncharge APIs patch into
> dmabuf: heaps: export system_heap buffers with GPU cgroup charging as a
> result of above.
> 
> Put the charge and uncharge code in the same file (system_heap_allocate,
> system_heap_dma_buf_release) instead of splitting them between the heap and
> the dma_buf_release. This avoids asymmetric management of the gpucg charges.
> 
> Modify the dma_buf_transfer_charge API to accept a task_struct instead
> of a gpucg. This avoids requiring the caller to manage the refcount
> of the gpucg upon failure and confusing ownership transfer logic.
> 
> Support all strings for gpucg_register_bucket instead of just string
> literals.
> 
> Enforce globally unique gpucg_bucket names.
> 
> Constrain gpucg_bucket name lengths to 64 bytes.
> 
> Append "-heap" to gpucg_bucket names from dmabuf-heaps.
> 
> Drop patch 7 from the series, which changed the types of
> binder_transaction_data's sender_pid and sender_euid fields. This was
> done in another commit here:
> https://lore.kernel.org/all/20220210021129.3386083-4-masahiroy@kernel.org/
> 
> Rename:
>   gpucg_try_charge -> gpucg_charge
>   find_cg_rpool_locked -> cg_rpool_find_locked
>   init_cg_rpool -> cg_rpool_init
>   get_cg_rpool_locked -> cg_rpool_get_locked
>   "gpu cgroup controller" -> "GPU controller"
>   gpucg_device -> gpucg_bucket
>   usage -> size
> 
> Tests:
>   Support both binder_fd_array_object and binder_fd_object. This is
>   necessary because new versions of Android will use binder_fd_object
>   instead of binder_fd_array_object, and we need to support both.
> 
>   Tests for both binder_fd_array_object and binder_fd_object.
> 
>   For binder_utils return error codes instead of
>   struct binder{fs}_ctx.
> 
>   Use ifdef __ANDROID__ to choose platform-dependent temp path instead
>   of a runtime fallback.
> 
>   Ensure binderfs_mntpt ends with a trailing '/' character instead of
>   prepending it where used.
> 
> v4:
> Skip test if not run as root per Shuah Khan
> 
> Add better test logging for abnormal child termination per Shuah Khan
> 
> Adjust ordering of charge/uncharge during transfer to avoid potentially
> hitting cgroup limit per Michal Koutný
> 
> Adjust gpucg_try_charge critical section for charge transfer functionality
> 
> Fix uninitialized return code error for dmabuf_try_charge error case
> 
> v3:
> Remove Upstreaming Plan from gpu-cgroup.rst per John Stultz
> 
> Use more common dual author commit message format per John Stultz
> 
> Remove android from binder changes title per Todd Kjos
> 
> Add a kselftest for this new behavior per Greg Kroah-Hartman
> 
> Include details on behavior for all combinations of kernel/userspace
> versions in changelog (thanks Suren Baghdasaryan) per Greg Kroah-Hartman.
> 
> Fix pid and uid types in binder UAPI header
> 
> v2:
> See the previous revision of this change submitted by Hridya Valsaraju
> at: https://lore.kernel.org/all/20220115010622.3185921-1-hridya@google.com/
> 
> Move dma-buf cgroup charge transfer from a dma_buf_op defined by every
> heap to a single dma-buf function for all heaps per Daniel Vetter and
> Christian König. Pointers to struct gpucg and struct gpucg_device
> tracking the current associations were added to the dma_buf struct to
> achieve this.
> 
> Fix incorrect Kconfig help section indentation per Randy Dunlap.
> 
> History of the GPU cgroup controller
> ====================================
> The GPU/DRM cgroup controller came into being when a consensus[1]
> was reached that the resources it tracked were unsuitable to be integrated
> into memcg. Originally, the proposed controller was specific to the DRM
> subsystem and was intended to track GEM buffers and GPU-specific
> resources[2]. In order to help establish a unified memory accounting model
> for all GPU and all related subsystems, Daniel Vetter put forth a
> suggestion to move it out of the DRM subsystem so that it can be used by
> other DMA-BUF exporters as well[3]. This RFC proposes an interface that
> does the same.
> 
> [1]: https://patchwork.kernel.org/project/dri-devel/cover/20190501140438.9506-1-brian.welty@intel.com/#22624705
> [2]: https://lore.kernel.org/amd-gfx/20210126214626.16260-1-brian.welty@intel.com/
> [3]: https://lore.kernel.org/amd-gfx/YCVOl8%2F87bqRSQei@phenom.ffwll.local/
> 
> Hridya Valsaraju (3):
>   gpu: rfc: Proposal for a GPU cgroup controller
>   cgroup: gpu: Add a cgroup controller for allocator attribution of GPU
>     memory
>   binder: Add flags to relinquish ownership of fds
> 
> T.J. Mercier (3):
>   dmabuf: heaps: export system_heap buffers with GPU cgroup charging
>   dmabuf: Add gpu cgroup charge transfer function
>   selftests: Add binder cgroup gpu memory transfer tests
> 
>  Documentation/admin-guide/cgroup-v2.rst       |  23 +
>  drivers/android/binder.c                      |  31 +-
>  drivers/dma-buf/dma-buf.c                     |  80 ++-
>  drivers/dma-buf/dma-heap.c                    |  38 ++
>  drivers/dma-buf/heaps/system_heap.c           |  28 +-
>  include/linux/cgroup_gpu.h                    | 146 +++++
>  include/linux/cgroup_subsys.h                 |   4 +
>  include/linux/dma-buf.h                       |  49 +-
>  include/linux/dma-heap.h                      |  15 +
>  include/uapi/linux/android/binder.h           |  23 +-
>  init/Kconfig                                  |   7 +
>  kernel/cgroup/Makefile                        |   1 +
>  kernel/cgroup/gpu.c                           | 390 +++++++++++++
>  .../selftests/drivers/android/binder/Makefile |   8 +
>  .../drivers/android/binder/binder_util.c      | 250 +++++++++
>  .../drivers/android/binder/binder_util.h      |  32 ++
>  .../selftests/drivers/android/binder/config   |   4 +
>  .../binder/test_dmabuf_cgroup_transfer.c      | 526 ++++++++++++++++++
>  18 files changed, 1632 insertions(+), 23 deletions(-)
>  create mode 100644 include/linux/cgroup_gpu.h
>  create mode 100644 kernel/cgroup/gpu.c
>  create mode 100644 tools/testing/selftests/drivers/android/binder/Makefile
>  create mode 100644 tools/testing/selftests/drivers/android/binder/binder_util.c
>  create mode 100644 tools/testing/selftests/drivers/android/binder/binder_util.h
>  create mode 100644 tools/testing/selftests/drivers/android/binder/config
>  create mode 100644 tools/testing/selftests/drivers/android/binder/test_dmabuf_cgroup_transfer.c
> 

Powered by blists - more mailing lists