Re: [PATCH v2] dma-buf: Move sysfs work out of DMA-BUF export path

["Christian König" <christian.koenig@amd.com>]

Am 17.05.22 um 08:13 schrieb Greg Kroah-Hartman:
> On Mon, May 16, 2022 at 05:08:05PM -0700, T.J. Mercier wrote:
>> [SNIP]
>>>>>> Fixes: bdb8d06dfefd ("dmabuf: Add the capability to expose DMA-BUF stats in sysfs")
>>>>>> Originally-by: Hridya Valsaraju <hridya@google.com>
>>>>>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
>>>>>>
>>>>>> ---
>>>>>> See the originally submitted patch by Hridya Valsaraju here:
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2022%2F1%2F4%2F1066&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C61d7d3acbe5f47c7d0e608da37cc5ed7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637883648212878440%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HdSHA2vbBkBgdKxPXIp57EHW49yoMjgmigkVOKeTasI%3D&amp;reserved=0
>>>>>>
>>>>>> v2 changes:
>>>>>> - Defer only sysfs creation instead of creation and teardown per
>>>>>> Christian König
>>>>>>
>>>>>> - Use a work queue instead of a kthread for deferred work per
>>>>>> Christian König
>>>>>> ---
>>>>>>     drivers/dma-buf/dma-buf-sysfs-stats.c | 56 ++++++++++++++++++++-------
>>>>>>     include/linux/dma-buf.h               | 14 ++++++-
>>>>>>     2 files changed, 54 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c
>>>>>> index 2bba0babcb62..67b0a298291c 100644
>>>>>> --- a/drivers/dma-buf/dma-buf-sysfs-stats.c
>>>>>> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>>     #include <linux/printk.h>
>>>>>>     #include <linux/slab.h>
>>>>>>     #include <linux/sysfs.h>
>>>>>> +#include <linux/workqueue.h>
>>>>>>
>>>>>>     #include "dma-buf-sysfs-stats.h"
>>>>>>
>>>>>> @@ -168,10 +169,46 @@ void dma_buf_uninit_sysfs_statistics(void)
>>>>>>         kset_unregister(dma_buf_stats_kset);
>>>>>>     }
>>>>>>
>>>>>> +static void sysfs_add_workfn(struct work_struct *work)
>>>>>> +{
>>>>>> +     struct dma_buf_sysfs_entry *sysfs_entry =
>>>>>> +             container_of(work, struct dma_buf_sysfs_entry, sysfs_add_work);
>>>>>> +     struct dma_buf *dmabuf = sysfs_entry->dmabuf;
>>>>>> +
>>>>>> +     /*
>>>>>> +      * A dmabuf is ref-counted via its file member. If this handler holds the only
>>>>>> +      * reference to the dmabuf, there is no need for sysfs kobject creation. This is an
>>>>>> +      * optimization and a race; when the reference count drops to 1 immediately after
>>>>>> +      * this check it is not harmful as the sysfs entry will still get cleaned up in
>>>>>> +      * dma_buf_stats_teardown, which won't get called until the final dmabuf reference
>>>>>> +      * is released, and that can't happen until the end of this function.
>>>>>> +      */
>>>>>> +     if (file_count(dmabuf->file) > 1) {
>>>>> Please completely drop that. I see absolutely no justification for this
>>>>> additional complexity.
>>>>>
>>>> This case gets hit around 5% of the time in my testing so the else is
>>>> not a completely unused branch.
>>> Well I can only repeat myself: This means that your userspace is
>>> severely broken!
>>>
>>> DMA-buf are meant to be long living objects
>> This patch addresses export *latency* regardless of how long-lived the
>> object is. Even a single, long-lived export will benefit from this
>> change if it would otherwise be blocked on adding an object to sysfs.
>> I think attempting to improve this latency still has merit.
> Fixing the latency is nice, but as it's just pushing the needed work off
> to another code path, it will take longer overall for the sysfs stuff to
> be ready for userspace to see.
>
> Perhaps we need to step back and understand what this code is supposed
> to be doing.  As I recall, it was created because some systems do not
> allow debugfs anymore, and they wanted the debugging information that
> the dmabuf code was exposing to debugfs on a "normal" system.  Moving
> that logic to sysfs made sense, but now I am wondering why we didn't see
> these issues in the debugfs code previously?

Well, I think that some key information is that adding the sysfs support 
was justified with the argument that this is not only used for debugging.

If it would be used only for debugging then debugfs would the right 
choice for this. If debugfs is then not available in your environment 
then you should *not* ask the kernel to work around that. Instead we 
should discuss why you want to disable some debugging access, but not 
all of that.

So for now let's assume that this is also used for accounting, e.g. when 
userspace wants to know how many DMA-bufs of which size are flying 
around to make decisions like which process to put into background or 
which to swap out based on that information.

> Perhaps we should go just one step further and make a misc device node
> 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 they
> 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 keen 
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.

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 descriptor 
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 why 
you think this patch is justified.

Regards,
Christian.

>
> thanks,
>
> greg k-h