This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
that a process sending an fd array to another process over binder IPC
can set to relinquish ownership of the fds being sent for memory
accounting purposes. If the flag is found to be set during the fd array
translation and the fd is for a DMA-BUF, the buffer is uncharged from
the sender's cgroup and charged to the receiving process's cgroup
instead.
It is up to the sending process to ensure that it closes the fds
regardless of whether the transfer failed or succeeded.
Most graphics shared memory allocations in Android are done by the
graphics allocator HAL process. On requests from clients, the HAL process
allocates memory and sends the fds to the clients over binder IPC.
The graphics allocator HAL will not retain any references to the
buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
correctly charge the buffers to the client processes instead of the
graphics allocator HAL.
From: Hridya Valsaraju <[email protected]>
Signed-off-by: Hridya Valsaraju <[email protected]>
Co-developed-by: T.J. Mercier <[email protected]>
Signed-off-by: T.J. Mercier <[email protected]>
---
changes in v2
- 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.
drivers/android/binder.c | 26 ++++++++++++++++++++++++++
include/uapi/linux/android/binder.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8351c5638880..f50d88ded188 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -42,6 +42,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/dma-buf.h>
#include <linux/fdtable.h>
#include <linux/file.h>
#include <linux/freezer.h>
@@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
{
binder_size_t fdi, fd_buf_size;
binder_size_t fda_offset;
+ bool transfer_gpu_charge = false;
const void __user *sender_ufda_base;
struct binder_proc *proc = thread->proc;
+ struct binder_proc *target_proc = t->to_proc;
int ret;
fd_buf_size = sizeof(u32) * fda->num_fds;
@@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
if (ret)
return ret;
+ if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
+ parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
+ transfer_gpu_charge = true;
+
for (fdi = 0; fdi < fda->num_fds; fdi++) {
u32 fd;
+ struct dma_buf *dmabuf;
+ struct gpucg *gpucg;
+
binder_size_t offset = fda_offset + fdi * sizeof(fd);
binder_size_t sender_uoffset = fdi * sizeof(fd);
@@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
in_reply_to);
if (ret)
return ret > 0 ? -EINVAL : ret;
+
+ if (!transfer_gpu_charge)
+ continue;
+
+ dmabuf = dma_buf_get(fd);
+ if (IS_ERR(dmabuf))
+ continue;
+
+ gpucg = gpucg_get(target_proc->tsk);
+ ret = dma_buf_charge_transfer(dmabuf, gpucg);
+ if (ret) {
+ pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
+ proc->pid, thread->pid, target_proc->pid);
+ gpucg_put(gpucg);
+ }
+ dma_buf_put(dmabuf);
}
return 0;
}
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 3246f2c74696..169fd5069a1a 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -137,6 +137,7 @@ struct binder_buffer_object {
enum {
BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
+ BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
};
/* struct binder_fd_array_object - object describing an array of fds in a buffer
--
2.35.1.265.g69c8d7142f-goog
On Fri, Feb 11, 2022 at 04:18:29PM +0000, T.J. Mercier wrote:
> This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> that a process sending an fd array to another process over binder IPC
> can set to relinquish ownership of the fds being sent for memory
> accounting purposes. If the flag is found to be set during the fd array
> translation and the fd is for a DMA-BUF, the buffer is uncharged from
> the sender's cgroup and charged to the receiving process's cgroup
> instead.
>
> It is up to the sending process to ensure that it closes the fds
> regardless of whether the transfer failed or succeeded.
>
> Most graphics shared memory allocations in Android are done by the
> graphics allocator HAL process. On requests from clients, the HAL process
> allocates memory and sends the fds to the clients over binder IPC.
> The graphics allocator HAL will not retain any references to the
> buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> correctly charge the buffers to the client processes instead of the
> graphics allocator HAL.
>
> From: Hridya Valsaraju <[email protected]>
> Signed-off-by: Hridya Valsaraju <[email protected]>
> Co-developed-by: T.J. Mercier <[email protected]>
> Signed-off-by: T.J. Mercier <[email protected]>
> ---
> changes in v2
> - 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.
>
> drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> include/uapi/linux/android/binder.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8351c5638880..f50d88ded188 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -42,6 +42,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/dma-buf.h>
> #include <linux/fdtable.h>
> #include <linux/file.h>
> #include <linux/freezer.h>
> @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> {
> binder_size_t fdi, fd_buf_size;
> binder_size_t fda_offset;
> + bool transfer_gpu_charge = false;
> const void __user *sender_ufda_base;
> struct binder_proc *proc = thread->proc;
> + struct binder_proc *target_proc = t->to_proc;
> int ret;
>
> fd_buf_size = sizeof(u32) * fda->num_fds;
> @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> if (ret)
> return ret;
>
> + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> + transfer_gpu_charge = true;
> +
> for (fdi = 0; fdi < fda->num_fds; fdi++) {
> u32 fd;
> + struct dma_buf *dmabuf;
> + struct gpucg *gpucg;
> +
> binder_size_t offset = fda_offset + fdi * sizeof(fd);
> binder_size_t sender_uoffset = fdi * sizeof(fd);
>
> @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> in_reply_to);
> if (ret)
> return ret > 0 ? -EINVAL : ret;
> +
> + if (!transfer_gpu_charge)
> + continue;
> +
> + dmabuf = dma_buf_get(fd);
> + if (IS_ERR(dmabuf))
> + continue;
> +
> + gpucg = gpucg_get(target_proc->tsk);
> + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> + if (ret) {
> + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> + proc->pid, thread->pid, target_proc->pid);
> + gpucg_put(gpucg);
> + }
> + dma_buf_put(dmabuf);
> }
> return 0;
> }
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index 3246f2c74696..169fd5069a1a 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -137,6 +137,7 @@ struct binder_buffer_object {
>
> enum {
> BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> };
>
> /* struct binder_fd_array_object - object describing an array of fds in a buffer
> --
> 2.35.1.265.g69c8d7142f-goog
>
How does userspace know that binder supports this new flag? And where
is the userspace test for this new feature? Isn't there a binder test
framework somewhere?
thanks,
greg k-h
On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Fri, Feb 11, 2022 at 04:18:29PM +0000, T.J. Mercier wrote:
Title: "android: binder: Add a buffer flag to relinquish ownership of fds"
Please drop the "android:" from the title.
> > This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> > that a process sending an fd array to another process over binder IPC
> > can set to relinquish ownership of the fds being sent for memory
> > accounting purposes. If the flag is found to be set during the fd array
> > translation and the fd is for a DMA-BUF, the buffer is uncharged from
> > the sender's cgroup and charged to the receiving process's cgroup
> > instead.
> >
> > It is up to the sending process to ensure that it closes the fds
> > regardless of whether the transfer failed or succeeded.
> >
> > Most graphics shared memory allocations in Android are done by the
> > graphics allocator HAL process. On requests from clients, the HAL process
> > allocates memory and sends the fds to the clients over binder IPC.
> > The graphics allocator HAL will not retain any references to the
> > buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> > arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> > correctly charge the buffers to the client processes instead of the
> > graphics allocator HAL.
> >
> > From: Hridya Valsaraju <[email protected]>
> > Signed-off-by: Hridya Valsaraju <[email protected]>
> > Co-developed-by: T.J. Mercier <[email protected]>
> > Signed-off-by: T.J. Mercier <[email protected]>
> > ---
> > changes in v2
> > - 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.
> >
> > drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> > include/uapi/linux/android/binder.h | 1 +
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 8351c5638880..f50d88ded188 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -42,6 +42,7 @@
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > +#include <linux/dma-buf.h>
> > #include <linux/fdtable.h>
> > #include <linux/file.h>
> > #include <linux/freezer.h>
> > @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > {
> > binder_size_t fdi, fd_buf_size;
> > binder_size_t fda_offset;
> > + bool transfer_gpu_charge = false;
> > const void __user *sender_ufda_base;
> > struct binder_proc *proc = thread->proc;
> > + struct binder_proc *target_proc = t->to_proc;
> > int ret;
> >
> > fd_buf_size = sizeof(u32) * fda->num_fds;
> > @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > if (ret)
> > return ret;
> >
> > + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> > + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> > + transfer_gpu_charge = true;
> > +
> > for (fdi = 0; fdi < fda->num_fds; fdi++) {
> > u32 fd;
> > + struct dma_buf *dmabuf;
> > + struct gpucg *gpucg;
> > +
> > binder_size_t offset = fda_offset + fdi * sizeof(fd);
> > binder_size_t sender_uoffset = fdi * sizeof(fd);
> >
> > @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > in_reply_to);
> > if (ret)
> > return ret > 0 ? -EINVAL : ret;
> > +
> > + if (!transfer_gpu_charge)
> > + continue;
> > +
> > + dmabuf = dma_buf_get(fd);
> > + if (IS_ERR(dmabuf))
> > + continue;
> > +
> > + gpucg = gpucg_get(target_proc->tsk);
> > + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> > + if (ret) {
> > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> > + proc->pid, thread->pid, target_proc->pid);
> > + gpucg_put(gpucg);
> > + }
> > + dma_buf_put(dmabuf);
Since we are creating a new gpu cgroup abstraction, couldn't this
"transfer" be done in userspace by the target instead of in the kernel
driver? Then this patch would reduce to just a flag on the buffer
object. This also solves the issue that Greg brought up about
userspace needing to know whether the kernel implements this feature
(older kernel running with newer userspace). I think we could just
reserve some flags for userspace to use (and since those flags are
"reserved" for older kernels, this would enable this feature even for
old kernels)
> > }
> > return 0;
> > }
> > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > index 3246f2c74696..169fd5069a1a 100644
> > --- a/include/uapi/linux/android/binder.h
> > +++ b/include/uapi/linux/android/binder.h
> > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> >
> > enum {
> > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > };
> >
> > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > --
> > 2.35.1.265.g69c8d7142f-goog
> >
>
> How does userspace know that binder supports this new flag? And where
> is the userspace test for this new feature? Isn't there a binder test
> framework somewhere?
>
> thanks,
>
> greg k-h
On Mon, Feb 14, 2022 at 11:29 AM Suren Baghdasaryan <[email protected]> wrote:
>
> On Mon, Feb 14, 2022 at 10:33 AM Todd Kjos <[email protected]> wrote:
> >
> > On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > On Fri, Feb 11, 2022 at 04:18:29PM +0000, T.J. Mercier wrote:
> >
> > Title: "android: binder: Add a buffer flag to relinquish ownership of fds"
> >
> > Please drop the "android:" from the title.
> >
> > > > This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> > > > that a process sending an fd array to another process over binder IPC
> > > > can set to relinquish ownership of the fds being sent for memory
> > > > accounting purposes. If the flag is found to be set during the fd array
> > > > translation and the fd is for a DMA-BUF, the buffer is uncharged from
> > > > the sender's cgroup and charged to the receiving process's cgroup
> > > > instead.
> > > >
> > > > It is up to the sending process to ensure that it closes the fds
> > > > regardless of whether the transfer failed or succeeded.
> > > >
> > > > Most graphics shared memory allocations in Android are done by the
> > > > graphics allocator HAL process. On requests from clients, the HAL process
> > > > allocates memory and sends the fds to the clients over binder IPC.
> > > > The graphics allocator HAL will not retain any references to the
> > > > buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> > > > arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> > > > correctly charge the buffers to the client processes instead of the
> > > > graphics allocator HAL.
> > > >
> > > > From: Hridya Valsaraju <[email protected]>
> > > > Signed-off-by: Hridya Valsaraju <[email protected]>
> > > > Co-developed-by: T.J. Mercier <[email protected]>
> > > > Signed-off-by: T.J. Mercier <[email protected]>
> > > > ---
> > > > changes in v2
> > > > - 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.
> > > >
> > > > drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> > > > include/uapi/linux/android/binder.h | 1 +
> > > > 2 files changed, 27 insertions(+)
> > > >
> > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > index 8351c5638880..f50d88ded188 100644
> > > > --- a/drivers/android/binder.c
> > > > +++ b/drivers/android/binder.c
> > > > @@ -42,6 +42,7 @@
> > > >
> > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > >
> > > > +#include <linux/dma-buf.h>
> > > > #include <linux/fdtable.h>
> > > > #include <linux/file.h>
> > > > #include <linux/freezer.h>
> > > > @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > > > {
> > > > binder_size_t fdi, fd_buf_size;
> > > > binder_size_t fda_offset;
> > > > + bool transfer_gpu_charge = false;
> > > > const void __user *sender_ufda_base;
> > > > struct binder_proc *proc = thread->proc;
> > > > + struct binder_proc *target_proc = t->to_proc;
> > > > int ret;
> > > >
> > > > fd_buf_size = sizeof(u32) * fda->num_fds;
> > > > @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> > > > + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> > > > + transfer_gpu_charge = true;
> > > > +
> > > > for (fdi = 0; fdi < fda->num_fds; fdi++) {
> > > > u32 fd;
> > > > + struct dma_buf *dmabuf;
> > > > + struct gpucg *gpucg;
> > > > +
> > > > binder_size_t offset = fda_offset + fdi * sizeof(fd);
> > > > binder_size_t sender_uoffset = fdi * sizeof(fd);
> > > >
> > > > @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > > > in_reply_to);
> > > > if (ret)
> > > > return ret > 0 ? -EINVAL : ret;
> > > > +
> > > > + if (!transfer_gpu_charge)
> > > > + continue;
> > > > +
> > > > + dmabuf = dma_buf_get(fd);
> > > > + if (IS_ERR(dmabuf))
> > > > + continue;
> > > > +
> > > > + gpucg = gpucg_get(target_proc->tsk);
> > > > + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> > > > + if (ret) {
> > > > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> > > > + proc->pid, thread->pid, target_proc->pid);
> > > > + gpucg_put(gpucg);
> > > > + }
> > > > + dma_buf_put(dmabuf);
> >
> > Since we are creating a new gpu cgroup abstraction, couldn't this
> > "transfer" be done in userspace by the target instead of in the kernel
> > driver? Then this patch would reduce to just a flag on the buffer
> > object.
>
> Are you suggesting to have a userspace accessible cgroup interface for
> transferring buffer charges and the target process to use that
> interface for requesting the buffer to be charged to its cgroup?
Well, I'm asking why we need to do these cgroup-ish actions in the
kernel when it seems more natural to do it in userspace.
> I'm worried about the case when the target process does not request
> the transfer after receiving the buffer with this flag set. The charge
> would stay with the wrong process and accounting will be invalid.
I suspect this would be implemented in libbinder wherever the fd array
object is handled, so it wouldn't require changes to every process.
>
> Technically, since the proposed cgroup supports charge transfer from
> the very beginning, the userspace can check if the cgroup is mounted
> and if so then it knows this feature is supported.
Has some userspace code for this been written? I'd like to be
convinced that these changes need to be in the binder kernel driver
instead of in userspace.
>
> > This also solves the issue that Greg brought up about
> > userspace needing to know whether the kernel implements this feature
> > (older kernel running with newer userspace). I think we could just
> > reserve some flags for userspace to use (and since those flags are
> > "reserved" for older kernels, this would enable this feature even for
> > old kernels)
> >
> > > > }
> > > > return 0;
> > > > }
> > > > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > > > index 3246f2c74696..169fd5069a1a 100644
> > > > --- a/include/uapi/linux/android/binder.h
> > > > +++ b/include/uapi/linux/android/binder.h
> > > > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> > > >
> > > > enum {
> > > > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > > > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > > > };
> > > >
> > > > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > > > --
> > > > 2.35.1.265.g69c8d7142f-goog
> > > >
> > >
> > > How does userspace know that binder supports this new flag? And where
> > > is the userspace test for this new feature? Isn't there a binder test
> > > framework somewhere?
> > >
> > > thanks,
> > >
> > > greg k-h
On Mon, Feb 14, 2022 at 12:37 PM John Stultz <[email protected]> wrote:
>
> On Mon, Feb 14, 2022 at 12:19 PM Todd Kjos <[email protected]> wrote:
> > On Mon, Feb 14, 2022 at 11:29 AM Suren Baghdasaryan <[email protected]> wrote:
> > > On Mon, Feb 14, 2022 at 10:33 AM Todd Kjos <[email protected]> wrote:
> > > >
> > > > Since we are creating a new gpu cgroup abstraction, couldn't this
> > > > "transfer" be done in userspace by the target instead of in the kernel
> > > > driver? Then this patch would reduce to just a flag on the buffer
> > > > object.
> > >
> > > Are you suggesting to have a userspace accessible cgroup interface for
> > > transferring buffer charges and the target process to use that
> > > interface for requesting the buffer to be charged to its cgroup?
> >
> > Well, I'm asking why we need to do these cgroup-ish actions in the
> > kernel when it seems more natural to do it in userspace.
This was our plan originally i.e. to create a cgroup interface that
userspace could use for explicit charge transfer. However, in our
initial discussions with all interested parties and cgroup maintainers
we reached a conclusion that an explicit charge transfer UAPI for the
cgroup controller did not fit in with the current cgroup
charge/uncharge mechanisms. Like John mentioned, the charge transfer
during binder IPC was suggested by Daniel during LPC.
Regards,
Hridya
> >
>
> In case its useful, some additional context from some of the Linux
> Plumber's discussions last fall:
>
> Daniel Stone outlines some concerns with the cgroup userland handling
> for accounting:
> https://youtu.be/3OqllZONTiQ?t=3430
>
> And the binder ownership transfer bit was suggested here by Daniel Vetter:
> https://youtu.be/3OqllZONTiQ?t=3730
>
> thanks
> -john
On Mon, Feb 14, 2022 at 12:19 PM Todd Kjos <[email protected]> wrote:
> On Mon, Feb 14, 2022 at 11:29 AM Suren Baghdasaryan <[email protected]> wrote:
> > On Mon, Feb 14, 2022 at 10:33 AM Todd Kjos <[email protected]> wrote:
> > >
> > > Since we are creating a new gpu cgroup abstraction, couldn't this
> > > "transfer" be done in userspace by the target instead of in the kernel
> > > driver? Then this patch would reduce to just a flag on the buffer
> > > object.
> >
> > Are you suggesting to have a userspace accessible cgroup interface for
> > transferring buffer charges and the target process to use that
> > interface for requesting the buffer to be charged to its cgroup?
>
> Well, I'm asking why we need to do these cgroup-ish actions in the
> kernel when it seems more natural to do it in userspace.
>
In case its useful, some additional context from some of the Linux
Plumber's discussions last fall:
Daniel Stone outlines some concerns with the cgroup userland handling
for accounting:
https://youtu.be/3OqllZONTiQ?t=3430
And the binder ownership transfer bit was suggested here by Daniel Vetter:
https://youtu.be/3OqllZONTiQ?t=3730
thanks
-john
On Fri, Feb 11, 2022 at 8:19 AM T.J. Mercier <[email protected]> wrote:
>
> This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> that a process sending an fd array to another process over binder IPC
> can set to relinquish ownership of the fds being sent for memory
> accounting purposes. If the flag is found to be set during the fd array
> translation and the fd is for a DMA-BUF, the buffer is uncharged from
> the sender's cgroup and charged to the receiving process's cgroup
> instead.
>
> It is up to the sending process to ensure that it closes the fds
> regardless of whether the transfer failed or succeeded.
>
> Most graphics shared memory allocations in Android are done by the
> graphics allocator HAL process. On requests from clients, the HAL process
> allocates memory and sends the fds to the clients over binder IPC.
> The graphics allocator HAL will not retain any references to the
> buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> correctly charge the buffers to the client processes instead of the
> graphics allocator HAL.
>
> From: Hridya Valsaraju <[email protected]>
> Signed-off-by: Hridya Valsaraju <[email protected]>
> Co-developed-by: T.J. Mercier <[email protected]>
> Signed-off-by: T.J. Mercier <[email protected]>
> ---
> changes in v2
> - 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.
>
> drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> include/uapi/linux/android/binder.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8351c5638880..f50d88ded188 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -42,6 +42,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/dma-buf.h>
> #include <linux/fdtable.h>
> #include <linux/file.h>
> #include <linux/freezer.h>
> @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
Is this only needed for the BINDER_TYPE_FDA case (multiple fds)? This
never needs to be done in the BINDER_TYPE_FD case (single fd)?
> {
> binder_size_t fdi, fd_buf_size;
> binder_size_t fda_offset;
> + bool transfer_gpu_charge = false;
> const void __user *sender_ufda_base;
> struct binder_proc *proc = thread->proc;
> + struct binder_proc *target_proc = t->to_proc;
> int ret;
>
> fd_buf_size = sizeof(u32) * fda->num_fds;
> @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> if (ret)
> return ret;
>
> + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> + transfer_gpu_charge = true;
> +
> for (fdi = 0; fdi < fda->num_fds; fdi++) {
> u32 fd;
> + struct dma_buf *dmabuf;
> + struct gpucg *gpucg;
> +
> binder_size_t offset = fda_offset + fdi * sizeof(fd);
> binder_size_t sender_uoffset = fdi * sizeof(fd);
>
> @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> in_reply_to);
> if (ret)
> return ret > 0 ? -EINVAL : ret;
> +
> + if (!transfer_gpu_charge)
> + continue;
> +
> + dmabuf = dma_buf_get(fd);
> + if (IS_ERR(dmabuf))
> + continue;
> +
> + gpucg = gpucg_get(target_proc->tsk);
> + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> + if (ret) {
> + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> + proc->pid, thread->pid, target_proc->pid);
> + gpucg_put(gpucg);
> + }
> + dma_buf_put(dmabuf);
> }
> return 0;
> }
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index 3246f2c74696..169fd5069a1a 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -137,6 +137,7 @@ struct binder_buffer_object {
>
> enum {
> BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> };
>
> /* struct binder_fd_array_object - object describing an array of fds in a buffer
> --
> 2.35.1.265.g69c8d7142f-goog
>
On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Fri, Feb 11, 2022 at 04:18:29PM +0000, T.J. Mercier wrote:
> > This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> > that a process sending an fd array to another process over binder IPC
> > can set to relinquish ownership of the fds being sent for memory
> > accounting purposes. If the flag is found to be set during the fd array
> > translation and the fd is for a DMA-BUF, the buffer is uncharged from
> > the sender's cgroup and charged to the receiving process's cgroup
> > instead.
> >
> > It is up to the sending process to ensure that it closes the fds
> > regardless of whether the transfer failed or succeeded.
> >
> > Most graphics shared memory allocations in Android are done by the
> > graphics allocator HAL process. On requests from clients, the HAL process
> > allocates memory and sends the fds to the clients over binder IPC.
> > The graphics allocator HAL will not retain any references to the
> > buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> > arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> > correctly charge the buffers to the client processes instead of the
> > graphics allocator HAL.
> >
> > From: Hridya Valsaraju <[email protected]>
> > Signed-off-by: Hridya Valsaraju <[email protected]>
> > Co-developed-by: T.J. Mercier <[email protected]>
> > Signed-off-by: T.J. Mercier <[email protected]>
> > ---
> > changes in v2
> > - 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.
> >
> > drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> > include/uapi/linux/android/binder.h | 1 +
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 8351c5638880..f50d88ded188 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -42,6 +42,7 @@
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > +#include <linux/dma-buf.h>
> > #include <linux/fdtable.h>
> > #include <linux/file.h>
> > #include <linux/freezer.h>
> > @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > {
> > binder_size_t fdi, fd_buf_size;
> > binder_size_t fda_offset;
> > + bool transfer_gpu_charge = false;
> > const void __user *sender_ufda_base;
> > struct binder_proc *proc = thread->proc;
> > + struct binder_proc *target_proc = t->to_proc;
> > int ret;
> >
> > fd_buf_size = sizeof(u32) * fda->num_fds;
> > @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > if (ret)
> > return ret;
> >
> > + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> > + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> > + transfer_gpu_charge = true;
> > +
> > for (fdi = 0; fdi < fda->num_fds; fdi++) {
> > u32 fd;
> > + struct dma_buf *dmabuf;
> > + struct gpucg *gpucg;
> > +
> > binder_size_t offset = fda_offset + fdi * sizeof(fd);
> > binder_size_t sender_uoffset = fdi * sizeof(fd);
> >
> > @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > in_reply_to);
> > if (ret)
> > return ret > 0 ? -EINVAL : ret;
> > +
> > + if (!transfer_gpu_charge)
> > + continue;
> > +
> > + dmabuf = dma_buf_get(fd);
> > + if (IS_ERR(dmabuf))
> > + continue;
> > +
> > + gpucg = gpucg_get(target_proc->tsk);
> > + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> > + if (ret) {
> > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> > + proc->pid, thread->pid, target_proc->pid);
> > + gpucg_put(gpucg);
> > + }
> > + dma_buf_put(dmabuf);
> > }
> > return 0;
> > }
> > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > index 3246f2c74696..169fd5069a1a 100644
> > --- a/include/uapi/linux/android/binder.h
> > +++ b/include/uapi/linux/android/binder.h
> > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> >
> > enum {
> > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > };
> >
> > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > --
> > 2.35.1.265.g69c8d7142f-goog
> >
>
> How does userspace know that binder supports this new flag?
Sorry, I don't completely follow even after Todd's comment. Doesn't
the presence of BINDER_BUFFER_FLAG_SENDER_NO_NEED in the header do
this? So wouldn't userspace need to be compiled against the wrong
kernel headers for there to be a problem? In that case the allocation
would still succeed, but there would be no charge transfer and
unfortunately no error code.
> And where is the userspace test for this new feature?
I tested this on a Pixel after modifying the gralloc implementation to
mark allocated buffers as not used by the sender. This required
setting the BINDER_BUFFER_FLAG_SENDER_NO_NEED in libhwbinder. That
code can be found here:
https://android-review.googlesource.com/c/platform/system/libhwbinder/+/1910752/1/Parcel.cpp
https://android-review.googlesource.com/c/platform/system/libhidl/+/1910611/
Then by inspecting gpu.memory.current files in sysfs I was able to see
the memory attributed to processes other than the graphics allocator
service. Before this change, several megabytes of memory were
attributed to the graphics allocator service but those buffers are
actually used by other processes like surfaceflinger, the camera, etc.
After the change, the gpu.memory.current amount for the graphics
allocator service was 0 and the charges showed up in the
gpu.memory.current files for those other processes like this:
PID: 764 Process Name: zygote64
system 8192
system-uncached 23191552
PID: 529 Process Name: /system/bin/surfaceflinger
system-uncached 109535232
system 92196864
PID: 530 Process Name:
/vendor/bin/hw/[email protected]
system-uncached 0
system 0
sensor_direct_heap 0
PID: 806 Process Name:
/apex/com.google.pixel.camera.hal/bin/hw/[email protected]
system 1196032
PID: 4608 Process Name: com.google.android.GoogleCamera
system 2408448
system-uncached 38887424
sensor_direct_heap 0
PID: 32102 Process Name: com.google.android.googlequicksearchbox:search
system-uncached 91279360
system 20480
PID: 2758 Process Name: com.google.android.youtube
system-uncached 1662976
system 8192
PID: 2517 Process Name: com.google.android.apps.nexuslauncher
system-uncached 115662848
system 122880
PID: 2066 Process Name: com.android.systemui
system 86016
system-uncached 37957632
> Isn't there a binder test framework somewhere?
Android has the Vendor Test Suite where automated tests could be added
for this. Is that what you're thinking of?
>
> thanks,
>
> greg k-h
On Mon, Feb 14, 2022 at 10:33 AM Todd Kjos <[email protected]> wrote:
>
> On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Fri, Feb 11, 2022 at 04:18:29PM +0000, T.J. Mercier wrote:
>
> Title: "android: binder: Add a buffer flag to relinquish ownership of fds"
>
> Please drop the "android:" from the title.
>
> > > This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> > > that a process sending an fd array to another process over binder IPC
> > > can set to relinquish ownership of the fds being sent for memory
> > > accounting purposes. If the flag is found to be set during the fd array
> > > translation and the fd is for a DMA-BUF, the buffer is uncharged from
> > > the sender's cgroup and charged to the receiving process's cgroup
> > > instead.
> > >
> > > It is up to the sending process to ensure that it closes the fds
> > > regardless of whether the transfer failed or succeeded.
> > >
> > > Most graphics shared memory allocations in Android are done by the
> > > graphics allocator HAL process. On requests from clients, the HAL process
> > > allocates memory and sends the fds to the clients over binder IPC.
> > > The graphics allocator HAL will not retain any references to the
> > > buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> > > arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> > > correctly charge the buffers to the client processes instead of the
> > > graphics allocator HAL.
> > >
> > > From: Hridya Valsaraju <[email protected]>
> > > Signed-off-by: Hridya Valsaraju <[email protected]>
> > > Co-developed-by: T.J. Mercier <[email protected]>
> > > Signed-off-by: T.J. Mercier <[email protected]>
> > > ---
> > > changes in v2
> > > - 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.
> > >
> > > drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> > > include/uapi/linux/android/binder.h | 1 +
> > > 2 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index 8351c5638880..f50d88ded188 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -42,6 +42,7 @@
> > >
> > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >
> > > +#include <linux/dma-buf.h>
> > > #include <linux/fdtable.h>
> > > #include <linux/file.h>
> > > #include <linux/freezer.h>
> > > @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > > {
> > > binder_size_t fdi, fd_buf_size;
> > > binder_size_t fda_offset;
> > > + bool transfer_gpu_charge = false;
> > > const void __user *sender_ufda_base;
> > > struct binder_proc *proc = thread->proc;
> > > + struct binder_proc *target_proc = t->to_proc;
> > > int ret;
> > >
> > > fd_buf_size = sizeof(u32) * fda->num_fds;
> > > @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > > if (ret)
> > > return ret;
> > >
> > > + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> > > + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> > > + transfer_gpu_charge = true;
> > > +
> > > for (fdi = 0; fdi < fda->num_fds; fdi++) {
> > > u32 fd;
> > > + struct dma_buf *dmabuf;
> > > + struct gpucg *gpucg;
> > > +
> > > binder_size_t offset = fda_offset + fdi * sizeof(fd);
> > > binder_size_t sender_uoffset = fdi * sizeof(fd);
> > >
> > > @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > > in_reply_to);
> > > if (ret)
> > > return ret > 0 ? -EINVAL : ret;
> > > +
> > > + if (!transfer_gpu_charge)
> > > + continue;
> > > +
> > > + dmabuf = dma_buf_get(fd);
> > > + if (IS_ERR(dmabuf))
> > > + continue;
> > > +
> > > + gpucg = gpucg_get(target_proc->tsk);
> > > + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> > > + if (ret) {
> > > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> > > + proc->pid, thread->pid, target_proc->pid);
> > > + gpucg_put(gpucg);
> > > + }
> > > + dma_buf_put(dmabuf);
>
> Since we are creating a new gpu cgroup abstraction, couldn't this
> "transfer" be done in userspace by the target instead of in the kernel
> driver? Then this patch would reduce to just a flag on the buffer
> object.
Are you suggesting to have a userspace accessible cgroup interface for
transferring buffer charges and the target process to use that
interface for requesting the buffer to be charged to its cgroup?
I'm worried about the case when the target process does not request
the transfer after receiving the buffer with this flag set. The charge
would stay with the wrong process and accounting will be invalid.
Technically, since the proposed cgroup supports charge transfer from
the very beginning, the userspace can check if the cgroup is mounted
and if so then it knows this feature is supported.
> This also solves the issue that Greg brought up about
> userspace needing to know whether the kernel implements this feature
> (older kernel running with newer userspace). I think we could just
> reserve some flags for userspace to use (and since those flags are
> "reserved" for older kernels, this would enable this feature even for
> old kernels)
>
> > > }
> > > return 0;
> > > }
> > > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > > index 3246f2c74696..169fd5069a1a 100644
> > > --- a/include/uapi/linux/android/binder.h
> > > +++ b/include/uapi/linux/android/binder.h
> > > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> > >
> > > enum {
> > > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > > };
> > >
> > > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > > --
> > > 2.35.1.265.g69c8d7142f-goog
> > >
> >
> > How does userspace know that binder supports this new flag? And where
> > is the userspace test for this new feature? Isn't there a binder test
> > framework somewhere?
> >
> > thanks,
> >
> > greg k-h
On Mon, Feb 14, 2022 at 1:26 PM Todd Kjos <[email protected]> wrote:
>
> On Fri, Feb 11, 2022 at 8:19 AM T.J. Mercier <[email protected]> wrote:
> >
> > This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> > that a process sending an fd array to another process over binder IPC
> > can set to relinquish ownership of the fds being sent for memory
> > accounting purposes. If the flag is found to be set during the fd array
> > translation and the fd is for a DMA-BUF, the buffer is uncharged from
> > the sender's cgroup and charged to the receiving process's cgroup
> > instead.
> >
> > It is up to the sending process to ensure that it closes the fds
> > regardless of whether the transfer failed or succeeded.
> >
> > Most graphics shared memory allocations in Android are done by the
> > graphics allocator HAL process. On requests from clients, the HAL process
> > allocates memory and sends the fds to the clients over binder IPC.
> > The graphics allocator HAL will not retain any references to the
> > buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> > arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> > correctly charge the buffers to the client processes instead of the
> > graphics allocator HAL.
> >
> > From: Hridya Valsaraju <[email protected]>
> > Signed-off-by: Hridya Valsaraju <[email protected]>
> > Co-developed-by: T.J. Mercier <[email protected]>
> > Signed-off-by: T.J. Mercier <[email protected]>
> > ---
> > changes in v2
> > - 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.
> >
> > drivers/android/binder.c | 26 ++++++++++++++++++++++++++
> > include/uapi/linux/android/binder.h | 1 +
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 8351c5638880..f50d88ded188 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -42,6 +42,7 @@
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > +#include <linux/dma-buf.h>
> > #include <linux/fdtable.h>
> > #include <linux/file.h>
> > #include <linux/freezer.h>
> > @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head *pf_head,
>
> Is this only needed for the BINDER_TYPE_FDA case (multiple fds)? This
> never needs to be done in the BINDER_TYPE_FD case (single fd)?
>
Currently this is the case as there is no user who would benefit from
the single fd case. The only known user is the gralloc HAL which
always uses BINDER_TYPE_FDA to send dmabufs. I guess we could move the
code into binder_translate_fd if we were willing to bring back
binder_fd_object's flags field. This looks possible, but I think it'd
be a more intrusive change.
> > {
> > binder_size_t fdi, fd_buf_size;
> > binder_size_t fda_offset;
> > + bool transfer_gpu_charge = false;
> > const void __user *sender_ufda_base;
> > struct binder_proc *proc = thread->proc;
> > + struct binder_proc *target_proc = t->to_proc;
> > int ret;
> >
> > fd_buf_size = sizeof(u32) * fda->num_fds;
> > @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > if (ret)
> > return ret;
> >
> > + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> > + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> > + transfer_gpu_charge = true;
> > +
> > for (fdi = 0; fdi < fda->num_fds; fdi++) {
> > u32 fd;
> > + struct dma_buf *dmabuf;
> > + struct gpucg *gpucg;
> > +
> > binder_size_t offset = fda_offset + fdi * sizeof(fd);
> > binder_size_t sender_uoffset = fdi * sizeof(fd);
> >
> > @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head *pf_head,
> > in_reply_to);
> > if (ret)
> > return ret > 0 ? -EINVAL : ret;
> > +
> > + if (!transfer_gpu_charge)
> > + continue;
> > +
> > + dmabuf = dma_buf_get(fd);
> > + if (IS_ERR(dmabuf))
> > + continue;
> > +
> > + gpucg = gpucg_get(target_proc->tsk);
> > + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> > + if (ret) {
> > + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to %d",
> > + proc->pid, thread->pid, target_proc->pid);
> > + gpucg_put(gpucg);
> > + }
> > + dma_buf_put(dmabuf);
> > }
> > return 0;
> > }
> > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > index 3246f2c74696..169fd5069a1a 100644
> > --- a/include/uapi/linux/android/binder.h
> > +++ b/include/uapi/linux/android/binder.h
> > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> >
> > enum {
> > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > };
> >
> > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > --
> > 2.35.1.265.g69c8d7142f-goog
> >
On Mon, Feb 14, 2022 at 02:25:47PM -0800, T.J. Mercier wrote:
> On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
> > > --- a/include/uapi/linux/android/binder.h
> > > +++ b/include/uapi/linux/android/binder.h
> > > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> > >
> > > enum {
> > > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > > };
> > >
> > > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > > --
> > > 2.35.1.265.g69c8d7142f-goog
> > >
> >
> > How does userspace know that binder supports this new flag?
>
> Sorry, I don't completely follow even after Todd's comment. Doesn't
> the presence of BINDER_BUFFER_FLAG_SENDER_NO_NEED in the header do
> this?
There is no "header" when running a new kernel on an old userspace,
right? How about the other way around, old kernel, new userspace?
> So wouldn't userspace need to be compiled against the wrong
> kernel headers for there to be a problem? In that case the allocation
> would still succeed, but there would be no charge transfer and
> unfortunately no error code.
No error code is not good. People upgrade their kernels all the time,
and do not do a "rebuild the world" when doing so.
> > And where is the userspace test for this new feature?
>
> I tested this on a Pixel after modifying the gralloc implementation to
> mark allocated buffers as not used by the sender. This required
> setting the BINDER_BUFFER_FLAG_SENDER_NO_NEED in libhwbinder. That
> code can be found here:
> https://android-review.googlesource.com/c/platform/system/libhwbinder/+/1910752/1/Parcel.cpp
> https://android-review.googlesource.com/c/platform/system/libhidl/+/1910611/
>
> Then by inspecting gpu.memory.current files in sysfs I was able to see
> the memory attributed to processes other than the graphics allocator
> service. Before this change, several megabytes of memory were
> attributed to the graphics allocator service but those buffers are
> actually used by other processes like surfaceflinger, the camera, etc.
> After the change, the gpu.memory.current amount for the graphics
> allocator service was 0 and the charges showed up in the
> gpu.memory.current files for those other processes like this:
>
> PID: 764 Process Name: zygote64
> system 8192
> system-uncached 23191552
>
> PID: 529 Process Name: /system/bin/surfaceflinger
> system-uncached 109535232
> system 92196864
>
> PID: 530 Process Name:
> /vendor/bin/hw/[email protected]
> system-uncached 0
> system 0
> sensor_direct_heap 0
>
> PID: 806 Process Name:
> /apex/com.google.pixel.camera.hal/bin/hw/[email protected]
> system 1196032
>
> PID: 4608 Process Name: com.google.android.GoogleCamera
> system 2408448
> system-uncached 38887424
> sensor_direct_heap 0
>
> PID: 32102 Process Name: com.google.android.googlequicksearchbox:search
> system-uncached 91279360
> system 20480
>
> PID: 2758 Process Name: com.google.android.youtube
> system-uncached 1662976
> system 8192
>
> PID: 2517 Process Name: com.google.android.apps.nexuslauncher
> system-uncached 115662848
> system 122880
>
> PID: 2066 Process Name: com.android.systemui
> system 86016
> system-uncached 37957632
>
> > Isn't there a binder test framework somewhere?
>
> Android has the Vendor Test Suite where automated tests could be added
> for this. Is that what you're thinking of?
tools/testing/selftests/ is a good start. VTS is the worst-case as no
one can really run that on their own, but it is better than nothing.
Having no test at all for this is not ok.
thanks,
greg k-h
On Mon, Feb 14, 2022 at 11:19:35PM -0800, Suren Baghdasaryan wrote:
> On Mon, Feb 14, 2022 at 11:01 PM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Mon, Feb 14, 2022 at 02:25:47PM -0800, T.J. Mercier wrote:
> > > On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
> > > > > --- a/include/uapi/linux/android/binder.h
> > > > > +++ b/include/uapi/linux/android/binder.h
> > > > > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> > > > >
> > > > > enum {
> > > > > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > > > > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > > > > };
> > > > >
> > > > > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > > > > --
> > > > > 2.35.1.265.g69c8d7142f-goog
> > > > >
> > > >
> > > > How does userspace know that binder supports this new flag?
> > >
> > > Sorry, I don't completely follow even after Todd's comment. Doesn't
> > > the presence of BINDER_BUFFER_FLAG_SENDER_NO_NEED in the header do
> > > this?
> >
> > There is no "header" when running a new kernel on an old userspace,
> > right? How about the other way around, old kernel, new userspace?
>
> 1. new kernel + old userspace = kernel supports the feature but
> userspace does not use it. The old userspace won't even mount the new
> cgroup controller, accounting is not performed, charge is not
> transferred.
> 2. old kernel + new userspace = the new cgroup controller is not
> supported by the kernel, accounting is not performed, charge is not
> transferred.
> 3. old kernel + old userspace = same as #2
> 4. new kernel + new userspace = cgroup is mounted, feature is
> supported and used.
> Does that work or do we need a separate indication of whether binder
> driver supports the charge transfer feature?
Ok, if that's all working, this is fine, it just seemed odd to add a new
type like this. Perhaps this can go into the changelog text...
thanks,
greg k-h
On Mon, Feb 14, 2022 at 11:01 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Mon, Feb 14, 2022 at 02:25:47PM -0800, T.J. Mercier wrote:
> > On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
> > > > --- a/include/uapi/linux/android/binder.h
> > > > +++ b/include/uapi/linux/android/binder.h
> > > > @@ -137,6 +137,7 @@ struct binder_buffer_object {
> > > >
> > > > enum {
> > > > BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> > > > + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> > > > };
> > > >
> > > > /* struct binder_fd_array_object - object describing an array of fds in a buffer
> > > > --
> > > > 2.35.1.265.g69c8d7142f-goog
> > > >
> > >
> > > How does userspace know that binder supports this new flag?
> >
> > Sorry, I don't completely follow even after Todd's comment. Doesn't
> > the presence of BINDER_BUFFER_FLAG_SENDER_NO_NEED in the header do
> > this?
>
> There is no "header" when running a new kernel on an old userspace,
> right? How about the other way around, old kernel, new userspace?
1. new kernel + old userspace = kernel supports the feature but
userspace does not use it. The old userspace won't even mount the new
cgroup controller, accounting is not performed, charge is not
transferred.
2. old kernel + new userspace = the new cgroup controller is not
supported by the kernel, accounting is not performed, charge is not
transferred.
3. old kernel + old userspace = same as #2
4. new kernel + new userspace = cgroup is mounted, feature is
supported and used.
Does that work or do we need a separate indication of whether binder
driver supports the charge transfer feature?
>
> > So wouldn't userspace need to be compiled against the wrong
> > kernel headers for there to be a problem? In that case the allocation
> > would still succeed, but there would be no charge transfer and
> > unfortunately no error code.
>
> No error code is not good. People upgrade their kernels all the time,
> and do not do a "rebuild the world" when doing so.
>
> > > And where is the userspace test for this new feature?
> >
> > I tested this on a Pixel after modifying the gralloc implementation to
> > mark allocated buffers as not used by the sender. This required
> > setting the BINDER_BUFFER_FLAG_SENDER_NO_NEED in libhwbinder. That
> > code can be found here:
> > https://android-review.googlesource.com/c/platform/system/libhwbinder/+/1910752/1/Parcel.cpp
> > https://android-review.googlesource.com/c/platform/system/libhidl/+/1910611/
> >
> > Then by inspecting gpu.memory.current files in sysfs I was able to see
> > the memory attributed to processes other than the graphics allocator
> > service. Before this change, several megabytes of memory were
> > attributed to the graphics allocator service but those buffers are
> > actually used by other processes like surfaceflinger, the camera, etc.
> > After the change, the gpu.memory.current amount for the graphics
> > allocator service was 0 and the charges showed up in the
> > gpu.memory.current files for those other processes like this:
> >
> > PID: 764 Process Name: zygote64
> > system 8192
> > system-uncached 23191552
> >
> > PID: 529 Process Name: /system/bin/surfaceflinger
> > system-uncached 109535232
> > system 92196864
> >
> > PID: 530 Process Name:
> > /vendor/bin/hw/[email protected]
> > system-uncached 0
> > system 0
> > sensor_direct_heap 0
> >
> > PID: 806 Process Name:
> > /apex/com.google.pixel.camera.hal/bin/hw/[email protected]
> > system 1196032
> >
> > PID: 4608 Process Name: com.google.android.GoogleCamera
> > system 2408448
> > system-uncached 38887424
> > sensor_direct_heap 0
> >
> > PID: 32102 Process Name: com.google.android.googlequicksearchbox:search
> > system-uncached 91279360
> > system 20480
> >
> > PID: 2758 Process Name: com.google.android.youtube
> > system-uncached 1662976
> > system 8192
> >
> > PID: 2517 Process Name: com.google.android.apps.nexuslauncher
> > system-uncached 115662848
> > system 122880
> >
> > PID: 2066 Process Name: com.android.systemui
> > system 86016
> > system-uncached 37957632
> >
> > > Isn't there a binder test framework somewhere?
> >
> > Android has the Vendor Test Suite where automated tests could be added
> > for this. Is that what you're thinking of?
>
> tools/testing/selftests/ is a good start. VTS is the worst-case as no
> one can really run that on their own, but it is better than nothing.
> Having no test at all for this is not ok.
>
> thanks,
>
> greg k-h