Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/25/21 9:33 AM, Mike Rapoport wrote:
> On Thu, Apr 22, 2021 at 02:08:51PM +, peter.enderb...@sony.com wrote:
>> On 4/22/21 10:06 AM, Mike Rapoport wrote:
>>> So the flow is like this:
>>>
>>> * a user has a problem and reports it to an application developer; at best
>>>   the user runs simple and limited app to collect some data
>>> * if the application developer considers this issue as a system related
>>>   they can open adb and collect some more information about the system
>>>   using non-root shell with selinux policy restrictions and send this
>>>   information to the device manufacturer.
>>> * the manufacturer continues to debug the issue and at this point as much
>>>   information is possible would have been useful.
>>>
>>> In this flow I still fail to understand why the manufacturer cannot provide
>>> userspace tools that will be able to collect the required information.
>>> These tools not necessarily need to target the end user, they may be only
>>> intended for the application developers, e.g. policy could allow such tool
>>> to access some of the system data only when the system is in developer
>>> mode.
>>>
>> The manufacture is trying to get the tool to work. This is what the
>> patch is about. Even for a application developer a commercial
>> phone is locked down.
> Right, but it's still in full control of the manufacturer what's flashed
> there, isn't it?

No. There is a lot of restrictions, and Google will provide a binary
kernel that is used on android devices that is the one hat MUST
be used on commercial models. It is called GKI.

> So there could be some tools that are only available in the developer mode?
> These tools could have different permissions etc.
>
>> Many vendors allow that you flash some other software like a AOSP.  But
>> that can be very different. Like installing a ubuntu on a PC to debug a
>> Fedora issue.
>>
>> And sure we can pickup parts of what using the dma-buf. But
>> we can not get the total and be sure that is the total without a
>> proper counter.
> If I understand you correctly, a user space tool that scans fdinfo and
> accumulates dma-buf size from there is not accurate enough, that's why an
> atomic counter exposed by kernel is a must.
And it is lightweight.
> But if the changes in consumption of dma-bufs are that frequent, I cannot
> see how a global counter will help to identify an issue.
Same goes for all memory counters. You can sample the counters
and build statistics when you have many devices. Statistics change
you usually see leaks.
> And if this counter is needed to see if there is a memory leak, summing
> sizes of dma-bufs from fdinfo will identify a leak.
>
> What am I missing?
>
I think you can only see dma-buf that is mapped to a process
with that method, we have buffers that goes to other subsystems like audiodsp.
And processing all fd's on a system frequently and sort out
all duplicates is not light, and it sill will not be a total. Nor is it
a snapshot.

When it is about to find the leaks, kmemleak works fine with
kernel leaks, and page_owner (also only in debugfs) is a good tool.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/22/21 10:06 AM, Mike Rapoport wrote:
> On Wed, Apr 21, 2021 at 05:35:57PM +, peter.enderb...@sony.com wrote:
>> On 4/21/21 5:31 PM, Mike Rapoport wrote:
>>> On Wed, Apr 21, 2021 at 10:37:11AM +, peter.enderb...@sony.com wrote:
 On 4/21/21 11:15 AM, Daniel Vetter wrote:
> We need to understand what the "correct" value is. Not in terms of kernel
> code, but in terms of semantics. Like if userspace allocates a GL texture,
> is this supposed to show up in your metric or not. Stuff like that.
 That it like that would like to only one pointer type. You need to know 
 what

 you pointing at to know what it is. it might be a hardware or a other 
 pointer.

 If there is a limitation on your pointers it is a good metric to count them
 even if you don't  know what they are. Same goes for dma-buf, they
 are generic, but they consume some resources that are counted in pages.

 It would be very good if there a sub division where you could measure
 all possible types separately.  We have the detailed in debugfs, but 
 nothing
 for the user. A summary in meminfo seems to be the best place for such
 metric.
>>>  
>>> Let me try to summarize my understanding of the problem, maybe it'll help
>>> others as well.
>> Thanks!
>>
>>
>>> A device driver allocates memory and exports this memory via dma-buf so
>>> that this memory will be accessible for userspace via a file descriptor.
>>>
>>> The allocated memory can be either allocated with alloc_page() from system
>>> RAM or by other means from dedicated VRAM (that is not managed by Linux mm)
>>> or even from on-device memory.
>>>
>>> The dma-buf driver tracks the amount of the memory it was requested to
>>> export and the size it sees is available at debugfs and fdinfo.
>>>
>>> The debugfs is not available to user and maybe entirely disabled in
>>> production systems.
>>>
>>> There could be quite a few open dma-bufs so there is no overall summary,
>>> plus fdinfo in production systems your refer to is also unavailable to the
>>> user because of selinux policy.
>>>
>>> And there are a few details that are not clear to me:
>>>
>>> * Since DRM device drivers seem to be the major user of dma-buf exports,
>>>   why cannot we add information about their memory consumption to, say,
>>>   /sys/class/graphics/drm/cardX/memory-usage?
>> Android is using it for binder that connect more or less everything
>> internally.
>  
> Ok, then it rules out /sys/class/graphics indeed.
>
>>> * How exactly user generates reports that would include the new counters?
>>>   From my (mostly outdated) experience Android users won't open a terminal
>>>   and type 'cat /proc/meminfo' there. I'd presume there is a vendor agent
>>>   that collects the data and sends it for analysis. In this case what is
>>>   the reason the vendor is unable to adjust selinix policy so that the
>>>   agent will be able to access fdinfo?
>> When you turn on developer mode on android you can use
>> usb with a program called adb. And there you get a normal shell.
>>
>> (not root though)
>>
>> There is applications that non developers can use to get
>> information. It is very limited though and there are API's
>> provide it.
>>
>>
>>> * And, as others already mentioned, it is not clear what are the problems
>>>   that can be detected by examining DmaBufTotal except saying "oh, there is
>>>   too much/too little memory exported via dma-buf". What would be user
>>>   visible effects of these problems? What are the next steps to investigate
>>>   them? What other data will be probably required to identify root cause?
>>>
>> When you debug thousands of devices it is quite nice to have
>> ways to classify what the problem it is not. The normal user does not
>> see anything of this. However they can generate bug-reports that
>> collect information about as much they can. Then the user have
>> to provide this bug-report to the manufacture or mostly the
>> application developer. And when the problem is
>> system related we need to reproduce the issue on a full
>> debug enabled unit.
> So the flow is like this:
>
> * a user has a problem and reports it to an application developer; at best
>   the user runs simple and limited app to collect some data
> * if the application developer considers this issue as a system related
>   they can open adb and collect some more information about the system
>   using non-root shell with selinux policy restrictions and send this
>   information to the device manufacturer.
> * the manufacturer continues to debug the issue and at this point as much
>   information is possible would have been useful.
>
> In this flow I still fail to understand why the manufacturer cannot provide
> userspace tools that will be able to collect the required information.
> These tools not necessarily need to target the end user, they may be only
> intended for the application developers, e.g. policy could allow such tool
> to access some of 

Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/21/21 5:31 PM, Mike Rapoport wrote:
> On Wed, Apr 21, 2021 at 10:37:11AM +, peter.enderb...@sony.com wrote:
>> On 4/21/21 11:15 AM, Daniel Vetter wrote:
>>> We need to understand what the "correct" value is. Not in terms of kernel
>>> code, but in terms of semantics. Like if userspace allocates a GL texture,
>>> is this supposed to show up in your metric or not. Stuff like that.
>> That it like that would like to only one pointer type. You need to know what
>>
>> you pointing at to know what it is. it might be a hardware or a other 
>> pointer.
>>
>> If there is a limitation on your pointers it is a good metric to count them
>> even if you don't  know what they are. Same goes for dma-buf, they
>> are generic, but they consume some resources that are counted in pages.
>>
>> It would be very good if there a sub division where you could measure
>> all possible types separately.  We have the detailed in debugfs, but nothing
>> for the user. A summary in meminfo seems to be the best place for such
>> metric.
>  
> Let me try to summarize my understanding of the problem, maybe it'll help
> others as well.

