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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org