Messages in this thread | | | Date | Tue, 10 May 2022 14:52:54 +0200 | Subject | Re: [Linaro-mm-sig] Re: [PATCH] dmabuf: ensure unique directory name for dmabuf stats | From | 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@lists.linaro.org > To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org
| |