Re: [Linaro-mm-sig] Re: [PATCH] dmabuf: ensure unique directory name for dmabuf stats

[Christian König]

Am 10.05.22 um 14:10 schrieb Greg KH:
> On Tue, May 10, 2022 at 01:35:41PM +0200, Christian König wrote:
> > Am 10.05.22 um 13:00 schrieb Greg KH:
> > > On Tue, May 10, 2022 at 03:53:32PM +0530, Charan Teja Kalla wrote:
> > > > The dmabuf file uses get_next_ino()(through dma_buf_getfile() ->
> > > > alloc_anon_inode()) to get an inode number and uses the same as a
> > > > directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is
> > > > used to collect the dmabuf stats and it is created through
> > > > dma_buf_stats_setup(). At current, failure to create this directory
> > > > entry can make the dma_buf_export() to fail.
> > > >
> > > > Now, as the get_next_ino() can definitely give a repetitive inode no
> > > > causing the directory entry creation to fail with -EEXIST. This is a
> > > > problem on the systems where dmabuf stats functionality is enabled on
> > > > the production builds can make the dma_buf_export(), though the dmabuf
> > > > memory is allocated successfully, to fail just because it couldn't
> > > > create stats entry.
> > > Then maybe we should not fail the creation path of the kobject fails to
> > > be created?  It's just for debugging, it should be fine if the creation
> > > of it isn't there.
> > Well if it's just for debugging then it should be under debugfs and not
> > sysfs.
> I'll note that the original patch series for this described why this was
> moved from debugfs to sysfs.
>
> > > > This issue we are able to see on the snapdragon system within 13 days
> > > > where there already exists a directory with inode no "122602" so
> > > > dma_buf_stats_setup() failed with -EEXIST as it is trying to create
> > > > the same directory entry.
> > > >
> > > > To make the directory entry as unique, append the inode creation time to
> > > > the inode. With this change the stats directory entries will be in the
> > > > format of: /sys/kernel/dmabuf/buffers/<inode no>-<inode creation time in
> > > > secs>.
> > > As you are changing the format here, shouldn't the Documentation/ABI/
> > > entry for this also be changed?
> > As far as I can see that is even an UAPI break, not sure if we can allow
> > that.
> Why?  Device names change all the time and should never be static.  A
> buffer name should just be a unique identifier in that directory, that's
> all.  No rules on the formatting of it unless for some reason the name
> being the inode number was somehow being used in userspace for that
> number?

My impression was that we documented that should have been a number, but 
I might be wrong on this. And if it's not documented to be a number, I 
think it should be.

The background is that you probably need to associate the DMA-buf with 
some userspace structure for accounting and that becomes easier when you 
can just put them into a radix.

Regards,
Christian.

> thanks,
>
> greg k-h
> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@xxxxxxxxxxxxxxxx
> To unsubscribe send an email to linaro-mm-sig-leave@xxxxxxxxxxxxxxxx