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

2020-12-11 Thread John Stultz
On Thu, Dec 10, 2020 at 5:10 AM Daniel Vetter  wrote:
> On Thu, Dec 10, 2020 at 1:06 PM Greg KH  wrote:
> > On Thu, Dec 10, 2020 at 12:26:01PM +0100, Daniel Vetter wrote:
> > > On Thu, Dec 10, 2020 at 11:55 AM Greg KH  
> > > wrote:
> > > > On Thu, Dec 10, 2020 at 11:27:27AM +0100, Daniel Vetter wrote:
> > > > > 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 :(
> > >
> > > If this is just for dma-heaps then why don't we add the stuff back
> > > over there? It reinforces more that the android gpu stack and the
> > > non-android gpu stack on linux are fairly different in fundamental
> > > ways, but that's not really new.
> >
> > Back "over where"?
> >
> > dma-bufs are not only used for the graphics stack on android from what I
> > can tell, so this shouldn't be a gpu-specific issue.
>
> dma-buf heaps exist because android, mostly because google mandates
> it.

So, I don't think that's fair.

dma-buf heaps and ION before exist because it solves a problem they
have for allocating shared buffers for multiple complicated
multi-device pipelines where the various devices have constraints.
It's not strictly required[1], as your next point makes clear (along
with ChromeOS's Android not using it).

> There's not a whole lot (meaning zero) of actually open gpu stacks
> around that run on android and use dma-buf heaps like approved google
> systems, largely because the gralloc implementation in mesa just
> doesnt.

So yes, db845c currently uses the gbm_gralloc, which doesn't use
dmabuf heaps or ION.

That said, the resulting system still uses quite a number of dmabufs,
as Hridya's patch shows:
==> /sys/kernel/dmabuf/28435/exporter_name <==
drm
==> /sys/kernel/dmabuf/28435/dev_map_info <==
==> /sys/kernel/dmabuf/28435/size <==
16384
==> /sys/kernel/dmabuf/28161/exporter_name <==
drm
==> /sys/kernel/dmabuf/28161/dev_map_info <==
==> /sys/kernel/dmabuf/28161/size <==
524288
==> /sys/kernel/dmabuf/30924/exporter_name <==
drm
==> /sys/kernel/dmabuf/30924/dev_map_info <==
==> /sys/kernel/dmabuf/30924/size <==
8192
==> /sys/kernel/dmabuf/26880/exporter_name <==
drm
==> /sys/kernel/dmabuf/26880/dev_map_info <==
==> /sys/kernel/dmabuf/26880/size <==
262144
...

So even when devices are not using dma-buf heaps (which I get, you
have an axe to grind with :), having some way to collect useful stats
for dmabufs in use can be valuable.

(Also one might note, the db845c also doesn't have many constrained
devices, and we've not yet enabled hw codec support or camera
pipelines, so it avoids much of the complexity that ION/dma-buf heaps
was created to solve)

> So if android needs some quick debug output in sysfs, we can just add
> that in dma-buf heaps, for android only, problem solved. And much less
> annoying review to make sure it actually fits into the wider ecosystem
> because as-is (and I'm not seeing that chance anytime soon), dma-buf
> heaps is for android only. dma-buf at large isn't, so merging a debug
> output sysfs api that's just for android but misses a ton of the more
> generic features and semantics of dma-buf is not great.

The intent behind this patch is *not* to create more Android-specific
logic, but to provide useful information generically.  Indeed, Android
does use dmabufs heavily for passing buffers around, and your point
that not all systems handle graphics buffers that way is valid, and
it's important we don't bake any Android-isms into the interface. But
being able to collect data about the active dmabufs in a system is
useful, regardless of how regardless of how the dma-buf was allocated.

So I'd much rather see your feedback on how we expose other aspects of
dmabufs (dma_resv, exporter devices, attachment links) integrated,
rather then trying to ghettoize it as android-only and limit it to the
dmabuf heaps, where I don't think it makes as much sense to add.

thanks
-john

[1] Out of the box, the codec2 code added a few years back does
directly call to ION (and now dmabuf heaps) for system buffers, but it
can be configured differently as it's used in ChromeOS's Android too.


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

2020-12-11 Thread Hridya Valsaraju
Thank you Christian!

On Fri, Dec 11, 2020 at 12:03 AM Christian König
 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
> >  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//exporter_name
> >>> /sys/kernel/dmabuf//size
> >>> /sys/kernel/dmabuf//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+unsubscr...@android.com.
>


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

2020-12-11 Thread 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
 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//exporter_name
/sys/kernel/dmabuf//size
/sys/kernel/dmabuf//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.


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

2020-12-10 Thread Hridya Valsaraju
Thanks again for the reviews!

On Thu, Dec 10, 2020 at 3:03 AM Christian König
 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//exporter_name
> > /sys/kernel/dmabuf//size
> > /sys/kernel/dmabuf//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.

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

>
> >> Furthermore we don't have the exporter device covered anywhere, how is
> >> that tracked? Yes Android just uses ion for all shared buffers, but that's
> >> not how all of linux userspace works.
> > Do we have the exporter device link in the dmabuf interface?  If so,
> > great, let's use that, but for some reason I didn't think it was there.
>
> Correct, since we don't really need a device as an exporter (it can just
> be a system heap as well) we only have a const char* as name for the
> exporter.

Yes, the file exporter_name prints out this information.

>
> >> Then I guess there's the mmaps, you can fish them out of procfs. A tool
> >> which collects all that information might be useful, just as demonstration
> >> of how this is all supposed to be used.
> > There's a script somewhere that does this today, again, Hridya knows
> > more.

That is correct, we do have a tool in AOSP that gathers the
per-process DMA-BUF map stats from procfs.
https://android.googlesource.com/platform/system/memory/libmeminfo/+/refs/heads/master/libdmabufinfo/tools/dmabuf_dump.cpp

When I send the next revision of the patch, I will also include links
to AOSP CLs that show the usage for the sysfs files.

> >
> >> There's also some things to make sure we're at least having thought about
> >> how other things fit in here. E.d. dma_resv attached to the dma-buf
> >> matters in general a lot. It doesn't matter on Android because
> >> everything's pinned all the time anyway.

I see your point Daniel!  I will make the interface extendable in the
next version of the patch.

> >>
> >> Also I thought sysfs was one value one file, dumping an entire list into
> >> dev_info_map with properties we'll need to extend (once you care about
> >> dma_resv you also want to know which attachments are dynamic) does not
> >> smell like sysfs design at all.
> > sysfs is one value per file, what is being exported that is larger than
> > that here?  Did I miss something on r

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

