Re: [RFC v2 6/6] android: binder: Add a buffer flag to relinquish ownership of fds
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
> 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
Re: [RFC v2 6/6] android: binder: Add a buffer flag to relinquish ownership of fds
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/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?
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
Re: [RFC v2 6/6] android: binder: Add a buffer flag to relinquish ownership of fds
On Mon, Feb 14, 2022 at 12:19 PM Todd Kjos wrote: > On Mon, Feb 14, 2022 at 11:29 AM Suren Baghdasaryan wrote: > > On Mon, Feb 14, 2022 at 10:33 AM Todd Kjos 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
Re: [RFC v2 6/6] android: binder: Add a buffer flag to relinquish ownership of fds
On Fri, Feb 11, 2022 at 04:18:29PM +, 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
> Signed-off-by: Hridya Valsaraju
> Co-developed-by: T.J. Mercier
> Signed-off-by: T.J. Mercier
> ---
> 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
> #include
> #include
> #include
> @@ -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
