Thank you Christian! On Fri, Dec 11, 2020 at 12:03 AM Christian König <christian.koenig@xxxxxxx> wrote: > > Am 10.12.20 um 23:41 schrieb Hridya Valsaraju: > > Thanks again for the reviews! > > > > On Thu, Dec 10, 2020 at 3:03 AM Christian König > > <christian.koenig@xxxxxxx> wrote: > >> Am 10.12.20 um 11:56 schrieb Greg KH: > >>> On Thu, Dec 10, 2020 at 11:27:27AM +0100, Daniel Vetter wrote: > >>>> On Thu, Dec 10, 2020 at 11:10:45AM +0100, Greg KH wrote: > >>>>> On Thu, Dec 10, 2020 at 10:58:50AM +0100, Christian König wrote: > >>>>>> In general a good idea, but I have a few concern/comments here. > >>>>>> > >>>>>> Am 10.12.20 um 05:43 schrieb Hridya Valsaraju: > >>>>>>> This patch allows statistics to be enabled for each DMA-BUF in > >>>>>>> sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > >>>>>>> > >>>>>>> The following stats will be exposed by the interface: > >>>>>>> > >>>>>>> /sys/kernel/dmabuf/<inode_number>/exporter_name > >>>>>>> /sys/kernel/dmabuf/<inode_number>/size > >>>>>>> /sys/kernel/dmabuf/<inode_number>/dev_map_info > >>>>>>> > >>>>>>> The inode_number is unique for each DMA-BUF and was added earlier [1] > >>>>>>> in order to allow userspace to track DMA-BUF usage across different > >>>>>>> processes. > >>>>>>> > >>>>>>> Currently, this information is exposed in > >>>>>>> /sys/kernel/debug/dma_buf/bufinfo. > >>>>>>> However, since debugfs is considered unsafe to be mounted in production, > >>>>>>> it is being duplicated in sysfs. > >>>>>> Mhm, this makes it part of the UAPI. What is the justification for this? > >>>>>> > >>>>>> In other words do we really need those debug information in a production > >>>>>> environment? > >>>>> Production environments seem to want to know who is using up memory :) > >>>> This only shows shared memory, so it does smell a lot like $specific_issue > >>>> and we're designing a narrow solution for that and then have to carry it > >>>> forever. > >>> I think the "issue" is that this was a feature from ion that people > >>> "missed" in the dmabuf move. Taking away the ability to see what kind > >>> of allocations were being made didn't make a lot of debugging tools > >>> happy :( > >> Yeah, that is certainly a very valid concern. > >> > >>> But Hridya knows more, she's been dealing with the transition for a long > >>> time now. > > Currently, telemetry tools capture this information(along with other > > memory metrics) periodically as well as on important events like a > > foreground app kill (which might have been triggered by an LMK). We > > would also like to get a snapshot of the system memory usage on other > > events such as OOM kills and ANRs. > > That userspace tools are going to use those files directly is the > justification you need for the stable UAPI provided by sysfs. > > Otherwise debugfs would be the way to go even when that is often disabled. > > Please change the commit message to reflect that. Sounds good, I will make sure to include it in the commit message of the next version. > > >>>> E.g. why is the list of attachments not a sysfs link? That's how we > >>>> usually expose struct device * pointers in sysfs to userspace, not as a > >>>> list of things. > >>> These aren't struct devices, so I don't understand the objection here. > >>> Where else could these go in sysfs? > >> Sure they are! Just take a look at an attachment: > >> > >> struct dma_buf_attachment { > >> struct dma_buf *dmabuf; > >> struct device *dev; > >> > >> This is the struct device which is importing the buffer and the patch in > >> discussion is just printing the name of this device into sysfs. > > I actually did not know that this is not ok to do. I will change it in > > the next version of the patch to be sysfs links instead. > > Thanks, you need to restructure this patch a bit. But I agree with > Daniel that links to the devices which are attached are more appropriate. > > I'm just not sure how we want to represent the map count for each > attachment then, probably best to have that as separate file as well. Yes, I can add the map count as a separate file. Thanks again! Regards, Hridya > > Regards, > Christian. > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@xxxxxxxxxxx. >