Thanks!


> A device driver allocates memory and exports this memory via dma-buf so
> that this memory will be accessible for userspace via a file descriptor.
>
> The allocated memory can be either allocated with alloc_page() from system
> RAM or by other means from dedicated VRAM (that is not managed by Linux mm)
> or even from on-device memory.
>
> The dma-buf driver tracks the amount of the memory it was requested to
> export and the size it sees is available at debugfs and fdinfo.
>
> The debugfs is not available to user and maybe entirely disabled in
> production systems.
>
> There could be quite a few open dma-bufs so there is no overall summary,
> plus fdinfo in production systems your refer to is also unavailable to the
> user because of selinux policy.
>
> And there are a few details that are not clear to me:
>
> * Since DRM device drivers seem to be the major user of dma-buf exports,
>   why cannot we add information about their memory consumption to, say,
>   /sys/class/graphics/drm/cardX/memory-usage?

Android is using it for binder that connect more or less everything
internally.

> * How exactly user generates reports that would include the new counters?
>   From my (mostly outdated) experience Android users won't open a terminal
>   and type 'cat /proc/meminfo' there. I'd presume there is a vendor agent
>   that collects the data and sends it for analysis. In this case what is
>   the reason the vendor is unable to adjust selinix policy so that the
>   agent will be able to access fdinfo?

When you turn on developer mode on android you can use
usb with a program called adb. And there you get a normal shell.

(not root though)

There is applications that non developers can use to get
information. It is very limited though and there are API's
provide it.


>
> * And, as others already mentioned, it is not clear what are the problems
>   that can be detected by examining DmaBufTotal except saying "oh, there is
>   too much/too little memory exported via dma-buf". What would be user
>   visible effects of these problems? What are the next steps to investigate
>   them? What other data will be probably required to identify root cause?
>
When you debug thousands of devices it is quite nice to have
ways to classify what the problem it is not. The normal user does not
see anything of this. However they can generate bug-reports that
collect information about as much they can. Then the user have
to provide this bug-report to the manufacture or mostly the
application developer. And when the problem is
system related we need to reproduce the issue on a full
debug enabled unit.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/21/21 11:15 AM, Daniel Vetter wrote:
> On Tue, Apr 20, 2021 at 11:37:41AM +, peter.enderb...@sony.com wrote:
>> On 4/20/21 1:14 PM, Daniel Vetter wrote:
>>> On Tue, Apr 20, 2021 at 09:26:00AM +, peter.enderb...@sony.com wrote:
 On 4/20/21 10:58 AM, Daniel Vetter wrote:
> On Sat, Apr 17, 2021 at 06:38:35PM +0200, Peter Enderborg wrote:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
>>
>> Signed-off-by: Peter Enderborg 
> So there have been tons of discussions around how to track dma-buf and
> why, and I really need to understand the use-cass here first I think. proc
> uapi is as much forever as anything else, and depending what you're doing
> this doesn't make any sense at all:
>
> - on most linux systems dma-buf are only instantiated for shared buffer.
>   So there this gives you a fairly meaningless number and not anything
>   reflecting gpu memory usage at all.
>
> - on Android all buffers are allocated through dma-buf afaik. But there
>   we've recently had some discussions about how exactly we should track
>   all this, and the conclusion was that most of this should be solved by
>   cgroups long term. So if this is for Android, then I don't think adding
>   random quick stop-gaps to upstream is a good idea (because it's a pretty
>   long list of patches that have come up on this).
>
> So what is this for?
 For the overview. dma-buf today only have debugfs for info. Debugfs
 is not allowed by google to use in andoid. So this aggregate the 
 information
 so we can get information on what going on on the system. 

 And the LKML standard respond to that is "SHOW ME THE CODE".
>>> Yes. Except this extends to how exactly this is supposed to be used in
>>> userspace and acted upon.
>>>
 When the top memgc has a aggregated information on dma-buf it is maybe
 a better source to meminfo. But then it also imply that dma-buf requires 
 memcg.

 And I dont see any problem to replace this with something better with it 
 is ready.
>>> The thing is, this is uapi. Once it's merged we cannot, ever, replace it.
>>> It must be kept around forever, or a very close approximation thereof. So
>>> merging this with the justification that we can fix it later on or replace
>>> isn't going to happen.
>> It is intended to be relevant as long there is a dma-buf. This is a proper
>> metric. If the newer implementations is not get the same result it is
>> not doing it right and is not better. If a memcg counter or a global_zone
>> counter do the same thing they it can replace the suggested method.
> We're not talking about a memcg controller, but about a dma-buf tracker.
>
> Also my point was that you might not have a dma-buf on most linux systems
> (outside of android really) for most gpu allocations. So we kinda need to
> understand what you actually want to measure, not "I want to count all the
> dma-buf in the system". Because that's a known-problematic metric in
> general.