2020-12-10 Thread Hridya Valsaraju
Thank you for the reviews Greg, Christian and Daniel!


On Thu, Dec 10, 2020 at 1:59 AM 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//exporter_name
> > /sys/kernel/dmabuf//size
> > /sys/kernel/dmabuf//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?

Yes, we currently collect this information on production devices as well.

>
> >
> > This information is intended to help with root-causing
> > low-memory kills and the debugging/analysis of other memory-related issues.
> >
> > It will also be used to derive DMA-BUF
> > per-exporter stats and per-device usage stats for Android Bug reports.
> >
> > [1]: 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1088791%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C30a0e015502b4d20e18208d89cc63f1a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431722574983797%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RdGMvj5VsFUwJcVOuSPaLuAr4eI3CR1YOaznupmpTqg%3D&reserved=0
> >
> > Signed-off-by: Hridya Valsaraju 
> > ---
> >   Documentation/ABI/testing/sysfs-kernel-dmabuf |  32 
> >   drivers/dma-buf/Kconfig   |  11 ++
> >   drivers/dma-buf/Makefile  |   1 +
> >   drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++
> >   drivers/dma-buf/dma-buf-sysfs-stats.h |  37 
> >   drivers/dma-buf/dma-buf.c |  29 
> >   include/linux/dma-buf.h   |  13 ++
> >   7 files changed, 285 insertions(+)
> >   create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf
> >   create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c
> >   create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h
> >
> > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf 
> > b/Documentation/ABI/testing/sysfs-kernel-dmabuf
> > new file mode 100644
> > index ..02d407d57aaa
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf
> > @@ -0,0 +1,32 @@
> > +What:/sys/kernel/dmabuf
> > +Date:November 2020
> > +KernelVersion:   v5.11
> > +Contact: Hridya Valsaraju 
> > +Description: The /sys/kernel/dmabuf directory contains a
> > + snapshot of the internal state of every DMA-BUF.
> > + /sys/kernel/dmabuf/ will contain the
> > + statistics for the DMA-BUF with the unique inode number
> > + 
> > +Users:   kernel memory tuning/debugging tools
> > +
> > +What:/sys/kernel/dmabuf//exporter_name
> > +Date:November 2020
> > +KernelVersion:   v5.11
> > +Contact: Hridya Valsaraju 
> > +Description: This file is read-only and contains the name of the exporter 
> > of
> > + the DMA-BUF.
> > +
> > +What:/sys/kernel/dmabuf//size
> > +Dat: November 2020
> > +KernelVersion:   v5.11
> > +Contact: Hridya Valsaraju 
> > +Description: This file is read-only and specifies the size of the DMA-BUF 
> > in
> > + bytes.
> > +
> > +What:/sys/kernel/dmabuf//dev_map_info
> > +Dat: November 2020
> > +KernelVersion:   v5.11
> > +Contact: Hridya Valsaraju 
> > +Description: This file is read-only and lists the name of devices currently
> > + mapping the DMA-BUF in a space-separated format.
> > +
> > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> > index 4f8224a6ac95..2fed26f14548 100644
> > --- a/drivers/dma-buf/Kconfig
> > +++ b/drivers/dma-buf/Kconfig
> > @@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS
> > allows userspace to allocate dma-bufs that can be shared
> > between drivers.
> >
> > +menuconfig DMABUF_SYSFS_STATS
> > + bool "DMA-BUF sysfs statistics"
> > + select DMA_SHARED_BUFFER
> > + help
> > +Choose this option to enable DMA-BUF sysfs statistics
> > +in location /sys/kernel/dmabuf.
> > +
> > +/sys/kernel/dmabuf/ will contain
> > +statistics for the DMA-BUF with the unique inode number
> > +.
> > +
> >   source "drivers/dma-buf/heaps/Kconfig"
> >
> >   endmen

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

