From: Hridya Valsaraju <hridya@google.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: Greg KH <gregkh@linuxfoundation.org>,
Suren Baghdasaryan <surenb@google.com>,
LKML <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
linaro-mm-sig@lists.linaro.org,
Android Kernel Team <kernel-team@android.com>,
linux-media@vger.kernel.org
Subject: Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
Date: Fri, 11 Dec 2020 10:30:00 -0800 [thread overview]
Message-ID: <CA+wgaPM2v=+j3bpTdDGkfm+qrizW=q1qnTP9yAVGRu7_4Lf6-w@mail.gmail.com> (raw)
In-Reply-To: <e854d168-9299-7326-743e-874882d8073f@amd.com>
Thank you Christian!
On Fri, Dec 11, 2020 at 12:03 AM Christian König
<christian.koenig@amd.com> 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@amd.com> 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@android.com.
>
next prev parent reply other threads:[~2020-12-11 20:04 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-10 4:43 [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs Hridya Valsaraju
2020-12-10 9:45 ` Greg KH
2020-12-10 9:58 ` Christian König
2020-12-10 10:10 ` Greg KH
2020-12-10 10:27 ` Daniel Vetter
2020-12-10 10:56 ` Greg KH
2020-12-10 11:02 ` Christian König
2020-12-10 22:41 ` Hridya Valsaraju
2020-12-11 8:03 ` Christian König
2020-12-11 18:30 ` Hridya Valsaraju [this message]
2020-12-10 11:26 ` Daniel Vetter
2020-12-10 12:07 ` Greg KH
2020-12-10 13:10 ` Daniel Vetter
2020-12-11 20:21 ` John Stultz
2020-12-10 20:05 ` Hridya Valsaraju
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+wgaPM2v=+j3bpTdDGkfm+qrizW=q1qnTP9yAVGRu7_4Lf6-w@mail.gmail.com' \
--to=hridya@google.com \
--cc=christian.koenig@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=kernel-team@android.com \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=surenb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).