I have got some android in my Fedora 33 then.

 cat /sys/kernel/debug/dma_buf/bufinfo

Dma-buf Objects:
size    flags   mode    count   exp_name    
ino
04456448        00080005    0003    drm 05267137
    Exclusive fence: nouveau Xwayland[154936] signalled
    Attached Devices:
Total 0 devices attached

00851968        00080005    0003    drm 03571790
    Exclusive fence: nouveau Xwayland[154936] signalled
    Shared fence: nouveau systemd-logind[691] signalled
    Attached Devices:
Total 0 devices attached

00851968        00080005    0003    drm 03556344
    Exclusive fence: nouveau Xwayland[154936] signalled
    Shared fence: nouveau systemd-logind[691] signalled
    Attached Devices:
Total 0 devices attached

00851968        00080005    0003    drm 03249623
    Exclusive fence: nouveau Xwayland[154936] signalled
    Shared fence: nouveau systemd-logind[691] signalled
    Attached Devices:
Total 0 devices attached


Total 4 objects, 7012352 bytes

It is on my old ubuntu's too, but no clients.



>> But I dont think they will. dma-buf does not have to be mapped to a process,
>> and the case of vram, it is not covered in current global_zone. All of them
>> would be very nice to have in some form. But it wont change what the
>> correct value of what "Total" is.
> We need to understand what the "correct" value is. Not in terms of kernel
> code, but in terms of semantics. Like if userspace allocates a GL texture,
> is this supposed to show up in your metric or 

Re: [PATCH v2] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 4:48 PM, Daniel Stone wrote:
> On Tue, 20 Apr 2021 at 14:46,  > wrote:
>
> On 4/20/21 3:34 PM, Daniel Stone wrote:
> > On Fri, 16 Apr 2021 at 13:34, Peter Enderborg    >> wrote:
> >     This adds a total used dma-buf memory. Details
> >     can be found in debugfs, however it is not for everyone
> >     and not always available. dma-buf are indirect allocated by
> >     userspace. So with this value we can monitor and detect
> >     userspace applications that have problems.
> >
> >
> > FWIW, this won't work super well for Android where gralloc is 
> implemented as a system service, so all graphics usage will instantly be 
> accounted to it.
>
> This resource allocation is a big part of why we need it. Why should it 
> not work?
>
>
> Sorry, I'd somehow completely misread that as being locally rather than 
> globally accounted. Given that, it's more correct, just also not super useful.
>
> Some drivers export allocation tracepoints which you could use if you have a 
> decent userspace tracing infrastructure. Short of that, many drivers export 
> this kind of thing through debugfs already. I think a better long-term 
> direction is probably getting accounting from dma-heaps rather than extending 
> core dmabuf itself.
>
> Cheers,
> Daniel 

Debugfs and traces are useful when you pin down your problem.  Debugfs does not 
exist on commercial devices so we need some hints on what going on, and trace 
points needs active debugging
and before the problem occurs. A metric on dma-buf can be sent with a bugreport.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 3:34 PM, Daniel Stone wrote:
> Hi Peter,
>
> On Fri, 16 Apr 2021 at 13:34, Peter Enderborg  > wrote:
>
> This adds a total used dma-buf memory. Details
> can be found in debugfs, however it is not for everyone
> and not always available. dma-buf are indirect allocated by
> userspace. So with this value we can monitor and detect
> userspace applications that have problems.
>
>
> FWIW, this won't work super well for Android where gralloc is implemented as 
> a system service, so all graphics usage will instantly be accounted to it.
>
> Cheers,
> Daniel 

This resource allocation is a big part of why we need it. Why should it not 
work? 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 1:52 PM, Mike Rapoport wrote:
> On Tue, Apr 20, 2021 at 10:45:21AM +, peter.enderb...@sony.com wrote:
>> On 4/20/21 11:41 AM, Mike Rapoport wrote:
>>> Hello Peter,
>>>
>>> On Tue, Apr 20, 2021 at 09:26:00AM +, peter.enderb...@sony.com wrote:
 On 4/20/21 10:58 AM, Daniel Vetter wrote:
> On Sat, Apr 17, 2021 at 06:38:35PM +0200, Peter Enderborg wrote:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
>>
>> Signed-off-by: Peter Enderborg 
> So there have been tons of discussions around how to track dma-buf and
> why, and I really need to understand the use-cass here first I think. proc
> uapi is as much forever as anything else, and depending what you're doing
> this doesn't make any sense at all:
>
> - on most linux systems dma-buf are only instantiated for shared buffer.
>   So there this gives you a fairly meaningless number and not anything
>   reflecting gpu memory usage at all.
>
> - on Android all buffers are allocated through dma-buf afaik. But there
>   we've recently had some discussions about how exactly we should track
>   all this, and the conclusion was that most of this should be solved by
>   cgroups long term. So if this is for Android, then I don't think adding
>   random quick stop-gaps to upstream is a good idea (because it's a pretty
>   long list of patches that have come up on this).
>
> So what is this for?
 For the overview. dma-buf today only have debugfs for info. Debugfs
 is not allowed by google to use in andoid. So this aggregate the 
 information
 so we can get information on what going on on the system. 
>>>  
>>> Can you send an example debugfs output to see what data are we talking
>>> about?
>> Sure. This is on a idle system. Im not sure why you need it.The problem is 
>> partly that debugfs is
>> not accessable on a commercial device.
> I wanted to see what kind of information is there, but I didn't think it's
> that long :)
Sorry, but it was making a point.
>  
>> Dma-buf Objects:
>> size        flags       mode        count       exp_name        buf name    
>> ino
>> 00032768    0002    00080007    0002    
>> ion-system-1006-allocator-servi    dmabuf17728    07400825    dmabuf17728
>>     Attached Devices:
>> Total 0 devices attached
>>
>> 11083776    0002    00080007    0003    
>> ion-system-1006-allocator-servi    dmabuf17727    07400824    dmabuf17727
>>     Attached Devices:
>>     ae0.qcom,mdss_mdp:qcom,smmu_sde_unsec_cb
>> Total 1 devices attached
>>
>> 00032768    0002    00080007    0002    
>> ion-system-1006-allocator-servi    dmabuf17726    07400823    dmabuf17726
>>     Attached Devices:
>> Total 0 devices attached
>>
>> 11083776    0002    00080007    0002    
>> ion-system-1006-allocator-servi    dmabuf17725    07400822    dmabuf17725
>>     Attached Devices:
>>     ae0.qcom,mdss_mdp:qcom,smmu_sde_unsec_cb
>> Total 1 devices attached
> ...
>
>> Total 654 objects, 744144896 bytes
>  
> Isn't the size from the first column also available in fdinfo?
>
> Is there anything that prevents monitoring those?
>
Yes, selinux.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 1:14 PM, Daniel Vetter wrote:
> On Tue, Apr 20, 2021 at 09:26:00AM +, peter.enderb...@sony.com wrote:
>> On 4/20/21 10:58 AM, Daniel Vetter wrote:
>>> On Sat, Apr 17, 2021 at 06:38:35PM +0200, Peter Enderborg wrote:
 This adds a total used dma-buf memory. Details
 can be found in debugfs, however it is not for everyone
 and not always available. dma-buf are indirect allocated by
 userspace. So with this value we can monitor and detect
 userspace applications that have problems.

 Signed-off-by: Peter Enderborg 