2020-12-10 Thread Daniel Vetter
On Thu, Dec 10, 2020 at 1:06 PM Greg KH  wrote:
>
> On Thu, Dec 10, 2020 at 12:26:01PM +0100, Daniel Vetter wrote:
> > On Thu, Dec 10, 2020 at 11:55 AM Greg KH  wrote:
> > >
> > > 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//exporter_name
> > > > > > > /sys/kernel/dmabuf//size
> > > > > > > /sys/kernel/dmabuf//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 :(
> >
> > If this is just for dma-heaps then why don't we add the stuff back
> > over there? It reinforces more that the android gpu stack and the
> > non-android gpu stack on linux are fairly different in fundamental
> > ways, but that's not really new.
>
> Back "over where"?
>
> dma-bufs are not only used for the graphics stack on android from what I
> can tell, so this shouldn't be a gpu-specific issue.

dma-buf heaps exist because android, mostly because google mandates
it. There's not a whole lot (meaning zero) of actually open gpu stacks
around that run on android and use dma-buf heaps like approved google
systems, largely because the gralloc implementation in mesa just
doesnt.

So if android needs some quick debug output in sysfs, we can just add
that in dma-buf heaps, for android only, problem solved. And much less
annoying review to make sure it actually fits into the wider ecosystem
because as-is (and I'm not seeing that chance anytime soon), dma-buf
heaps is for android only. dma-buf at large isn't, so merging a debug
output sysfs api that's just for android but misses a ton of the more
generic features and semantics of dma-buf is not great.
-Daniel

-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


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

2020-12-10 Thread Greg KH
On Thu, Dec 10, 2020 at 12:26:01PM +0100, Daniel Vetter wrote:
> On Thu, Dec 10, 2020 at 11:55 AM Greg KH  wrote:
> >
> > 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//exporter_name
> > > > > > /sys/kernel/dmabuf//size
> > > > > > /sys/kernel/dmabuf//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 :(
> 
> If this is just for dma-heaps then why don't we add the stuff back
> over there? It reinforces more that the android gpu stack and the
> non-android gpu stack on linux are fairly different in fundamental
> ways, but that's not really new.

Back "over where"?

dma-bufs are not only used for the graphics stack on android from what I
can tell, so this shouldn't be a gpu-specific issue.

confused,

greg k-h


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

2020-12-10 Thread Daniel Vetter
On Thu, Dec 10, 2020 at 11:55 AM Greg KH  wrote:
>
> 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//exporter_name
> > > > > /sys/kernel/dmabuf//size
> > > > > /sys/kernel/dmabuf//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 :(

If this is just for dma-heaps then why don't we add the stuff back
over there? It reinforces more that the android gpu stack and the
non-android gpu stack on linux are fairly different in fundamental
ways, but that's not really new.
-Daniel

> But Hridya knows more, she's been dealing with the transition for a long
> time now.
>
> > 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?
>
> > Furthermore we don't have the exporter device covered anywhere, how is
> > that tracked? Yes Android just uses ion for all shared buffers, but that's
> > not how all of linux userspace works.
>
> Do we have the exporter device link in the dmabuf interface?  If so,
> great, let's use that, but for some reason I didn't think it was there.
>
> > Then I guess there's the mmaps, you can fish them out of procfs. A tool
> > which collects all that information might be useful, just as demonstration
> > of how this is all supposed to be used.
>
> There's a script somewhere that does this today, again, Hridya knows
> more.
>
> > There's also some things to make sure we're at least having thought about
> > how other things fit in here. E.d. dma_resv attached to the dma-buf
> > matters in general a lot. It doesn't matter on Android because
> > everything's pinned all the time anyway.
> >
> > Also I thought sysfs was one value one file, dumping an entire list into
> > dev_info_map with properties we'll need to extend (once you care about
> > dma_resv you also want to know which attachments are dynamic) does not
> > smell like sysfs design at all.
>
> sysfs is one value per file, what is being exported that is larger than
> that here?  Did I miss something on review?
>
> thanks,
>
> greg k-h
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


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

