Message ID | 20220115010622.3185921-5-hridya@google.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1n8XYT-009qHL-7x; Sat, 15 Jan 2022 01:08:29 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231824AbiAOBI1 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 14 Jan 2022 20:08:27 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55162 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231814AbiAOBI0 (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 14 Jan 2022 20:08:26 -0500 Received: from mail-yb1-xb49.google.com (mail-yb1-xb49.google.com [IPv6:2607:f8b0:4864:20::b49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B062AC061574 for <linux-media@vger.kernel.org>; Fri, 14 Jan 2022 17:08:26 -0800 (PST) Received: by mail-yb1-xb49.google.com with SMTP id b186-20020a25cbc3000000b00611b032ccadso17033618ybg.16 for <linux-media@vger.kernel.org>; Fri, 14 Jan 2022 17:08:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=sWrCFDJqqXXVSbGVP8s44HP9fGoTeRrZv/20U3l5a9g=; b=HsuhTKzXI1C0Bg5qlXVEUKgy0Y59ylb/5klYriv4XRxp76HIR6zzyRrTkNqoZeAa/+ b49JcXDqX9e/xJ/SUE/3bVyoaZ0lg1xpEMerwoqfVEOSdzj9xLfLi1s1GhQ+IDW9v6/g 5GED0tKyYGSjJ2/1/kebwAkX3iCP3oywer4GN/EkL+RsKPE2U2Hk2kgIljMzFb8dnS5Y FqX9cDLP+EMxfP4f35RPYTdaQONdRKxIF2yg0spELgieeV12+CV7Uxpttxe+nF4d6tb3 BSX5dX+1OeDTvZq4IqC6x7NYZPz51nGacX/FtiiRJ2MyzFoQX3zRra2Gfbo1QjE20yf4 5vjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=sWrCFDJqqXXVSbGVP8s44HP9fGoTeRrZv/20U3l5a9g=; b=CUIAcc6PxsWBNQu9ciNcXhNWLKfJbrHq0rawbGU9d+NnX80eAQpRxAINQTiHZiSfpL +vvIuJw5Ywwhc5MjL1UosgqrUSh4FGhVG2lKBC1WP23Pez0793uJYPEgvU48ggAKSPBx oCnhzWtHKAjKq1QqOujqBIQBruh+lpL3J09RX4XwaJTOOyt6SFpZYXP32vKymT9C+FcJ 4MafXbyGP/t2D7oxwJxuWeybzErH4BdsY0jZwZzooUui9NNCim/xnABqNaMocee0Gyce T2Af/ruo+L4t+jc6Zokao3Q0xAC8lXUjsMeJy5F2QffyCE3ko0kWy6VJzw+dpT7KhZbW LBFA== X-Gm-Message-State: AOAM532UIM/zWD5eouGmc0WwpCQ1U2WwSzUro07d+aEQsYwEHnw7JfOW nbh0TOLHN82mh6J+s90ImIg9UOcrV2Y= X-Google-Smtp-Source: ABdhPJxrB6uVRtSUJNiCU0mlnPcpg+H8hNUC8zW0t5DkVGpaOEH4KKhobnZfY7Jxl05uHClUdyHKGaFNjYw= X-Received: from hridya.mtv.corp.google.com ([2620:15c:211:200:5860:362a:3112:9d85]) (user=hridya job=sendgmr) by 2002:a05:6902:723:: with SMTP id l3mr17660046ybt.378.1642208905843; Fri, 14 Jan 2022 17:08:25 -0800 (PST) Date: Fri, 14 Jan 2022 17:06:02 -0800 In-Reply-To: <20220115010622.3185921-1-hridya@google.com> Message-Id: <20220115010622.3185921-5-hridya@google.com> Mime-Version: 1.0 References: <20220115010622.3185921-1-hridya@google.com> X-Mailer: git-send-email 2.34.1.703.g22d0c6ccf7-goog Subject: [RFC 4/6] dma-buf: Add DMA-BUF exporter op to charge a DMA-BUF to a cgroup. From: Hridya Valsaraju <hridya@google.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Jonathan Corbet <corbet@lwn.net>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, " =?utf-8?q?Arve_Hj=C3=B8n?= =?utf-8?q?nev=C3=A5g?= " <arve@android.com>, Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>, Joel Fernandes <joel@joelfernandes.org>, Christian Brauner <christian@brauner.io>, Hridya Valsaraju <hridya@google.com>, Suren Baghdasaryan <surenb@google.com>, Sumit Semwal <sumit.semwal@linaro.org>, Benjamin Gaignard <benjamin.gaignard@linaro.org>, Liam Mark <lmark@codeaurora.org>, Laura Abbott <labbott@redhat.com>, Brian Starkey <Brian.Starkey@arm.com>, John Stultz <john.stultz@linaro.org>, " =?utf-8?q?Christian_K=C3=B6nig?= " <christian.koenig@amd.com>, Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, Johannes Weiner <hannes@cmpxchg.org>, Dave Airlie <airlied@redhat.com>, Jason Ekstrand <jason@jlekstrand.net>, Matthew Auld <matthew.auld@intel.com>, Matthew Brost <matthew.brost@intel.com>, Li Li <dualli@google.com>, Marco Ballesio <balejs@google.com>, Miguel Ojeda <ojeda@kernel.org>, Hang Lu <hangl@codeaurora.org>, Wedson Almeida Filho <wedsonaf@google.com>, Masahiro Yamada <masahiroy@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, Nathan Chancellor <nathan@kernel.org>, Kees Cook <keescook@chromium.org>, Nick Desaulniers <ndesaulniers@google.com>, Chris Down <chris@chrisdown.name>, Vipin Sharma <vipinsh@google.com>, Daniel Borkmann <daniel@iogearbox.net>, Vlastimil Babka <vbabka@suse.cz>, Arnd Bergmann <arnd@arndb.de>, dri-devel@lists.freedesktop.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, cgroups@vger.kernel.org Cc: Kenny.Ho@amd.com, daniels@collabora.com, kaleshsingh@google.com, tjmercier@google.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -15.0 (---------------) X-LSpam-Report: No, score=-15.0 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_MED=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_HI=-5,USER_IN_DEF_DKIM_WL=-7.5 autolearn=ham autolearn_force=no |
Series |
Proposal for a GPU cgroup controller
|
|
Commit Message
Hridya Valsaraju
Jan. 15, 2022, 1:06 a.m. UTC
The optional exporter op provides a way for processes to transfer
charge of a buffer to a different process. This is essential for the
cases where a central allocator process does allocations for various
subsystems, hands over the fd to the client who
requested the memory and drops all references to the allocated memory.
Signed-off-by: Hridya Valsaraju <hridya@google.com>
---
include/linux/dma-buf.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
Comments
Am 15.01.22 um 02:06 schrieb Hridya Valsaraju: > The optional exporter op provides a way for processes to transfer > charge of a buffer to a different process. This is essential for the > cases where a central allocator process does allocations for various > subsystems, hands over the fd to the client who > requested the memory and drops all references to the allocated memory. > > Signed-off-by: Hridya Valsaraju <hridya@google.com> > --- > include/linux/dma-buf.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 7ab50076e7a6..d5e52f81cc6f 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -13,6 +13,7 @@ > #ifndef __DMA_BUF_H__ > #define __DMA_BUF_H__ > > +#include <linux/cgroup_gpu.h> > #include <linux/dma-buf-map.h> > #include <linux/file.h> > #include <linux/err.h> > @@ -285,6 +286,23 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @charge_to_cgroup: > + * > + * This is called by an exporter to charge a buffer to the specified > + * cgroup. Well that sentence makes absolutely no sense at all. The dma_buf_ops are supposed to be called by the DMA-buf subsystem on behalves of the importer and never by the exporter itself. I hope that this is just a documentation mixup. Regards, Christian. > The caller must hold a reference to @gpucg obtained via > + * gpucg_get(). The DMA-BUF will be uncharged from the cgroup it is > + * currently charged to before being charged to @gpucg. The caller must > + * belong to the cgroup the buffer is currently charged to. > + * > + * This callback is optional. > + * > + * Returns: > + * > + * 0 on success or negative error code on failure. > + */ > + int (*charge_to_cgroup)(struct dma_buf *dmabuf, struct gpucg *gpucg); > }; > > /**
On Sun, Jan 16, 2022 at 11:46 PM Christian König <christian.koenig@amd.com> wrote: > > Am 15.01.22 um 02:06 schrieb Hridya Valsaraju: > > The optional exporter op provides a way for processes to transfer > > charge of a buffer to a different process. This is essential for the > > cases where a central allocator process does allocations for various > > subsystems, hands over the fd to the client who > > requested the memory and drops all references to the allocated memory. > > > > Signed-off-by: Hridya Valsaraju <hridya@google.com> > > --- > > include/linux/dma-buf.h | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > index 7ab50076e7a6..d5e52f81cc6f 100644 > > --- a/include/linux/dma-buf.h > > +++ b/include/linux/dma-buf.h > > @@ -13,6 +13,7 @@ > > #ifndef __DMA_BUF_H__ > > #define __DMA_BUF_H__ > > > > +#include <linux/cgroup_gpu.h> > > #include <linux/dma-buf-map.h> > > #include <linux/file.h> > > #include <linux/err.h> > > @@ -285,6 +286,23 @@ struct dma_buf_ops { > > > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > + > > + /** > > + * @charge_to_cgroup: > > + * > > + * This is called by an exporter to charge a buffer to the specified > > + * cgroup. > > Well that sentence makes absolutely no sense at all. > > The dma_buf_ops are supposed to be called by the DMA-buf subsystem on > behalves of the importer and never by the exporter itself. > > I hope that this is just a documentation mixup. Thank you for taking a look Christian! Yes, that was poor wording, sorry about that. It should instead say that the op would be called by the process the buffer is currently charged to in order to transfer the buffer's charge to a different cgroup. This is helpful in the case where a process acts as an allocator for multiple client processes and we would like the allocated buffers to be charged to the clients who requested their allocation(instead of the allocating process as is the default behavior). In Android, the graphics allocator HAL process[1] does most of the graphics allocations on behalf of various clients. After allocation, the HAL process passes the fd to the client over binder IPC and the binder driver invokes the charge_to_cgroup() DMA-BUF op to uncharge the buffer from the HAL process and charge it to the client process instead. [1]: https://source.android.com/devices/graphics/arch-bq-gralloc Regards, Hridya > > Regards, > Christian. > > > The caller must hold a reference to @gpucg obtained via > > + * gpucg_get(). The DMA-BUF will be uncharged from the cgroup it is > > + * currently charged to before being charged to @gpucg. The caller must > > + * belong to the cgroup the buffer is currently charged to. > > + * > > + * This callback is optional. > > + * > > + * Returns: > > + * > > + * 0 on success or negative error code on failure. > > + */ > > + int (*charge_to_cgroup)(struct dma_buf *dmabuf, struct gpucg *gpucg); > > }; > > > > /** >
On Tue, Jan 18, 2022 at 10:54:16AM -0800, Hridya Valsaraju wrote: > On Sun, Jan 16, 2022 at 11:46 PM Christian König > <christian.koenig@amd.com> wrote: > > > > Am 15.01.22 um 02:06 schrieb Hridya Valsaraju: > > > The optional exporter op provides a way for processes to transfer > > > charge of a buffer to a different process. This is essential for the > > > cases where a central allocator process does allocations for various > > > subsystems, hands over the fd to the client who > > > requested the memory and drops all references to the allocated memory. > > > > > > Signed-off-by: Hridya Valsaraju <hridya@google.com> > > > --- > > > include/linux/dma-buf.h | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > > index 7ab50076e7a6..d5e52f81cc6f 100644 > > > --- a/include/linux/dma-buf.h > > > +++ b/include/linux/dma-buf.h > > > @@ -13,6 +13,7 @@ > > > #ifndef __DMA_BUF_H__ > > > #define __DMA_BUF_H__ > > > > > > +#include <linux/cgroup_gpu.h> > > > #include <linux/dma-buf-map.h> > > > #include <linux/file.h> > > > #include <linux/err.h> > > > @@ -285,6 +286,23 @@ struct dma_buf_ops { > > > > > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > > + > > > + /** > > > + * @charge_to_cgroup: > > > + * > > > + * This is called by an exporter to charge a buffer to the specified > > > + * cgroup. > > > > Well that sentence makes absolutely no sense at all. > > > > The dma_buf_ops are supposed to be called by the DMA-buf subsystem on > > behalves of the importer and never by the exporter itself. > > > > I hope that this is just a documentation mixup. > > Thank you for taking a look Christian! > > Yes, that was poor wording, sorry about that. It should instead say > that the op would be called by the process the buffer is currently > charged to in order to transfer the buffer's charge to a different > cgroup. This is helpful in the case where a process acts as an > allocator for multiple client processes and we would like the > allocated buffers to be charged to the clients who requested their > allocation(instead of the allocating process as is the default > behavior). In Android, the graphics allocator HAL process[1] does > most of the graphics allocations on behalf of various clients. After > allocation, the HAL process passes the fd to the client over binder > IPC and the binder driver invokes the charge_to_cgroup() DMA-BUF op to > uncharge the buffer from the HAL process and charge it to the client > process instead. > > [1]: https://source.android.com/devices/graphics/arch-bq-gralloc For that use-case, do we really need to have the vfunc abstraction and force all exporters to do something reasonable with it? I think just storing the cgrpus gpu memory bucket this is charged against and doing this in a generic way would be a lot better. That way we can also easily add other neat features in the future, like e.g. ttm could take care of charge-assignement automatically maybe, or we could print the current cgroups charge relationship in the sysfs info file. Or anything else really. I do feel that in general for gpu memory cgroups to be useful, we should really have memory pools as a fairly strong concept. Otherwise every driver/allocator/thing is going to come up with their own, and very likely incompatible interpretation. And we end up with a supposed generic cgroups interface which cannot actually be used in a driver/vendor agnostic way at all. -Daniel > > Regards, > Hridya > > > > > > Regards, > > Christian. > > > > > The caller must hold a reference to @gpucg obtained via > > > + * gpucg_get(). The DMA-BUF will be uncharged from the cgroup it is > > > + * currently charged to before being charged to @gpucg. The caller must > > > + * belong to the cgroup the buffer is currently charged to. > > > + * > > > + * This callback is optional. > > > + * > > > + * Returns: > > > + * > > > + * 0 on success or negative error code on failure. > > > + */ > > > + int (*charge_to_cgroup)(struct dma_buf *dmabuf, struct gpucg *gpucg); > > > }; > > > > > > /** > >
Am 19.01.22 um 16:54 schrieb Daniel Vetter: > On Tue, Jan 18, 2022 at 10:54:16AM -0800, Hridya Valsaraju wrote: >> On Sun, Jan 16, 2022 at 11:46 PM Christian König >> <christian.koenig@amd.com> wrote: >>> Am 15.01.22 um 02:06 schrieb Hridya Valsaraju: >>>> The optional exporter op provides a way for processes to transfer >>>> charge of a buffer to a different process. This is essential for the >>>> cases where a central allocator process does allocations for various >>>> subsystems, hands over the fd to the client who >>>> requested the memory and drops all references to the allocated memory. >>>> >>>> Signed-off-by: Hridya Valsaraju <hridya@google.com> >>>> --- >>>> include/linux/dma-buf.h | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >>>> index 7ab50076e7a6..d5e52f81cc6f 100644 >>>> --- a/include/linux/dma-buf.h >>>> +++ b/include/linux/dma-buf.h >>>> @@ -13,6 +13,7 @@ >>>> #ifndef __DMA_BUF_H__ >>>> #define __DMA_BUF_H__ >>>> >>>> +#include <linux/cgroup_gpu.h> >>>> #include <linux/dma-buf-map.h> >>>> #include <linux/file.h> >>>> #include <linux/err.h> >>>> @@ -285,6 +286,23 @@ struct dma_buf_ops { >>>> >>>> int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); >>>> void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); >>>> + >>>> + /** >>>> + * @charge_to_cgroup: >>>> + * >>>> + * This is called by an exporter to charge a buffer to the specified >>>> + * cgroup. >>> Well that sentence makes absolutely no sense at all. >>> >>> The dma_buf_ops are supposed to be called by the DMA-buf subsystem on >>> behalves of the importer and never by the exporter itself. >>> >>> I hope that this is just a documentation mixup. >> Thank you for taking a look Christian! >> >> Yes, that was poor wording, sorry about that. It should instead say >> that the op would be called by the process the buffer is currently >> charged to in order to transfer the buffer's charge to a different >> cgroup. This is helpful in the case where a process acts as an >> allocator for multiple client processes and we would like the >> allocated buffers to be charged to the clients who requested their >> allocation(instead of the allocating process as is the default >> behavior). In Android, the graphics allocator HAL process[1] does >> most of the graphics allocations on behalf of various clients. After >> allocation, the HAL process passes the fd to the client over binder >> IPC and the binder driver invokes the charge_to_cgroup() DMA-BUF op to >> uncharge the buffer from the HAL process and charge it to the client >> process instead. >> >> [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.android.com%2Fdevices%2Fgraphics%2Farch-bq-gralloc&data=04%7C01%7Cchristian.koenig%40amd.com%7C838d25da974d4ea4257508d9db63eb70%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782044488604857%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Qn7JeyF5Rq9tnrGw1KgNuQkpu5RbcrvPhDOa1OBJ6TU%3D&reserved=0 > For that use-case, do we really need to have the vfunc abstraction and > force all exporters to do something reasonable with it? I was about to write up a similar answer, but more from the technical side. Why in the world should that be done on the DMA-buf object as a communication function between importer and exporter? That design makes absolutely no sense at all to me. Regards, Christian. > > I think just storing the cgrpus gpu memory bucket this is charged against > and doing this in a generic way would be a lot better. > > That way we can also easily add other neat features in the future, like > e.g. ttm could take care of charge-assignement automatically maybe, or we > could print the current cgroups charge relationship in the sysfs info > file. Or anything else really. > > I do feel that in general for gpu memory cgroups to be useful, we should > really have memory pools as a fairly strong concept. Otherwise every > driver/allocator/thing is going to come up with their own, and very likely > incompatible interpretation. And we end up with a supposed generic cgroups > interface which cannot actually be used in a driver/vendor agnostic way at > all. > -Daniel > >> Regards, >> Hridya >> >> >>> Regards, >>> Christian. >>> >>>> The caller must hold a reference to @gpucg obtained via >>>> + * gpucg_get(). The DMA-BUF will be uncharged from the cgroup it is >>>> + * currently charged to before being charged to @gpucg. The caller must >>>> + * belong to the cgroup the buffer is currently charged to. >>>> + * >>>> + * This callback is optional. >>>> + * >>>> + * Returns: >>>> + * >>>> + * 0 on success or negative error code on failure. >>>> + */ >>>> + int (*charge_to_cgroup)(struct dma_buf *dmabuf, struct gpucg *gpucg); >>>> }; >>>> >>>> /**
On Wed, Jan 19, 2022 at 7:58 AM Christian König <christian.koenig@amd.com> wrote: > > Am 19.01.22 um 16:54 schrieb Daniel Vetter: > > On Tue, Jan 18, 2022 at 10:54:16AM -0800, Hridya Valsaraju wrote: > >> On Sun, Jan 16, 2022 at 11:46 PM Christian König > >> <christian.koenig@amd.com> wrote: > >>> Am 15.01.22 um 02:06 schrieb Hridya Valsaraju: > >>>> The optional exporter op provides a way for processes to transfer > >>>> charge of a buffer to a different process. This is essential for the > >>>> cases where a central allocator process does allocations for various > >>>> subsystems, hands over the fd to the client who > >>>> requested the memory and drops all references to the allocated memory. > >>>> > >>>> Signed-off-by: Hridya Valsaraju <hridya@google.com> > >>>> --- > >>>> include/linux/dma-buf.h | 18 ++++++++++++++++++ > >>>> 1 file changed, 18 insertions(+) > >>>> > >>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > >>>> index 7ab50076e7a6..d5e52f81cc6f 100644 > >>>> --- a/include/linux/dma-buf.h > >>>> +++ b/include/linux/dma-buf.h > >>>> @@ -13,6 +13,7 @@ > >>>> #ifndef __DMA_BUF_H__ > >>>> #define __DMA_BUF_H__ > >>>> > >>>> +#include <linux/cgroup_gpu.h> > >>>> #include <linux/dma-buf-map.h> > >>>> #include <linux/file.h> > >>>> #include <linux/err.h> > >>>> @@ -285,6 +286,23 @@ struct dma_buf_ops { > >>>> > >>>> int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > >>>> void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > >>>> + > >>>> + /** > >>>> + * @charge_to_cgroup: > >>>> + * > >>>> + * This is called by an exporter to charge a buffer to the specified > >>>> + * cgroup. > >>> Well that sentence makes absolutely no sense at all. > >>> > >>> The dma_buf_ops are supposed to be called by the DMA-buf subsystem on > >>> behalves of the importer and never by the exporter itself. > >>> > >>> I hope that this is just a documentation mixup. > >> Thank you for taking a look Christian! > >> > >> Yes, that was poor wording, sorry about that. It should instead say > >> that the op would be called by the process the buffer is currently > >> charged to in order to transfer the buffer's charge to a different > >> cgroup. This is helpful in the case where a process acts as an > >> allocator for multiple client processes and we would like the > >> allocated buffers to be charged to the clients who requested their > >> allocation(instead of the allocating process as is the default > >> behavior). In Android, the graphics allocator HAL process[1] does > >> most of the graphics allocations on behalf of various clients. After > >> allocation, the HAL process passes the fd to the client over binder > >> IPC and the binder driver invokes the charge_to_cgroup() DMA-BUF op to > >> uncharge the buffer from the HAL process and charge it to the client > >> process instead. > >> > >> [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.android.com%2Fdevices%2Fgraphics%2Farch-bq-gralloc&data=04%7C01%7Cchristian.koenig%40amd.com%7C838d25da974d4ea4257508d9db63eb70%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637782044488604857%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Qn7JeyF5Rq9tnrGw1KgNuQkpu5RbcrvPhDOa1OBJ6TU%3D&reserved=0 > > For that use-case, do we really need to have the vfunc abstraction and > > force all exporters to do something reasonable with it? > > I was about to write up a similar answer, but more from the technical side. > > Why in the world should that be done on the DMA-buf object as a > communication function between importer and exporter? > > That design makes absolutely no sense at all to me. > > Regards, > Christian. > > > > > I think just storing the cgrpus gpu memory bucket this is charged against > > and doing this in a generic way would be a lot better. > > > > That way we can also easily add other neat features in the future, like > > e.g. ttm could take care of charge-assignement automatically maybe, or we > > could print the current cgroups charge relationship in the sysfs info > > file. Or anything else really. Thank you for the comments Daniel and Christian! I made the charge/uncharge/transfer part of the exporter implementation since it provided exporters a choice on whether they wanted to enable cgroup memory accounting for the buffers they were exporting. I also see the benefits of making the charge/uncharge/transfer generic by moving it to the DMA-BUF framework like you are suggesting though. We will move to a more generic design in the next version of the RFC. Regards, Hridya > > > > I do feel that in general for gpu memory cgroups to be useful, we should > > really have memory pools as a fairly strong concept. Otherwise every > > driver/allocator/thing is going to come up with their own, and very likely > > incompatible interpretation. And we end up with a supposed generic cgroups > > interface which cannot actually be used in a driver/vendor agnostic way at > > all. > > -Daniel > > > >> Regards, > >> Hridya > >> > >> > >>> Regards, > >>> Christian. > >>> > >>>> The caller must hold a reference to @gpucg obtained via > >>>> + * gpucg_get(). The DMA-BUF will be uncharged from the cgroup it is > >>>> + * currently charged to before being charged to @gpucg. The caller must > >>>> + * belong to the cgroup the buffer is currently charged to. > >>>> + * > >>>> + * This callback is optional. > >>>> + * > >>>> + * Returns: > >>>> + * > >>>> + * 0 on success or negative error code on failure. > >>>> + */ > >>>> + int (*charge_to_cgroup)(struct dma_buf *dmabuf, struct gpucg *gpucg); > >>>> }; > >>>> > >>>> /** >
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 7ab50076e7a6..d5e52f81cc6f 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -13,6 +13,7 @@ #ifndef __DMA_BUF_H__ #define __DMA_BUF_H__ +#include <linux/cgroup_gpu.h> #include <linux/dma-buf-map.h> #include <linux/file.h> #include <linux/err.h> @@ -285,6 +286,23 @@ struct dma_buf_ops { int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); + + /** + * @charge_to_cgroup: + * + * This is called by an exporter to charge a buffer to the specified + * cgroup. The caller must hold a reference to @gpucg obtained via + * gpucg_get(). The DMA-BUF will be uncharged from the cgroup it is + * currently charged to before being charged to @gpucg. The caller must + * belong to the cgroup the buffer is currently charged to. + * + * This callback is optional. + * + * Returns: + * + * 0 on success or negative error code on failure. + */ + int (*charge_to_cgroup)(struct dma_buf *dmabuf, struct gpucg *gpucg); }; /**