>>> So there have been tons of discussions around how to track dma-buf and
>>> why, and I really need to understand the use-cass here first I think. proc
>>> uapi is as much forever as anything else, and depending what you're doing
>>> this doesn't make any sense at all:
>>>
>>> - on most linux systems dma-buf are only instantiated for shared buffer.
>>>   So there this gives you a fairly meaningless number and not anything
>>>   reflecting gpu memory usage at all.
>>>
>>> - on Android all buffers are allocated through dma-buf afaik. But there
>>>   we've recently had some discussions about how exactly we should track
>>>   all this, and the conclusion was that most of this should be solved by
>>>   cgroups long term. So if this is for Android, then I don't think adding
>>>   random quick stop-gaps to upstream is a good idea (because it's a pretty
>>>   long list of patches that have come up on this).
>>>
>>> So what is this for?
>> For the overview. dma-buf today only have debugfs for info. Debugfs
>> is not allowed by google to use in andoid. So this aggregate the information
>> so we can get information on what going on on the system. 
>>
>> And the LKML standard respond to that is "SHOW ME THE CODE".
> Yes. Except this extends to how exactly this is supposed to be used in
> userspace and acted upon.
>
>> When the top memgc has a aggregated information on dma-buf it is maybe
>> a better source to meminfo. But then it also imply that dma-buf requires 
>> memcg.
>>
>> And I dont see any problem to replace this with something better with it is 
>> ready.
> The thing is, this is uapi. Once it's merged we cannot, ever, replace it.
> It must be kept around forever, or a very close approximation thereof. So
> merging this with the justification that we can fix it later on or replace
> isn't going to happen.

It is intended to be relevant as long there is a dma-buf. This is a proper
metric. If the newer implementations is not get the same result it is
not doing it right and is not better. If a memcg counter or a global_zone
counter do the same thing they it can replace the suggested method.

But I dont think they will. dma-buf does not have to be mapped to a process,
and the case of vram, it is not covered in current global_zone. All of them
would be very nice to have in some form. But it wont change what the
correct value of what "Total" is.