2020-12-10 Thread Christian König

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//exporter_name
/sys/kernel/dmabuf//size
/sys/kernel/dmabuf//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.


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.



Furthermore we don't have the exporter device covered anywhere, how is
that tracked? Yes Android just uses ion for all shared buffers, but that's
not how all of linux userspace works.

Do we have the exporter device link in the dmabuf interface?  If so,
great, let's use that, but for some reason I didn't think it was there.


Correct, since we don't really need a device as an exporter (it can just 
be a system heap as well) we only have a const char* as name for the 
exporter.



Then I guess there's the mmaps, you can fish them out of procfs. A tool
which collects all that information might be useful, just as demonstration
of how this is all supposed to be used.

There's a script somewhere that does this today, again, Hridya knows
more.


There's also some things to make sure we're at least having thought about
how other things fit in here. E.d. dma_resv attached to the dma-buf
matters in general a lot. It doesn't matter on Android because
everything's pinned all the time anyway.

Also I thought sysfs was one value one file, dumping an entire list into
dev_info_map with properties we'll need to extend (once you care about
dma_resv you also want to know which attachments are dynamic) does not
smell like sysfs design at all.

sysfs is one value per file, what is being exported that is larger than
that here?  Did I miss something on review?


See this chunk here:

+
+    list_for_each_entry(attachment, &dmabuf->attachments, node) {
+        if (attachment->map_counter) {
+            ret += sysfs_emit_at(buf, ret, "%s ",
+                     dev_name(attachment->dev));
+        }
+    }

And yes now that Daniel mentioned that it looks like a sysfs rules 
violation to me as well.


Regards,
Christian.




thanks,

greg k-h




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

2020-12-10 Thread 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//exporter_name
> > > > /sys/kernel/dmabuf//size
> > > > /sys/kernel/dmabuf//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 :(

But Hridya knows more, she's been dealing with the transition for a long
time now.

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

> Furthermore we don't have the exporter device covered anywhere, how is
> that tracked? Yes Android just uses ion for all shared buffers, but that's
> not how all of linux userspace works.

Do we have the exporter device link in the dmabuf interface?  If so,
great, let's use that, but for some reason I didn't think it was there.

> Then I guess there's the mmaps, you can fish them out of procfs. A tool
> which collects all that information might be useful, just as demonstration
> of how this is all supposed to be used.

There's a script somewhere that does this today, again, Hridya knows
more.

> There's also some things to make sure we're at least having thought about
> how other things fit in here. E.d. dma_resv attached to the dma-buf
> matters in general a lot. It doesn't matter on Android because
> everything's pinned all the time anyway.
> 
> Also I thought sysfs was one value one file, dumping an entire list into
> dev_info_map with properties we'll need to extend (once you care about
> dma_resv you also want to know which attachments are dynamic) does not
> smell like sysfs design at all.

sysfs is one value per file, what is being exported that is larger than
that here?  Did I miss something on review?

thanks,

greg k-h


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

2020-12-10 Thread Daniel Vetter
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//exporter_name
> > > /sys/kernel/dmabuf//size
> > > /sys/kernel/dmabuf//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.

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.

