On Fri, May 03, 2024 at 04:16:15PM -0700, Linus Torvalds wrote:
On Fri, 3 May 2024 at 15:07, Al Viro viro@zeniv.linux.org.uk wrote:
Suppose your program calls select() on a pipe and dmabuf, sees data to be read from pipe, reads it, closes both pipe and dmabuf and exits.
Would you expect that dmabuf file would stick around for hell knows how long after that? I would certainly be very surprised by running into that...
Why?
That's the _point_ of refcounts. They make the thing they refcount stay around until it's no longer referenced.
Now, I agree that dmabuf's are a bit odd in how they use a 'struct file' *as* their refcount, but hey, it's a specialty use. Unusual perhaps, but not exactly wrong.
I suspect that if you saw a dmabuf just have its own 'refcount_t' and stay around until it was done, you wouldn't bat an eye at it, and it's really just the "it uses a struct file for counting" that you are reacting to.
*IF* those files are on purely internal filesystem, that's probably OK; do that with something on something mountable (char device, sysfs file, etc.) and you have a problem with filesystem staying busy.
I'm really unfamiliar with the subsystem; it might be OK with all objects that use that for ->poll(), but that's definitely not a good thing to see in ->poll() instance in general. And code gets copied, so there really should be a big fat comment about the reasons why it's OK in this particular case.
Said that, it seems that a better approach might be to have their ->release() cancel callbacks and drop fence references. Note that they *do* have refcounts - on fences. The file (well, dmabuf, really) is pinned only to protect against the situation when pending callback is still around. And Kees' observation about multiple fences is also interesting - we don't get extra fput(), but only because we get events only from one fence, which does look fishy...