Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-12 Thread Charan Teja Kalla 
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.

Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-11 Thread Christian König 

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

Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-10 Thread 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.

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_chara...@quicinc.com/#24780139

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=e809d5f0b5c912fe981dce738f3283b2010665f0

Thanks,
Charan

Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-10 Thread kernel test robot 
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-...@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 

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,

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-10 Thread Christian König 

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.

Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-10 Thread 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.

>>     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.

Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

2022-05-10 Thread Christian König 

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/. 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/.

Signed-off-by: Charan Teja Kalla 
---
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  -- 
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 .
   -- 
https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_chara...@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 
  Description:  The /sys/kernel/dmabuf/buffers directory contains a
snapshot of the internal state of every DMA-BUF.
-   /sys/kernel/dmabuf/buffers/ will contain the
-   statistics for the DMA-BUF with the unique inode number
-   
+   /sys/kernel/dmabuf/buffers/ will
+   contain the statistics for the DMA-BUF with the unique
+   pair 
  Users:kernel memory tuning/debugging tools
  
-What:		/sys/kernel/dmabuf/buffers//exporter_name

+What:  
/sys/kernel/dmabuf/buffers//exporter_name
  Date: May 2021
  KernelVersion:v5.13
  Contact:  Hridya Valsaraju 
  Description:  This file is read-only and contains the name of the exporter of
the DMA-BUF.
  
-What:		/sys/kernel/dmabuf/buffers//size

+What:  /sys/kernel/dmabuf/buffers//size
  Date: May 2021
  KernelVersion:v5.13
  Contact:  Hridya Valsaraju 
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/ will contain

-  statistics for the DMA-BUF with the unique inode number
-  .
+  /sys/kernel/dmabuf/buffers/ will contain
+  statistics for the DMA-BUF with the unique pair
+  .
  
  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//exporter_name``
- * * ``/sys/kernel/dmabuf/buffers//size``
+ * * ``/sys/kernel/dmabuf/buffers//exporter_name``
+ * * ``/sys/kernel/dmabuf/buffers//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 direc