> -Daniel
>
>>> -Daniel
>>>
 ---
  drivers/dma-buf/dma-buf.c | 12 
  fs/proc/meminfo.c |  5 -
  include/linux/dma-buf.h   |  1 +
  3 files changed, 17 insertions(+), 1 deletion(-)

 diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
 index f264b70c383e..4dc37cd4293b 100644
 --- a/drivers/dma-buf/dma-buf.c
 +++ b/drivers/dma-buf/dma-buf.c
 @@ -37,6 +37,7 @@ struct dma_buf_list {
  };
  
  static struct dma_buf_list db_list;
 +static atomic_long_t dma_buf_global_allocated;
  
  static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int 
 buflen)
  {
 @@ -79,6 +80,7 @@ static void dma_buf_release(struct dentry *dentry)
if (dmabuf->resv == (struct dma_resv *)[1])
dma_resv_fini(dmabuf->resv);
  
 +  atomic_long_sub(dmabuf->size, _buf_global_allocated);
module_put(dmabuf->owner);
kfree(dmabuf->name);
kfree(dmabuf);
 @@ -586,6 +588,7 @@ struct dma_buf *dma_buf_export(const struct 
 dma_buf_export_info *exp_info)
mutex_lock(_list.lock);
list_add(>list_node, _list.head);
mutex_unlock(_list.lock);
 +  atomic_long_add(dmabuf->size, _buf_global_allocated);
  
return dmabuf;
  
 @@ -1346,6 +1349,15 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct 
 dma_buf_map *map)
  }
  EXPORT_SYMBOL_GPL(dma_buf_vunmap);
  
 +/**
 + * dma_buf_allocated_pages - Return the used nr of pages
 + * allocated for dma-buf
 + */
 +long dma_buf_allocated_pages(void)
 +{
 +  return atomic_long_read(_buf_global_allocated) >> PAGE_SHIFT;
 +}
 +
  #ifdef CONFIG_DEBUG_FS
  static int dma_buf_debug_show(struct seq_file *s, void *unused)
  {
 diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
 index 6fa761c9cc78..ccc7c40c8db7 100644
 --- a/fs/proc/meminfo.c
 +++ b/fs/proc/meminfo.c
 @@ -16,6 +16,7 @@
  #ifdef CONFIG_CMA
  #include 

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 1:04 PM, Michal Hocko wrote:
> On Tue 20-04-21 09:25:51, peter.enderb...@sony.com wrote:
>> On 4/20/21 11:12 AM, Michal Hocko wrote:
>>> On Tue 20-04-21 09:02:57, peter.enderb...@sony.com wrote:
>> But that isn't really system memory at all, it's just allocated device
>> memory.
> OK, that was not really clear to me. So this is not really accounted to
> MemTotal? If that is really the case then reporting it into the oom
> report is completely pointless and I am not even sure /proc/meminfo is
> the right interface either. It would just add more confusion I am
> afraid.
>  
 Why is it confusing? Documentation is quite clear:
>>> Because a single counter without a wider context cannot be put into any
>>> reasonable context. There is no notion of the total amount of device
>>> memory usable for dma-buf. As Christian explained some of it can be RAM
>>> based. So a single number is rather pointless on its own in many cases.
>>>
>>> Or let me just ask. What can you tell from dma-bud: $FOO kB in its
>>> current form?
>> It is better to be blind?
> No it is better to have a sensible counter that can be reasoned about.
> So far you are only claiming that having something is better than
> nothing and I would agree with you if that was a debugging one off
> interface. But /proc/meminfo and other proc files have to be maintained
> with future portability in mind. This is not a dumping ground for _some_
> counters that might be interesting at the _current_ moment. E.g. what
> happens if somebody wants to have a per device resp. memory based
> dma-buf data? Are you going to change the semantic or add another
> 2 counters?

This is the DmaBufTotal. It is the upper limit. If is not there is  is 
something else.

And when we have a better resolution on measuring it, it would make sense
to add a DmaBufVram, DmaBufMemGC or what ever we can pickup.

This is what we can measure today.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 10:58 AM, Daniel Vetter wrote:
> On Sat, Apr 17, 2021 at 06:38:35PM +0200, Peter Enderborg wrote:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
>>
>> Signed-off-by: Peter Enderborg 
> So there have been tons of discussions around how to track dma-buf and
> why, and I really need to understand the use-cass here first I think. proc
> uapi is as much forever as anything else, and depending what you're doing
> this doesn't make any sense at all:
>
> - on most linux systems dma-buf are only instantiated for shared buffer.
>   So there this gives you a fairly meaningless number and not anything
>   reflecting gpu memory usage at all.
>
> - on Android all buffers are allocated through dma-buf afaik. But there
>   we've recently had some discussions about how exactly we should track
>   all this, and the conclusion was that most of this should be solved by
>   cgroups long term. So if this is for Android, then I don't think adding
>   random quick stop-gaps to upstream is a good idea (because it's a pretty
>   long list of patches that have come up on this).
>
> So what is this for?

For the overview. dma-buf today only have debugfs for info. Debugfs
is not allowed by google to use in andoid. So this aggregate the information
so we can get information on what going on on the system. 

And the LKML standard respond to that is "SHOW ME THE CODE".

When the top memgc has a aggregated information on dma-buf it is maybe
a better source to meminfo. But then it also imply that dma-buf requires memcg.

And I dont see any problem to replace this with something better with it is 
ready.

> -Daniel
>
>> ---
>>  drivers/dma-buf/dma-buf.c | 12 
>>  fs/proc/meminfo.c |  5 -
>>  include/linux/dma-buf.h   |  1 +
>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index f264b70c383e..4dc37cd4293b 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -37,6 +37,7 @@ struct dma_buf_list {
>>  };
>>  
>>  static struct dma_buf_list db_list;
>> +static atomic_long_t dma_buf_global_allocated;
>>  
>>  static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
>>  {
>> @@ -79,6 +80,7 @@ static void dma_buf_release(struct dentry *dentry)
>>  if (dmabuf->resv == (struct dma_resv *)[1])
>>  dma_resv_fini(dmabuf->resv);
>>  
>> +atomic_long_sub(dmabuf->size, _buf_global_allocated);
>>  module_put(dmabuf->owner);
>>  kfree(dmabuf->name);
>>  kfree(dmabuf);
>> @@ -586,6 +588,7 @@ struct dma_buf *dma_buf_export(const struct 
>> dma_buf_export_info *exp_info)
>>  mutex_lock(_list.lock);
>>  list_add(>list_node, _list.head);
>>  mutex_unlock(_list.lock);
>> +atomic_long_add(dmabuf->size, _buf_global_allocated);
>>  
>>  return dmabuf;
>>  
>> @@ -1346,6 +1349,15 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct 
>> dma_buf_map *map)
>>  }
>>  EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>>  
>> +/**
>> + * dma_buf_allocated_pages - Return the used nr of pages
>> + * allocated for dma-buf
>> + */
>> +long dma_buf_allocated_pages(void)
>> +{
>> +return atomic_long_read(_buf_global_allocated) >> PAGE_SHIFT;
>> +}
>> +
>>  #ifdef CONFIG_DEBUG_FS
>>  static int dma_buf_debug_show(struct seq_file *s, void *unused)
>>  {
>> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
>> index 6fa761c9cc78..ccc7c40c8db7 100644
>> --- a/fs/proc/meminfo.c
>> +++ b/fs/proc/meminfo.c
>> @@ -16,6 +16,7 @@
>>  #ifdef CONFIG_CMA
>>  #include 
>>  #endif
>> +#include 
>>  #include 
>>  #include "internal.h"
>>  
>> @@ -145,7 +146,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
>>  show_val_kb(m, "CmaFree:",
>>  global_zone_page_state(NR_FREE_CMA_PAGES));
>>  #endif
>> -
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>> +show_val_kb(m, "DmaBufTotal:", dma_buf_allocated_pages());
>> +#endif
>>  hugetlb_report_meminfo(m);
>>  
>>  arch_report_meminfo(m);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index efdc56b9d95f..5b05816bd2cd 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct 
>> *,
>>   unsigned long);
>>  int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>>  void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>> +long dma_buf_allocated_pages(void);
>>  #endif /* __DMA_BUF_H__ */
>> -- 
>> 2.17.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> 

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/20/21 11:12 AM, Michal Hocko wrote:
> On Tue 20-04-21 09:02:57, peter.enderb...@sony.com wrote:
 But that isn't really system memory at all, it's just allocated device
 memory.
>>> OK, that was not really clear to me. So this is not really accounted to
>>> MemTotal? If that is really the case then reporting it into the oom
>>> report is completely pointless and I am not even sure /proc/meminfo is
>>> the right interface either. It would just add more confusion I am
>>> afraid.
>>>  
>> Why is it confusing? Documentation is quite clear:
> Because a single counter without a wider context cannot be put into any
> reasonable context. There is no notion of the total amount of device
> memory usable for dma-buf. As Christian explained some of it can be RAM
> based. So a single number is rather pointless on its own in many cases.
>
> Or let me just ask. What can you tell from dma-bud: $FOO kB in its
> current form?

It is better to be blind? The value can still be used a relative metric.
You collect the data and see how it change. And when you see
a unexpected change you start to dig in. It fore sure wont tell what line
in your application that has a bug.  But it might be an indicator that
a new game trigger a leak. And it is very well specified, it exactly the
size of mapped dma-buf. For what you use dma-buf you need to know
other parts of the system.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

>> But that isn't really system memory at all, it's just allocated device
>> memory.
> OK, that was not really clear to me. So this is not really accounted to
> MemTotal? If that is really the case then reporting it into the oom
> report is completely pointless and I am not even sure /proc/meminfo is
> the right interface either. It would just add more confusion I am
> afraid.
>  