Furthermore we don't have the exporter device covered anywhere, how is
that tracked? Yes Android just uses ion for all shared buffers, but that's
not how all of linux userspace works.

Then I guess there's the mmaps, you can fish them out of procfs. A tool
which collects all that information might be useful, just as demonstration
of how this is all supposed to be used.

Finally we have kernel internal mappings too. Not tracked.

There's also some things to make sure we're at least having thought about
how other things fit in here. E.d. dma_resv attached to the dma-buf
matters in general a lot. It doesn't matter on Android because
everything's pinned all the time anyway.

Also I thought sysfs was one value one file, dumping an entire list into
dev_info_map with properties we'll need to extend (once you care about
dma_resv you also want to know which attachments are dynamic) does not
smell like sysfs design at all.

So yeah, why? worksformeonandroidweneeditthere not good enough for uapi of
something this core to how the gpu stack works on linux in general, at
least imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


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

2020-12-10 Thread Greg KH
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//exporter_name
> > /sys/kernel/dmabuf//size
> > /sys/kernel/dmabuf//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 :)


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

2020-12-10 Thread Christian König

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//exporter_name
/sys/kernel/dmabuf//size
/sys/kernel/dmabuf//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?




This information is intended to help with root-causing
low-memory kills and the debugging/analysis of other memory-related issues.

It will also be used to derive DMA-BUF
per-exporter stats and per-device usage stats for Android Bug reports.

[1]: 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1088791%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C30a0e015502b4d20e18208d89cc63f1a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431722574983797%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RdGMvj5VsFUwJcVOuSPaLuAr4eI3CR1YOaznupmpTqg%3D&reserved=0

Signed-off-by: Hridya Valsaraju 
---
  Documentation/ABI/testing/sysfs-kernel-dmabuf |  32 
  drivers/dma-buf/Kconfig   |  11 ++
  drivers/dma-buf/Makefile  |   1 +
  drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++
  drivers/dma-buf/dma-buf-sysfs-stats.h |  37 
  drivers/dma-buf/dma-buf.c |  29 
  include/linux/dma-buf.h   |  13 ++
  7 files changed, 285 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf
  create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c
  create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h

diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf 
b/Documentation/ABI/testing/sysfs-kernel-dmabuf
new file mode 100644
index ..02d407d57aaa
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf
@@ -0,0 +1,32 @@
+What:  /sys/kernel/dmabuf
+Date:  November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   The /sys/kernel/dmabuf directory contains a
+   snapshot of the internal state of every DMA-BUF.
+   /sys/kernel/dmabuf/ will contain the
+   statistics for the DMA-BUF with the unique inode number
+   
+Users: kernel memory tuning/debugging tools
+
+What:  /sys/kernel/dmabuf//exporter_name
+Date:  November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   This file is read-only and contains the name of the exporter of
+   the DMA-BUF.
+
+What:  /sys/kernel/dmabuf//size
+Dat:   November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   This file is read-only and specifies the size of the DMA-BUF in
+   bytes.
+
+What:  /sys/kernel/dmabuf//dev_map_info
+Dat:   November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   This file is read-only and lists the name of devices currently
+   mapping the DMA-BUF in a space-separated format.
+
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 4f8224a6ac95..2fed26f14548 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS
  allows userspace to allocate dma-bufs that can be shared
  between drivers.
  
+menuconfig DMABUF_SYSFS_STATS

+   bool "DMA-BUF sysfs statistics"
+   select DMA_SHARED_BUFFER
+   help
+  Choose this option to enable DMA-BUF sysfs statistics
+  in location /sys/kernel/dmabuf.
+
+  /sys/kernel/dmabuf/ will contain
+  statistics for the DMA-BUF with the unique inode number
+  .
+
  source "drivers/dma-buf/heaps/Kconfig"
  
  endmenu

diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 995e05f609ff..40d81f23cacf 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_DMABUF_HEAPS)  += heaps/
  obj-$(CONFIG_SYNC_FILE)   += sync_file.o
  obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o
  obj-$(CONFIG_UDMABUF) += udmabuf.o
+obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o
  
  dmabuf_selftests-y := \

selftest.o \
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c 
b/drivers/dma-buf/dma-buf-sysfs-stats.c
new file mode 100644
ind

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

