linux-kernel - Re: [PATCH] dma-buf: Move sysfs work out of DMA-BUF export/release path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3610ecd0-03c7-2cae-8f36-f8fd555757b0@amd.com>
Date:   Tue, 11 Jan 2022 12:43:08 +0100
From:   Christian König <christian.koenig@....com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        Hridya Valsaraju <hridya@...gle.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org,
        john.stultz@...aro.org, surenb@...gle.com, kaleshsingh@...gle.com,
        tjmercier@...gle.com, keescook@...gle.com
Subject: Re: [PATCH] dma-buf: Move sysfs work out of DMA-BUF export/release
 path


Am 11.01.22 um 12:16 schrieb Greg Kroah-Hartman:
> On Tue, Jan 11, 2022 at 11:58:07AM +0100, Christian König wrote:
>>>> This is also not a problem due to the high number of DMA-BUF
>>>> exports during launch time, as even a single export can be delayed for
>>>> an unpredictable amount of time. We cannot eliminate DMA-BUF exports
>>>> completely during app-launches and we are unfortunately seeing reports
>>>> of the exporting process occasionally sleeping long enough to cause
>>>> user-visible jankiness :(
>>>>
>>>> We also looked at whether any optimizations are possible from the
>>>> kernfs implementation side[1] but the semaphore is used quite extensively
>>>> and it looks like the best way forward would be to remove sysfs
>>>> creation/teardown from the DMA-BUF export/release path altogether. We
>>>> have some ideas on how we can reduce the code-complexity in the
>>>> current patch. If we manage to
>>>> simplify it considerably, would the approach of offloading sysfs
>>>> creation and teardown into a separate thread be acceptable Christian?
>> At bare minimum I suggest to use a work_struct instead of re-inventing that
>> with kthread.
>>
>> And then only put the exporting of buffers into the background and not the
>> teardown.
>>
>>>> Thank you for the guidance!
>>> One worry I have here with doing this async that now userspace might
>>> have a dma-buf, but the sysfs entry does not yet exist, or the dma-buf
>>> is gone, but the sysfs entry still exists. That's a bit awkward wrt
>>> semantics.
>>>
>>> Also I'm pretty sure that if we can hit this, then other subsystems
>>> using kernfs have similar problems, so trying to fix this in kernfs
>>> with slightly more fine-grained locking sounds like a much more solid
>>> approach. The linked patch talks about how the big delays happen due
>>> to direct reclaim, and that might be limited to specific code paths
>>> that we need to look at? As-is this feels a bit much like papering
>>> over kernfs issues in hackish ways in sysfs users, instead of tackling
>>> the problem at its root.
>> Which is exactly my feeling as well, yes.
> More and more people are using sysfs/kernfs now for things that it was
> never designed for (i.e. high-speed statistic gathering).  That's not
> the fault of kernfs, it's the fault of people thinking it can be used
> for stuff like that :)

I'm starting to get the feeling that we should maybe have questioned 
adding sysfs files for each exported DMA-buf a bit more. Anyway, to late 
for that. We have to live with the consequences.

> But delays like this is odd, tearing down sysfs attributes should
> normally _never_ be a fast-path that matters to system throughput.  So
> offloading it to a workqueue makes sense as the attributes here are for
> objects that are on the fast-path.

That's what is puzzling me as well. As far as I understood Hridya 
tearing down things is not the problem, because during teardown we 
usually have a dying task where it's usually not much of a problem if 
the corpse is around for another few milliseconds until everything is 
cleaned up.

The issue happens during creation of the sysfs attribute and that's 
extremely odd because if this waits for reclaim then drivers will 
certainly wait for reclaim as well. See we need a few bytes for the 
sysfs attribute, but drivers usually need a few megabytes for the 
DMA-buf backing store before they can even export the DMA-buf.

So something doesn't add up in the rational for this problem.

Regards,
Christian.

>
> thanks,
>
> greg k-h