Why is it confusing? Documentation is quite clear:

"Provides information about distribution and utilization of memory. This
varies by architecture and compile options."

A topology with VRAM fits very well on that. The point is to have an
overview.


>>> See where I am heading?
>> Yeah, totally. Thanks for pointing this out.
>>
>> Suggestions how to handle that?
> As I've pointed out in previous reply we do have an API to account per
> node memory but now that you have brought up that this is not something
> we account as a regular memory then this doesn't really fit into that
> model. But maybe I am just confused.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/19/21 5:00 PM, Michal Hocko wrote:
> On Mon 19-04-21 12:41:58, peter.enderb...@sony.com wrote:
>> On 4/19/21 2:16 PM, Michal Hocko wrote:
>>> On Sat 17-04-21 12:40:32, Peter Enderborg wrote:
 This adds a total used dma-buf memory. Details
 can be found in debugfs, however it is not for everyone
 and not always available. dma-buf are indirect allocated by
 userspace. So with this value we can monitor and detect
 userspace applications that have problems.
>>> The changelog would benefit from more background on why this is needed,
>>> and who is the primary consumer of that value.
>>>
>>> I cannot really comment on the dma-buf internals but I have two remarks.
>>> Documentation/filesystems/proc.rst needs an update with the counter
>>> explanation and secondly is this information useful for OOM situations
>>> analysis? If yes then show_mem should dump the value as well.
>>>
>>> From the implementation point of view, is there any reason why this
>>> hasn't used the existing global_node_page_state infrastructure?
>> I fix doc in next version.  Im not sure what you expect the commit message 
>> to include.
> As I've said. Usual justification covers answers to following questions
>   - Why do we need it?
>   - Why the existing data is insuficient?
>   - Who is supposed to use the data and for what?
>
> I can see an answer for the first two questions (because this can be a
> lot of memory and the existing infrastructure is not production suitable
> - debugfs). But the changelog doesn't really explain who is going to use
> the new data. Is this a monitoring to raise an early alarm when the
> value grows? Is this for debugging misbehaving drivers? How is it
> valuable for those?
>
>> The function of the meminfo is: (From Documentation/filesystems/proc.rst)
>>
>> "Provides information about distribution and utilization of memory."
> True. Yet we do not export any random counters, do we?
>
>> Im not the designed of dma-buf, I think  global_node_page_state as a kernel
>> internal.
> It provides a node specific and optimized counters. Is this a good fit
> with your new counter? Or the NUMA locality is of no importance?

Sounds good to me, if Christian Koenig think it is good, I will use that.
It is only virtio in drivers that use the global_node_page_state if
that matters.


>
>> dma-buf is a device driver that provides a function so I might be
>> on the outside. However I also see that it might be relevant for a OOM.
>> It is memory that can be freed by killing userspace processes.
>>
>> The show_mem thing. Should it be a separate patch?
> This is up to you but if you want to expose the counter then send it in
> one series.
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/19/21 2:16 PM, Michal Hocko wrote:
> On Sat 17-04-21 12:40:32, Peter Enderborg wrote:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
> The changelog would benefit from more background on why this is needed,
> and who is the primary consumer of that value.
>
> I cannot really comment on the dma-buf internals but I have two remarks.
> Documentation/filesystems/proc.rst needs an update with the counter
> explanation and secondly is this information useful for OOM situations
> analysis? If yes then show_mem should dump the value as well.
>
> From the implementation point of view, is there any reason why this
> hasn't used the existing global_node_page_state infrastructure?

I fix doc in next version.  Im not sure what you expect the commit message to 
include.

The function of the meminfo is: (From Documentation/filesystems/proc.rst)

"Provides information about distribution and utilization of memory."

Im not the designed of dma-buf, I think  global_node_page_state as a kernel
internal. dma-buf is a device driver that provides a function so I might be
on the outside. However I also see that it might be relevant for a OOM.
It is memory that can be freed by killing userspace processes.

The show_mem thing. Should it be a separate patch?




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/17/21 3:07 PM, Muchun Song wrote:
> On Sat, Apr 17, 2021 at 6:41 PM Peter Enderborg
>  wrote:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
>>
>> Signed-off-by: Peter Enderborg 
>> ---
>>  drivers/dma-buf/dma-buf.c | 13 +
>>  fs/proc/meminfo.c |  5 -
>>  include/linux/dma-buf.h   |  1 +
>>  3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index f264b70c383e..197e5c45dd26 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -37,6 +37,7 @@ struct dma_buf_list {
>>  };
>>
>>  static struct dma_buf_list db_list;
>> +static atomic_long_t dma_buf_global_allocated;
>>
>>  static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
>>  {
>> @@ -79,6 +80,7 @@ static void dma_buf_release(struct dentry *dentry)
>> if (dmabuf->resv == (struct dma_resv *)[1])
>> dma_resv_fini(dmabuf->resv);
>>
>> +   atomic_long_sub(dmabuf->size, _buf_global_allocated);
>> module_put(dmabuf->owner);
>> kfree(dmabuf->name);
>> kfree(dmabuf);
>> @@ -586,6 +588,7 @@ struct dma_buf *dma_buf_export(const struct 
>> dma_buf_export_info *exp_info)
>> mutex_lock(_list.lock);
>> list_add(>list_node, _list.head);
>> mutex_unlock(_list.lock);
>> +   atomic_long_add(dmabuf->size, _buf_global_allocated);
>>
>> return dmabuf;
>>
>> @@ -1346,6 +1349,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct 
>> dma_buf_map *map)
>>  }
>>  EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>>
>> +/**
>> + * dma_buf_allocated_pages - Return the used nr of pages
>> + * allocated for dma-buf
>> + */
>> +long dma_buf_allocated_pages(void)
>> +{
>> +   return atomic_long_read(_buf_global_allocated) >> PAGE_SHIFT;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_allocated_pages);
> dma_buf_allocated_pages is only called from fs/proc/meminfo.c.
> I am confused why it should be exported. If it won't be called
> from the driver module, we should not export it.

Ah. I thought you did not want the GPL restriction. I don't have real
opinion about it. It's written to be following the rest of the module.
It is not needed for the usage of dma-buf in kernel module. But I
don't see any reason for hiding it either.