2020-12-10 Thread Greg KH
On Wed, Dec 09, 2020 at 08:43:57PM -0800, Hridya Valsaraju wrote:
> 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//exporter_name
> /sys/kernel/dmabuf//size
> /sys/kernel/dmabuf//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.
> 
> This information is intended to help with root-causing
> low-memory kills and the debugging/analysis of other memory-related issues.
> 
> It will also be used to derive DMA-BUF
> per-exporter stats and per-device usage stats for Android Bug reports.
> 
> [1]: https://lore.kernel.org/patchwork/patch/1088791/
> 
> Signed-off-by: Hridya Valsaraju 

Thanks for adding all of this, nice work!

Reviewed-by: Greg Kroah-Hartman 


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

2020-12-09 Thread 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//exporter_name
/sys/kernel/dmabuf//size
/sys/kernel/dmabuf//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.

This information is intended to help with root-causing
low-memory kills and the debugging/analysis of other memory-related issues.

It will also be used to derive DMA-BUF
per-exporter stats and per-device usage stats for Android Bug reports.

[1]: https://lore.kernel.org/patchwork/patch/1088791/

Signed-off-by: Hridya Valsaraju 
---
 Documentation/ABI/testing/sysfs-kernel-dmabuf |  32 
 drivers/dma-buf/Kconfig   |  11 ++
 drivers/dma-buf/Makefile  |   1 +
 drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++
 drivers/dma-buf/dma-buf-sysfs-stats.h |  37 
 drivers/dma-buf/dma-buf.c |  29 
 include/linux/dma-buf.h   |  13 ++
 7 files changed, 285 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf
 create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c
 create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h

diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf 
b/Documentation/ABI/testing/sysfs-kernel-dmabuf
new file mode 100644
index ..02d407d57aaa
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf
@@ -0,0 +1,32 @@
+What:  /sys/kernel/dmabuf
+Date:  November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   The /sys/kernel/dmabuf directory contains a
+   snapshot of the internal state of every DMA-BUF.
+   /sys/kernel/dmabuf/ will contain the
+   statistics for the DMA-BUF with the unique inode number
+   
+Users: kernel memory tuning/debugging tools
+
+What:  /sys/kernel/dmabuf//exporter_name
+Date:  November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   This file is read-only and contains the name of the exporter of
+   the DMA-BUF.
+
+What:  /sys/kernel/dmabuf//size
+Dat:   November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   This file is read-only and specifies the size of the DMA-BUF in
+   bytes.
+
+What:  /sys/kernel/dmabuf//dev_map_info
+Dat:   November 2020
+KernelVersion: v5.11
+Contact:   Hridya Valsaraju 
+Description:   This file is read-only and lists the name of devices currently
+   mapping the DMA-BUF in a space-separated format.
+
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 4f8224a6ac95..2fed26f14548 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS
  allows userspace to allocate dma-bufs that can be shared
  between drivers.
 
+menuconfig DMABUF_SYSFS_STATS
+   bool "DMA-BUF sysfs statistics"
+   select DMA_SHARED_BUFFER
+   help
+  Choose this option to enable DMA-BUF sysfs statistics
+  in location /sys/kernel/dmabuf.
+
+  /sys/kernel/dmabuf/ will contain
+  statistics for the DMA-BUF with the unique inode number
+  .
+
 source "drivers/dma-buf/heaps/Kconfig"
 
 endmenu
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 995e05f609ff..40d81f23cacf 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_DMABUF_HEAPS)  += heaps/
 obj-$(CONFIG_SYNC_FILE)+= sync_file.o
 obj-$(CONFIG_SW_SYNC)  += sw_sync.o sync_debug.o
 obj-$(CONFIG_UDMABUF)  += udmabuf.o
+obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o
 
 dmabuf_selftests-y := \
selftest.o \
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c 
b/drivers/dma-buf/dma-buf-sysfs-stats.c
new file mode 100644
index ..bcbef81e0a5f
--- /dev/null
+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define to_dma_buf_entry_from_kobj(x) container_of(x, struct 
dma_buf_sysfs_entry, kobj)
+
+struct dma_buf_stats_attribute {
+   struct attribute attr;
+   ssize_t (*show)(struct dma_buf *dmabuf,
+   struct dma_buf_stats_attribute *attr, char *buf);
+};
+#define to_dma_buf_stats_attr(x) container_of(x, struct 
dma_buf_stats_attribute, attr)
+
+static ssize_t dma_buf_stats_attribute_show(struct kobject *kobj,
+