Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220516171315.2400578-1-tjmercier@google.com>
 <175c5af3-9224-9c8e-0784-349dad9a2954@amd.com> <CABdmKX2GcgCs1xANYPBp8OEtk9qqH7AvCzpdppj9rHXvMqWSAw@mail.gmail.com>
 <0875fa95-3a25-a354-1433-201fca81ed3e@amd.com> <CABdmKX1+VYfdzyVYOS5MCsr4ptGTygmuUP9ikyh-vW6DgKk2kg@mail.gmail.com>
 <YoM9BAwybcjG7K/H@kroah.com> <d820893c-fa2e-3bac-88be-f39c06d89c01@amd.com>
 <CABdmKX2m1b1kdACKM19S+u9uR5RTy1UGMRgd+3QA_oAyCpeggg@mail.gmail.com> <7f895a99-adfa-bcbd-c130-a924c668b8af@amd.com>
In-Reply-To: <7f895a99-adfa-bcbd-c130-a924c668b8af@amd.com>
From:   "T.J. Mercier" <tjmercier@google.com>
Date:   Thu, 19 May 2022 15:58:36 -0700
Message-ID: <CABdmKX0XLvRZvXyiN0P_B-fUACiF5xwQ07+u_gaR+hDhu_x_TA@mail.gmail.com>
Subject: Re: [PATCH v2] dma-buf: Move sysfs work out of DMA-BUF export path
To:     =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Suren Baghdasaryan <surenb@google.com>,
        Kalesh Singh <kaleshsingh@google.com>,
        Minchan Kim <minchan@google.com>,
        Greg Kroah-Hartman <gregkh@google.com>,
        John Stultz <jstultz@google.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Hridya Valsaraju <hridya@google.com>, kernel-team@android.com,
        linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 18, 2022 at 5:03 AM Christian K=C3=B6nig
<christian.koenig@amd.com> wrote:
>
> Am 18.05.22 um 01:09 schrieb T.J. Mercier:
> > [SNIP]
> >>> Perhaps we should go just one step further and make a misc device nod=
e
> >>> for dmabug debugging information to be in and just have userspace
> >>> poll/read on the device node and we spit the info that used to be in
> >>> debugfs out through that?  That way this only affects systems when th=
ey
> >>> want to read the information and not normal code paths?  Yeah that's =
a
> >>> hack, but this whole thing feels overly complex now.
> >> Yeah, totally agree on the complexity note. I'm just absolutely not ke=
en
> >> to add hack over hack over hack to make something work which from my
> >> point of view has some serious issues with it's design.
> >>
> > Why is this patch a hack? We found a problem with the initial design
> > which nobody saw when it was originally created, and now we're trying
> > to address it within the constraints that exist.
>
> Well the issue is that you don't try to tackle the underlying problem,
> but rather just mitigate the unforeseen consequences. That is pretty
> clearly a hack to me.
>
> > Is there some other
> > solution to the problem of exports getting blocked that you would
> > suggest here?
>
> Well pretty much the same as Greg outlined as well. Go back to your
> drawing board and come back with a solution which does not need such
> workarounds.
>
> Alternatively you can give me a full overview of the design and explain
> why exactly that interface here is necessary in exactly that form.
>
We ended up here because we could not use debugfs.
https://source.android.com/setup/start/android-11-release#debugfs

Another idea was adding per-buffer stats to procfs, but that was not
an option since per-buffer stats are not process specific.

So it seemed like sysfs was an appropriate solution at the time. It
comes with a stable interface as a bonus, but with the limitation of 1
value per file this leads to creating lots of files in sysfs for all
dma buffers. This leads to increased kernfs lock contention, and
unfortunately we try to take the lock on the hot path.

With the description and links to the userspace code which actually
uses the feature I feel like that's a complete picture of what's
currently happening with this interface. If you could explain what
information is missing I'll do my best to provide it.
https://cs.android.com/android/platform/superproject/+/master:system/memory=
/libmeminfo/libdmabufinfo/dmabuf_sysfs_stats.cpp;l=3D55;drc=3D3f7db7aec60bb=
ba14c6dd81600820ed62fe3f475;bpv=3D0;bpt=3D1
https://cs.android.com/android/platform/superproject/+/master:system/memory=
/libmeminfo/libdmabufinfo/tools/dmabuf_dump.cpp;l=3D182;drc=3D3f7db7aec60bb=
ba14c6dd81600820ed62fe3f475;bpv=3D0;bpt=3D1
https://cs.android.com/android/platform/superproject/+/master:frameworks/na=
tive/cmds/dumpstate/dumpstate.cpp;drc=3D621533f895b30ca281e79a7a7c03eefd352=
359b5;l=3D1832-1833

> >> For example trying to do accounting based on DMA-bufs is extremely
> >> questionable to begin with. See a modern game for example can have
> >> between 10k and 100k of different buffers, reserving one file descript=
or
> >> for each of those objects is absolutely not going to work.
> >>
> >> So my request is to please describe your full use case and not just wh=
y
> >> you think this patch is justified.
> >>
> > The use case was described in the commit message when the feature was
> > initially added (after discussion about it on the list) including
> > links to code that uses the feature:
> > https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flor=
e.kernel.org%2Fall%2F20210603214758.2955251-1-hridya%40google.com%2F&amp;da=
ta=3D05%7C01%7Cchristian.koenig%40amd.com%7C3f6e3e98fc6f45ead80d08da385a59e=
6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637884257979664387%7CUnknown=
%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6=
Mn0%3D%7C3000%7C%7C%7C&amp;sdata=3Di%2BSfpB%2B6iBolBHu6KrKH3njq0zo1SBbrKDHZ=
OjpsIks%3D&amp;reserved=3D0
>
> Yeah and to be honest I have the strong feeling now that this was
> absolutely not well thought through.

I'm open to working on a replacement for this if we can't find an
acceptable solution here, but I would appreciate some direction on
what would be acceptable. For example Greg's idea sounds workable, but
the question is if it mergeable?

> This description as well as the in
> kernel documentation are not even remotely sufficient to explain what
> you guys are doing with this.
>
> So please come up with a complete design description for both userspace
> and kernel why this interface here is necessary inside the upstream kerne=
l.
>
> If you can't convince me that this is a good idea I will just bluntly
> mark this DMA-buf sysfs interface as deprecated.
>
> Regards,
> Christian.
>
> >
> >
> >> Regards,
> >> Christian.
> >>
> >>> thanks,
> >>>
> >>> greg k-h
>