> Thanks.
>
>> +
>>  #ifdef CONFIG_DEBUG_FS
>>  static int dma_buf_debug_show(struct seq_file *s, void *unused)
>>  {
>> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
>> index 6fa761c9cc78..ccc7c40c8db7 100644
>> --- a/fs/proc/meminfo.c
>> +++ b/fs/proc/meminfo.c
>> @@ -16,6 +16,7 @@
>>  #ifdef CONFIG_CMA
>>  #include 
>>  #endif
>> +#include 
>>  #include 
>>  #include "internal.h"
>>
>> @@ -145,7 +146,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
>> show_val_kb(m, "CmaFree:",
>> global_zone_page_state(NR_FREE_CMA_PAGES));
>>  #endif
>> -
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>> +   show_val_kb(m, "DmaBufTotal:", dma_buf_allocated_pages());
>> +#endif
>> hugetlb_report_meminfo(m);
>>
>> arch_report_meminfo(m);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index efdc56b9d95f..5b05816bd2cd 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct 
>> *,
>>  unsigned long);
>>  int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>>  void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>> +long dma_buf_allocated_pages(void);
>>  #endif /* __DMA_BUF_H__ */
>> --
>> 2.17.1
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/17/21 1:54 PM, Christian König wrote:
> Am 17.04.21 um 13:20 schrieb peter.enderb...@sony.com:
>> On 4/17/21 12:59 PM, Christian König wrote:
>>> Am 17.04.21 um 12:40 schrieb Peter Enderborg:
 This adds a total used dma-buf memory. Details
 can be found in debugfs, however it is not for everyone
 and not always available. dma-buf are indirect allocated by
 userspace. So with this value we can monitor and detect
 userspace applications that have problems.

 Signed-off-by: Peter Enderborg 
>>> Reviewed-by: Christian König 
>>>
>>> How do you want to upstream this?
>> I don't understand that question. The patch applies on Torvalds 5.12-rc7,
>> but I guess 5.13 is what we work on right now.
>
> Yeah, but how do you want to get it into Linus tree?
>
> I can push it together with other DMA-buf patches through drm-misc-next and 
> then Dave will send it to Linus for inclusion in 5.13.
>
> But could be that you are pushing multiple changes towards Linus through some 
> other branch. In this case I'm fine if you pick that way instead if you want 
> to keep your patches together for some reason.
>
It is a dma-buf functionality so it make very much sense that you as maintainer 
of dma-buf pick them the way you usually send them. I don't have any other path 
for this patch.

Thx!

Peter.


> Christian.
>
>>
 ---
    drivers/dma-buf/dma-buf.c | 13 +
    fs/proc/meminfo.c |  5 -
    include/linux/dma-buf.h   |  1 +
    3 files changed, 18 insertions(+), 1 deletion(-)

 diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
 index f264b70c383e..197e5c45dd26 100644
 --- a/drivers/dma-buf/dma-buf.c
 +++ b/drivers/dma-buf/dma-buf.c
 @@ -37,6 +37,7 @@ struct dma_buf_list {
    };
      static struct dma_buf_list db_list;
 +static atomic_long_t dma_buf_global_allocated;
      static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int 
 buflen)
    {
 @@ -79,6 +80,7 @@ static void dma_buf_release(struct dentry *dentry)
    if (dmabuf->resv == (struct dma_resv *)[1])
    dma_resv_fini(dmabuf->resv);
    +    atomic_long_sub(dmabuf->size, _buf_global_allocated);
    module_put(dmabuf->owner);
    kfree(dmabuf->name);
    kfree(dmabuf);
 @@ -586,6 +588,7 @@ struct dma_buf *dma_buf_export(const struct 
 dma_buf_export_info *exp_info)
    mutex_lock(_list.lock);
    list_add(>list_node, _list.head);
    mutex_unlock(_list.lock);
 +    atomic_long_add(dmabuf->size, _buf_global_allocated);
      return dmabuf;
    @@ -1346,6 +1349,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, 
 struct dma_buf_map *map)
    }
    EXPORT_SYMBOL_GPL(dma_buf_vunmap);
    +/**
 + * dma_buf_allocated_pages - Return the used nr of pages
 + * allocated for dma-buf
 + */
 +long dma_buf_allocated_pages(void)
 +{
 +    return atomic_long_read(_buf_global_allocated) >> PAGE_SHIFT;
 +}
 +EXPORT_SYMBOL_GPL(dma_buf_allocated_pages);
 +
    #ifdef CONFIG_DEBUG_FS
    static int dma_buf_debug_show(struct seq_file *s, void *unused)
    {
 diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
 index 6fa761c9cc78..ccc7c40c8db7 100644
 --- a/fs/proc/meminfo.c
 +++ b/fs/proc/meminfo.c
 @@ -16,6 +16,7 @@
    #ifdef CONFIG_CMA
    #include 
    #endif
 +#include 
    #include 
    #include "internal.h"
    @@ -145,7 +146,9 @@ static int meminfo_proc_show(struct seq_file *m, 
 void *v)
    show_val_kb(m, "CmaFree:    ",
    global_zone_page_state(NR_FREE_CMA_PAGES));
    #endif
 -
 +#ifdef CONFIG_DMA_SHARED_BUFFER
 +    show_val_kb(m, "DmaBufTotal:    ", dma_buf_allocated_pages());
 +#endif
    hugetlb_report_meminfo(m);
      arch_report_meminfo(m);
 diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
 index efdc56b9d95f..5b05816bd2cd 100644
 --- a/include/linux/dma-buf.h
 +++ b/include/linux/dma-buf.h
 @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct 
 vm_area_struct *,
     unsigned long);
    int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
    void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
 +long dma_buf_allocated_pages(void);
    #endif /* __DMA_BUF_H__ */
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/17/21 12:59 PM, Christian König wrote:
> Am 17.04.21 um 12:40 schrieb Peter Enderborg:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
>>
>> Signed-off-by: Peter Enderborg 
>
> Reviewed-by: Christian König 
>
> How do you want to upstream this?

I don't understand that question. The patch applies on Torvalds 5.12-rc7,
but I guess 5.13 is what we work on right now.

