Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs

[Christian König]

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.

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

Regards,
Christian.