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

[Hridya Valsaraju]

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