>
>> ---
>>   drivers/dma-buf/dma-buf.c | 13 +
>>   fs/proc/meminfo.c |  5 -
>>   include/linux/dma-buf.h   |  1 +
>>   3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index f264b70c383e..197e5c45dd26 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -37,6 +37,7 @@ struct dma_buf_list {
>>   };
>>     static struct dma_buf_list db_list;
>> +static atomic_long_t dma_buf_global_allocated;
>>     static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int 
>> buflen)
>>   {
>> @@ -79,6 +80,7 @@ static void dma_buf_release(struct dentry *dentry)
>>   if (dmabuf->resv == (struct dma_resv *)[1])
>>   dma_resv_fini(dmabuf->resv);
>>   +    atomic_long_sub(dmabuf->size, _buf_global_allocated);
>>   module_put(dmabuf->owner);
>>   kfree(dmabuf->name);
>>   kfree(dmabuf);
>> @@ -586,6 +588,7 @@ struct dma_buf *dma_buf_export(const struct 
>> dma_buf_export_info *exp_info)
>>   mutex_lock(_list.lock);
>>   list_add(>list_node, _list.head);
>>   mutex_unlock(_list.lock);
>> +    atomic_long_add(dmabuf->size, _buf_global_allocated);
>>     return dmabuf;
>>   @@ -1346,6 +1349,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct 
>> dma_buf_map *map)
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>>   +/**
>> + * dma_buf_allocated_pages - Return the used nr of pages
>> + * allocated for dma-buf
>> + */
>> +long dma_buf_allocated_pages(void)
>> +{
>> +    return atomic_long_read(_buf_global_allocated) >> PAGE_SHIFT;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_allocated_pages);
>> +
>>   #ifdef CONFIG_DEBUG_FS
>>   static int dma_buf_debug_show(struct seq_file *s, void *unused)
>>   {
>> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
>> index 6fa761c9cc78..ccc7c40c8db7 100644
>> --- a/fs/proc/meminfo.c
>> +++ b/fs/proc/meminfo.c
>> @@ -16,6 +16,7 @@
>>   #ifdef CONFIG_CMA
>>   #include 
>>   #endif
>> +#include 
>>   #include 
>>   #include "internal.h"
>>   @@ -145,7 +146,9 @@ static int meminfo_proc_show(struct seq_file *m, void 
>> *v)
>>   show_val_kb(m, "CmaFree:    ",
>>   global_zone_page_state(NR_FREE_CMA_PAGES));
>>   #endif
>> -
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>> +    show_val_kb(m, "DmaBufTotal:    ", dma_buf_allocated_pages());
>> +#endif
>>   hugetlb_report_meminfo(m);
>>     arch_report_meminfo(m);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index efdc56b9d95f..5b05816bd2cd 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct 
>> *,
>>    unsigned long);
>>   int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>>   void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>> +long dma_buf_allocated_pages(void);
>>   #endif /* __DMA_BUF_H__ */
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] [PATCH v3] dma-buf: Add DmaBufTotal counter in meminfo

[Peter.Enderborg]

On 4/17/21 5:05 AM, Muchun Song wrote:
> On Sat, Apr 17, 2021 at 12:08 AM Peter Enderborg
>  wrote:
>> This adds a total used dma-buf memory. Details
>> can be found in debugfs, however it is not for everyone
>> and not always available. dma-buf are indirect allocated by
>> userspace. So with this value we can monitor and detect
>> userspace applications that have problems.
> I want to know more details about the problems.
> Can you share what problems you have encountered?
>
> Thanks.

What do you expect to be relevant for the kernel? Applications that leaks
is not that important. This types of buffers are important for android
applications, and android have moved a from ION buffers that has
metrics. It easily get in to 5-10 percent of the total amount ram.

This provide that information for end users or application developers
using commercial devices.  The end user get to know why their device
is running out of memory.


>> Signed-off-by: Peter Enderborg 
>> ---
>>  drivers/dma-buf/dma-buf.c | 12 
>>  fs/proc/meminfo.c |  5 -
>>  include/linux/dma-buf.h   |  1 +
>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index f264b70c383e..d40fff2ae1fa 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -37,6 +37,7 @@ struct dma_buf_list {
>>  };
>>
>>  static struct dma_buf_list db_list;
>> +static atomic_long_t dma_buf_global_allocated;
>>
>>  static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
>>  {
>> @@ -79,6 +80,7 @@ static void dma_buf_release(struct dentry *dentry)
>> if (dmabuf->resv == (struct dma_resv *)[1])
>> dma_resv_fini(dmabuf->resv);
>>
>> +   atomic_long_sub(dmabuf->size, _buf_global_allocated);
>> module_put(dmabuf->owner);
>> kfree(dmabuf->name);
>> kfree(dmabuf);
>> @@ -586,6 +588,7 @@ struct dma_buf *dma_buf_export(const struct 
>> dma_buf_export_info *exp_info)
>> mutex_lock(_list.lock);
>> list_add(>list_node, _list.head);
>> mutex_unlock(_list.lock);
>> +   atomic_long_add(dmabuf->size, _buf_global_allocated);
>>
>> return dmabuf;
>>
>> @@ -1346,6 +1349,15 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct 
>> dma_buf_map *map)
>>  }
>>  EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>>
>> +/**
>> + * dma_buf_get_size - Return the used nr pages by dma-buf
>> + */
>> +long dma_buf_allocated_pages(void)
>> +{
>> +   return atomic_long_read(_buf_global_allocated) >> PAGE_SHIFT;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_allocated_pages);
> Why need "EXPORT_SYMBOL_GPL"?
This what all other exported functions for this module are. I don't see any 
reason for this do be different.
>
>> +
>>  #ifdef CONFIG_DEBUG_FS
>>  static int dma_buf_debug_show(struct seq_file *s, void *unused)
>>  {
>> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
>> index 6fa761c9cc78..ccc7c40c8db7 100644
>> --- a/fs/proc/meminfo.c
>> +++ b/fs/proc/meminfo.c
>> @@ -16,6 +16,7 @@
>>  #ifdef CONFIG_CMA
>>  #include 
>>  #endif
>> +#include 
>>  #include 
>>  #include "internal.h"
>>
>> @@ -145,7 +146,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
>> show_val_kb(m, "CmaFree:",
>> global_zone_page_state(NR_FREE_CMA_PAGES));
>>  #endif
>> -
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>> +   show_val_kb(m, "DmaBufTotal:", dma_buf_allocated_pages());
>> +#endif
>> hugetlb_report_meminfo(m);
>>
>> arch_report_meminfo(m);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index efdc56b9d95f..5b05816bd2cd 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct 
>> *,
>>  unsigned long);
>>  int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>>  void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
>> +long dma_buf_allocated_pages(void);
>>  #endif /* __DMA_BUF_H__ */
>> --
>> 2.17.1
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel