| Message ID | 1652191562-18700-1-git-send-email-quic_charante@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] dmabuf: ensure unique directory name for dmabuf stats | expand |
Am 10.05.22 um 16:06 schrieb Charan Teja Kalla: > 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. > > 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 unique_id for every > inode. With this change the stats directory entries will be in the > format of: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>. > > Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> > --- > Changes in V2: > -- Used the atomic64_t variable to generate a unique_id to be appended to inode > to have an unique directory with name <inode_number-unique_id> -- Suggested by christian > -- Updated the ABI documentation -- Identified by Greg. > -- Massaged the commit log. > > Changes in V1: > -- Used the inode->i_ctime->tv_secs as an id appended to inode to create the > unique directory with name <inode_number-time_in_secs>. > -- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante@quicinc.com/ > > Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers | 10 +++++----- > drivers/dma-buf/Kconfig | 6 +++--- > drivers/dma-buf/dma-buf-sysfs-stats.c | 8 +++++--- > 3 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > index 5d3bc99..9fffbd3 100644 > --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > @@ -4,19 +4,19 @@ KernelVersion: v5.13 > Contact: Hridya Valsaraju <hridya@google.com> > Description: The /sys/kernel/dmabuf/buffers directory contains a > snapshot of the internal state of every DMA-BUF. > - /sys/kernel/dmabuf/buffers/<inode_number> will contain the > - statistics for the DMA-BUF with the unique inode number > - <inode_number> > + /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will > + contain the statistics for the DMA-BUF with the unique > + pair <inode_number-unique_id> > Users: kernel memory tuning/debugging tools > > -What: /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name > +What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name > Date: May 2021 > KernelVersion: v5.13 > Contact: Hridya Valsaraju <hridya@google.com> > Description: This file is read-only and contains the name of the exporter of > the DMA-BUF. > > -What: /sys/kernel/dmabuf/buffers/<inode_number>/size > +What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size > Date: May 2021 > KernelVersion: v5.13 > Contact: Hridya Valsaraju <hridya@google.com> > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > index 541efe0..5bcbdb1 100644 > --- a/drivers/dma-buf/Kconfig > +++ b/drivers/dma-buf/Kconfig > @@ -81,9 +81,9 @@ menuconfig DMABUF_SYSFS_STATS > Choose this option to enable DMA-BUF sysfs statistics > in location /sys/kernel/dmabuf/buffers. > > - /sys/kernel/dmabuf/buffers/<inode_number> will contain > - statistics for the DMA-BUF with the unique inode number > - <inode_number>. > + /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will contain > + statistics for the DMA-BUF with the unique pair > + <inode_number-unique_id>. > > source "drivers/dma-buf/heaps/Kconfig" > > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c > index 2bba0ba..29e9e23 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.c > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -38,8 +38,8 @@ > * > * The following stats are exposed by the interface: > * > - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name`` > - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size`` > + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name`` > + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size`` > * > * The information in the interface can also be used to derive per-exporter > * statistics. The data from the interface can be gathered on error conditions > @@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > { > struct dma_buf_sysfs_entry *sysfs_entry; > int ret; > + static atomic64_t unique_id = ATOMIC_INIT(0); Please move that to the beginning of the declarations. > > if (!dmabuf || !dmabuf->file) > return -EINVAL; > @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > > /* create the directory for buffer stats */ > ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, > - "%lu", file_inode(dmabuf->file)->i_ino); > + "%lu-%lu", file_inode(dmabuf->file)->i_ino, Why not just use the unique value here? Or is the inode number necessary for something? Regards, Christian. > + atomic64_add_return(1, &unique_id)); > if (ret) > goto err_sysfs_dmabuf; >
On Tue, May 10, 2022 at 7:07 AM Charan Teja Kalla <quic_charante@quicinc.com> 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. > > 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 unique_id for every > inode. With this change the stats directory entries will be in the > format of: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>. > > Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> > --- > Changes in V2: > -- Used the atomic64_t variable to generate a unique_id to be appended to inode > to have an unique directory with name <inode_number-unique_id> -- Suggested by christian > -- Updated the ABI documentation -- Identified by Greg. > -- Massaged the commit log. > > Changes in V1: > -- Used the inode->i_ctime->tv_secs as an id appended to inode to create the > unique directory with name <inode_number-time_in_secs>. > -- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante@quicinc.com/ > > Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers | 10 +++++----- > drivers/dma-buf/Kconfig | 6 +++--- > drivers/dma-buf/dma-buf-sysfs-stats.c | 8 +++++--- > 3 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > index 5d3bc99..9fffbd3 100644 > --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > @@ -4,19 +4,19 @@ KernelVersion: v5.13 > Contact: Hridya Valsaraju <hridya@google.com> > Description: The /sys/kernel/dmabuf/buffers directory contains a > snapshot of the internal state of every DMA-BUF. > - /sys/kernel/dmabuf/buffers/<inode_number> will contain the > - statistics for the DMA-BUF with the unique inode number > - <inode_number> > + /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will > + contain the statistics for the DMA-BUF with the unique > + pair <inode_number-unique_id> Android userspace does have a dependency on this being an inode number. Or at least, a single unsigned int. Not the end of the world, but still... this will break. https://cs.android.com/android/platform/superproject/+/master:system/memory/libmeminfo/libdmabufinfo/dmabuf_sysfs_stats.cpp;l=76-77;drc=6951984bbefb96423970b82005ae381065e36704 > Users: kernel memory tuning/debugging tools > > -What: /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name > +What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name > Date: May 2021 > KernelVersion: v5.13 > Contact: Hridya Valsaraju <hridya@google.com> > Description: This file is read-only and contains the name of the exporter of > the DMA-BUF. > > -What: /sys/kernel/dmabuf/buffers/<inode_number>/size > +What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size > Date: May 2021 > KernelVersion: v5.13 > Contact: Hridya Valsaraju <hridya@google.com> > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > index 541efe0..5bcbdb1 100644 > --- a/drivers/dma-buf/Kconfig > +++ b/drivers/dma-buf/Kconfig > @@ -81,9 +81,9 @@ menuconfig DMABUF_SYSFS_STATS > Choose this option to enable DMA-BUF sysfs statistics > in location /sys/kernel/dmabuf/buffers. > > - /sys/kernel/dmabuf/buffers/<inode_number> will contain > - statistics for the DMA-BUF with the unique inode number > - <inode_number>. > + /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will contain > + statistics for the DMA-BUF with the unique pair > + <inode_number-unique_id>. > > source "drivers/dma-buf/heaps/Kconfig" > > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c > index 2bba0ba..29e9e23 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.c > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -38,8 +38,8 @@ > * > * The following stats are exposed by the interface: > * > - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name`` > - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size`` > + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name`` > + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size`` > * > * The information in the interface can also be used to derive per-exporter > * statistics. The data from the interface can be gathered on error conditions > @@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > { > struct dma_buf_sysfs_entry *sysfs_entry; > int ret; > + static atomic64_t unique_id = ATOMIC_INIT(0); > > if (!dmabuf || !dmabuf->file) > return -EINVAL; > @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > > /* create the directory for buffer stats */ > ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, > - "%lu", file_inode(dmabuf->file)->i_ino); > + "%lu-%lu", file_inode(dmabuf->file)->i_ino, > + atomic64_add_return(1, &unique_id)); > if (ret) > goto err_sysfs_dmabuf; > > -- > 2.7.4 >
On 5/10/2022 8:42 PM, Christian König wrote: >> * The information in the interface can also be used to derive >> per-exporter >> * statistics. The data from the interface can be gathered on error >> conditions >> @@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >> { >> struct dma_buf_sysfs_entry *sysfs_entry; >> int ret; >> + static atomic64_t unique_id = ATOMIC_INIT(0); > > Please move that to the beginning of the declarations. > Done. Any scripts I can run at my end to catch these type of trivial changes? checkpatch.pl didn't report this coding style. >> if (!dmabuf || !dmabuf->file) >> return -EINVAL; >> @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >> /* create the directory for buffer stats */ >> ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, >> NULL, >> - "%lu", file_inode(dmabuf->file)->i_ino); >> + "%lu-%lu", file_inode(dmabuf->file)->i_ino, > > Why not just use the unique value here? Or is the inode number necessary > for something? This will ease the debugging a lot. Given the dump, I can easily map which dmabuf buffer to the process. On the crashutilty I just have to search for this inode in the files output, just one example.
Am 10.05.22 um 19:14 schrieb Charan Teja Kalla: > On 5/10/2022 8:42 PM, Christian König wrote: >>> * The information in the interface can also be used to derive >>> per-exporter >>> * statistics. The data from the interface can be gathered on error >>> conditions >>> @@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >>> { >>> struct dma_buf_sysfs_entry *sysfs_entry; >>> int ret; >>> + static atomic64_t unique_id = ATOMIC_INIT(0); >> Please move that to the beginning of the declarations. >> > Done. Any scripts I can run at my end to catch these type of trivial > changes? checkpatch.pl didn't report this coding style. Not that I know of. It's also not a hard requirement, I let it mostly slip in the drivers I maintain. But upstream people sometimes insist on that, so I want to be clean at least in driver independent frameworks. >>> if (!dmabuf || !dmabuf->file) >>> return -EINVAL; >>> @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >>> /* create the directory for buffer stats */ >>> ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, >>> NULL, >>> - "%lu", file_inode(dmabuf->file)->i_ino); >>> + "%lu-%lu", file_inode(dmabuf->file)->i_ino, >> Why not just use the unique value here? Or is the inode number necessary >> for something? > This will ease the debugging a lot. Given the dump, I can easily map > which dmabuf buffer to the process. On the crashutilty I just have to > search for this inode in the files output, just one example. T.J. Mercier just confirmed my suspicion that this would break the UAPI. So that won't work. This needs to be a single number, preferable documented as such. Regards, Christian.
Hi Charan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.18-rc6 next-20220510]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Charan-Teja-Kalla/dmabuf-ensure-unique-directory-name-for-dmabuf-stats/20220510-221009
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9be9ed2612b5aedb52a2c240edb1630b6b743cb6
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220511/202205110511.E0d8TXXC-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1bc947824de94b4d4a87c48772c0d36872eaf731
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Charan-Teja-Kalla/dmabuf-ensure-unique-directory-name-for-dmabuf-stats/20220510-221009
git checkout 1bc947824de94b4d4a87c48772c0d36872eaf731
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/dma-buf/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/dma-buf/dma-buf-sysfs-stats.c: In function 'dma_buf_stats_setup':
>> drivers/dma-buf/dma-buf-sysfs-stats.c:196:43: warning: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 's64' {aka 'long long int'} [-Wformat=]
196 | "%lu-%lu", file_inode(dmabuf->file)->i_ino,
| ~~^
| |
| long unsigned int
| %llu
197 | atomic64_add_return(1, &unique_id));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| s64 {aka long long int}
vim +196 drivers/dma-buf/dma-buf-sysfs-stats.c
170
171 int dma_buf_stats_setup(struct dma_buf *dmabuf)
172 {
173 struct dma_buf_sysfs_entry *sysfs_entry;
174 int ret;
175 static atomic64_t unique_id = ATOMIC_INIT(0);
176
177 if (!dmabuf || !dmabuf->file)
178 return -EINVAL;
179
180 if (!dmabuf->exp_name) {
181 pr_err("exporter name must not be empty if stats needed\n");
182 return -EINVAL;
183 }
184
185 sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), GFP_KERNEL);
186 if (!sysfs_entry)
187 return -ENOMEM;
188
189 sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset;
190 sysfs_entry->dmabuf = dmabuf;
191
192 dmabuf->sysfs_entry = sysfs_entry;
193
194 /* create the directory for buffer stats */
195 ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL,
> 196 "%lu-%lu", file_inode(dmabuf->file)->i_ino,
Thanks Christian for the inputs!! On 5/10/2022 10:52 PM, Christian König wrote: >>>> if (!dmabuf || !dmabuf->file) >>>> return -EINVAL; >>>> @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >>>> /* create the directory for buffer stats */ >>>> ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, >>>> NULL, >>>> - "%lu", file_inode(dmabuf->file)->i_ino); >>>> + "%lu-%lu", file_inode(dmabuf->file)->i_ino, >>> Why not just use the unique value here? Or is the inode number necessary >>> for something? >> This will ease the debugging a lot. Given the dump, I can easily map >> which dmabuf buffer to the process. On the crashutilty I just have to >> search for this inode in the files output, just one example. > > T.J. Mercier just confirmed my suspicion that this would break the UAPI. > So that won't work. > > This needs to be a single number, preferable documented as such. Usually, What are the chances that a patch breaking UAPI will get accepted. IMO, If there are few users, I had learnt that it is allowed to merge. (Eg: In [1] where Andrew, -mm maintainer, mentioned that: "I think we should just absorb any transitory damage which this causes people." for the patch posted breaking the UAPI). Even the patch c715def51591 ("dma-buf: Delete the DMA-BUF attachment sysfs statistics") deleted the sysfs entries which also comes under the UAPI breakage but still allowed to merge. On those lines, Is it fair to say If few users are there, uapi breakage changes are allowed to merge on the assumption that userspace code needs to be aligned with the new uapi changes? To my knowledge, Android is the only user which is just getting the dmabuf stats as part of the debug code. The single number approach, generated by atomic, wouldn't break the uapi, but that number won't give any meaningful information especially when this is targeted just for debug purpose. And just 'inode' is not usable for already stated reasons. How about using the atomic number generated it self used as inode number? I see tmpfs also maintains its own inode numbers for the same overflow reasons[2]. The code will be like below(untested): diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6fc96e..eeed770 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -408,11 +408,17 @@ static inline int is_dma_buf_file(struct file *file) static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) { struct file *file; + static atomic64_t unique_id = ATOMIC64_INIT(0); struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb); if (IS_ERR(inode)) return ERR_CAST(inode); + /* + * Override the inode->i_no number with the unique + * dmabuf specific value + */ + inode->i_no = atomic64_add_return(1, &unique_id); inode->i_size = dmabuf->size; inode_set_bytes(inode, dmabuf->size); [1] https://patchwork.kernel.org/project/linux-mm/patch/4f091776142f2ebf7b94018146de72318474e686.1647008754.git.quic_charante@quicinc.com/#24780139 [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=e809d5f0b5c912fe981dce738f3283b2010665f0 Thanks, Charan
Am 11.05.22 um 08:49 schrieb Charan Teja Kalla: > Thanks Christian for the inputs!! > > On 5/10/2022 10:52 PM, Christian König wrote: >>>>> if (!dmabuf || !dmabuf->file) >>>>> return -EINVAL; >>>>> @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >>>>> /* create the directory for buffer stats */ >>>>> ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, >>>>> NULL, >>>>> - "%lu", file_inode(dmabuf->file)->i_ino); >>>>> + "%lu-%lu", file_inode(dmabuf->file)->i_ino, >>>> Why not just use the unique value here? Or is the inode number necessary >>>> for something? >>> This will ease the debugging a lot. Given the dump, I can easily map >>> which dmabuf buffer to the process. On the crashutilty I just have to >>> search for this inode in the files output, just one example. >> T.J. Mercier just confirmed my suspicion that this would break the UAPI. >> So that won't work. >>> This needs to be a single number, preferable documented as such. > Usually, What are the chances that a patch breaking UAPI will get > accepted. IMO, If there are few users, I had learnt that it is allowed > to merge. (Eg: In [1] where Andrew, -mm maintainer, mentioned that: "I > think we should just absorb any transitory damage which this causes > people." for the patch posted breaking the UAPI). Even the patch > c715def51591 ("dma-buf: Delete the DMA-BUF attachment sysfs statistics") > deleted the sysfs entries which also comes under the UAPI breakage but > still allowed to merge. On those lines, Is it fair to say If few users > are there, uapi breakage changes are allowed to merge on the assumption > that userspace code needs to be aligned with the new uapi changes? To my > knowledge, Android is the only user which is just getting the dmabuf > stats as part of the debug code. I don't want to open up the can of worms discussing under which cases an UAPI breakage is acceptable and under which cases it's not. So to make it short: When this causes a regression for Android it's a clear NAK. > The single number approach, generated by atomic, wouldn't break the > uapi, but that number won't give any meaningful information especially > when this is targeted just for debug purpose. And just 'inode' is not > usable for already stated reasons. Well, why do you want to use the ino in the first place? This is an anonymous inode not associated with any filesystem, so that number is meaningless anyway. > How about using the atomic number generated it self used as inode > number? I see tmpfs also maintains its own inode numbers for the same > overflow reasons[2]. Yeah, that could potentially work as well. Regards, Christian. > The code will be like below(untested): > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index a6fc96e..eeed770 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -408,11 +408,17 @@ static inline int is_dma_buf_file(struct file *file) > static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) > { > struct file *file; > + static atomic64_t unique_id = ATOMIC64_INIT(0); > struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb); > > if (IS_ERR(inode)) > return ERR_CAST(inode); > > + /* > + * Override the inode->i_no number with the unique > + * dmabuf specific value > + */ > + inode->i_no = atomic64_add_return(1, &unique_id); > inode->i_size = dmabuf->size; > inode_set_bytes(inode, dmabuf->size); > > > [1] > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-mm%2Fpatch%2F4f091776142f2ebf7b94018146de72318474e686.1647008754.git.quic_charante%40quicinc.com%2F%2324780139&data=05%7C01%7Cchristian.koenig%40amd.com%7C62c0b07ffb084635dcbe08da331a6830%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637878485789848020%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=V4QdoKGL8Ifuq2gvhNcP8oPJt%2FI%2BPkhzVDhSYShCS2M%3D&reserved=0 > > [2] > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Fpatch%2F%3Fid%3De809d5f0b5c912fe981dce738f3283b2010665f0&data=05%7C01%7Cchristian.koenig%40amd.com%7C62c0b07ffb084635dcbe08da331a6830%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637878485789848020%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Vid4T1njdOMZv%2B1ADKfjiiuzt8z6%2FiFP%2BcbUwNcZmdw%3D&reserved=0 > > Thanks, > Charan
Thanks Christian for the comments!! On 5/11/2022 12:33 PM, Christian König wrote: > >> The single number approach, generated by atomic, wouldn't break the >> uapi, but that number won't give any meaningful information especially >> when this is targeted just for debug purpose. And just 'inode' is not >> usable for already stated reasons. > > Well, why do you want to use the ino in the first place? This is an > anonymous inode not associated with any filesystem, so that number is > meaningless anyway. > It is just for ease of debugging. Nothing more. I can quickly traverse the /sys/kernel/dmabuf/buffers/* and get complete information about the dmabuf buffers while relating to which process this buffer is allocated by, using this inode as the 'unique' reference. https://cs.android.com/android/platform/superproject/+/master:system/memory/libmeminfo/libdmabufinfo/tools/dmabuf_dump.cpp >> How about using the atomic number generated it self used as inode >> number? I see tmpfs also maintains its own inode numbers for the same >> overflow reasons[2]. > > Yeah, that could potentially work as well. > Thanks. Will work on the next version of this patch. > Regards, > Christian.
diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers index 5d3bc99..9fffbd3 100644 --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -4,19 +4,19 @@ KernelVersion: v5.13 Contact: Hridya Valsaraju <hridya@google.com> Description: The /sys/kernel/dmabuf/buffers directory contains a snapshot of the internal state of every DMA-BUF. - /sys/kernel/dmabuf/buffers/<inode_number> will contain the - statistics for the DMA-BUF with the unique inode number - <inode_number> + /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will + contain the statistics for the DMA-BUF with the unique + pair <inode_number-unique_id> Users: kernel memory tuning/debugging tools -What: /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name +What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name Date: May 2021 KernelVersion: v5.13 Contact: Hridya Valsaraju <hridya@google.com> Description: This file is read-only and contains the name of the exporter of the DMA-BUF. -What: /sys/kernel/dmabuf/buffers/<inode_number>/size +What: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size Date: May 2021 KernelVersion: v5.13 Contact: Hridya Valsaraju <hridya@google.com> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 541efe0..5bcbdb1 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -81,9 +81,9 @@ menuconfig DMABUF_SYSFS_STATS Choose this option to enable DMA-BUF sysfs statistics in location /sys/kernel/dmabuf/buffers. - /sys/kernel/dmabuf/buffers/<inode_number> will contain - statistics for the DMA-BUF with the unique inode number - <inode_number>. + /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will contain + statistics for the DMA-BUF with the unique pair + <inode_number-unique_id>. source "drivers/dma-buf/heaps/Kconfig" diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c index 2bba0ba..29e9e23 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.c +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -38,8 +38,8 @@ * * The following stats are exposed by the interface: * - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name`` - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size`` + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name`` + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size`` * * The information in the interface can also be used to derive per-exporter * statistics. The data from the interface can be gathered on error conditions @@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) { struct dma_buf_sysfs_entry *sysfs_entry; int ret; + static atomic64_t unique_id = ATOMIC_INIT(0); if (!dmabuf || !dmabuf->file) return -EINVAL; @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) /* create the directory for buffer stats */ ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, - "%lu", file_inode(dmabuf->file)->i_ino); + "%lu-%lu", file_inode(dmabuf->file)->i_ino, + atomic64_add_return(1, &unique_id)); if (ret) goto err_sysfs_dmabuf;
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. 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 unique_id for every inode. With this change the stats directory entries will be in the format of: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>. Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> --- Changes in V2: -- Used the atomic64_t variable to generate a unique_id to be appended to inode to have an unique directory with name <inode_number-unique_id> -- Suggested by christian -- Updated the ABI documentation -- Identified by Greg. -- Massaged the commit log. Changes in V1: -- Used the inode->i_ctime->tv_secs as an id appended to inode to create the unique directory with name <inode_number-time_in_secs>. -- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante@quicinc.com/ Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers | 10 +++++----- drivers/dma-buf/Kconfig | 6 +++--- drivers/dma-buf/dma-buf-sysfs-stats.c | 8 +++++--- 3 files changed, 13 insertions(+), 11 deletions(-)