Re: [RFC v2 6/6] android: binder: Add a buffer flag to relinquish ownership of fds

[T.J. Mercier]

On Fri, Feb 11, 2022 at 11:19 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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 <hridya@xxxxxxxxxx>
> > Signed-off-by: Hridya Valsaraju <hridya@xxxxxxxxxx>
> > Co-developed-by: T.J. Mercier <tjmercier@xxxxxxxxxx>
> > Signed-off-by: T.J. Mercier <tjmercier@xxxxxxxxxx>
> > ---
> > 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/android.hardware.graphics.allocator@4.0-service
system-uncached 0
system 0
sensor_direct_heap 0

PID: 806 Process Name:
/apex/com.google.pixel.camera.hal/bin/hw/android.hardware.camera.provider@2.7-service-google
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