[v2] mm/cma: print total and used count in cma_alloc()

[PATCH v2] mm/cma: print total and used count in cma_alloc()

Posted by Xiang Gao 2 months ago

From: gaoxiang17 <gaoxiang17@xiaomi.com>

before:
[   24.407814] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
[   24.413397] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
[   24.415886] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)

after:
[   24.069738] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 64, request pages: 1, align 0)
[   24.075317] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 65, request pages: 1, align 0)
[   24.078455] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 66, request pages: 1, align 0)

Signed-off-by: gaoxiang17 <gaoxiang17@xiaomi.com>
---
 mm/cma.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/mm/cma.c b/mm/cma.c
index 2d9fae939283..90b3fdbac19c 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -403,6 +403,17 @@ static void cma_debug_show_areas(struct cma *cma)
 	spin_unlock_irq(&cma->lock);
 }
 
+static unsigned long cma_get_used_pages(struct cma *cma)
+{
+	unsigned long used;
+
+	spin_lock_irq(&cma->lock);
+	used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
+	spin_unlock_irq(&cma->lock);
+
+	return used << cma->order_per_bit;
+}
+
 static struct page *__cma_alloc(struct cma *cma, unsigned long count,
 				unsigned int align, gfp_t gfp)
 {
@@ -420,8 +431,8 @@ static struct page *__cma_alloc(struct cma *cma, unsigned long count,
 	if (!cma || !cma->count || !cma->bitmap)
 		return page;
 
-	pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
-		(void *)cma, cma->name, count, align);
+	pr_debug("%s(cma %p, name: %s, total pages: %lu, used pages: %lu, request pages: %lu, align %d)\n", __func__,
+		(void *)cma, cma->name, cma->count, cma_get_used_pages(cma), count, align);
 
 	if (!count)
 		return page;
-- 
2.34.1

Re: [PATCH v2] mm/cma: print total and used count in cma_alloc()

Posted by David Hildenbrand 1 month, 4 weeks ago

On 29.09.24 05:27, Xiang Gao wrote:
> From: gaoxiang17 <gaoxiang17@xiaomi.com>
> 

We should add here

"To debug CMA allocations (especially failing ones), it is valuable to 
know the state of CMA: how many pages out of the total ones are 
allocated, and how many were requested to be allocated. Let's print
some more information."

I assume Andrew can fix that up when applying.

> before:
> [   24.407814] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> [   24.413397] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> [   24.415886] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> 
> after:
> [   24.069738] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 64, request pages: 1, align 0)
> [   24.075317] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 65, request pages: 1, align 0)
> [   24.078455] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 66, request pages: 1, align 0)
> 
> Signed-off-by: gaoxiang17 <gaoxiang17@xiaomi.com>
> ---
>   mm/cma.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index 2d9fae939283..90b3fdbac19c 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -403,6 +403,17 @@ static void cma_debug_show_areas(struct cma *cma)
>   	spin_unlock_irq(&cma->lock);
>   }
>   
> +static unsigned long cma_get_used_pages(struct cma *cma)
> +{
> +	unsigned long used;
> +
> +	spin_lock_irq(&cma->lock);
> +	used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
> +	spin_unlock_irq(&cma->lock);

This adds overhead to each allocation, even if debug outputs are ignored 
I assume?

I wonder if we'd want to print these details only when our allocation 
failed?

Alternatively, we could actually track how many pages are allocated in 
the cma, so we don't have to traverse the complete bitmap on every 
allocation.


-- 
Cheers,

David / dhildenb

Re: [PATCH v2] mm/cma: print total and used count in cma_alloc()

Posted by Frank van der Linden 1 month, 3 weeks ago

On Mon, Sep 30, 2024 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 29.09.24 05:27, Xiang Gao wrote:
> > From: gaoxiang17 <gaoxiang17@xiaomi.com>
> >
>
> We should add here
>
> "To debug CMA allocations (especially failing ones), it is valuable to
> know the state of CMA: how many pages out of the total ones are
> allocated, and how many were requested to be allocated. Let's print
> some more information."
>
> I assume Andrew can fix that up when applying.
>
> > before:
> > [   24.407814] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> > [   24.413397] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> > [   24.415886] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> >
> > after:
> > [   24.069738] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 64, request pages: 1, align 0)
> > [   24.075317] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 65, request pages: 1, align 0)
> > [   24.078455] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 66, request pages: 1, align 0)
> >
> > Signed-off-by: gaoxiang17 <gaoxiang17@xiaomi.com>
> > ---
> >   mm/cma.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/cma.c b/mm/cma.c
> > index 2d9fae939283..90b3fdbac19c 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -403,6 +403,17 @@ static void cma_debug_show_areas(struct cma *cma)
> >       spin_unlock_irq(&cma->lock);
> >   }
> >
> > +static unsigned long cma_get_used_pages(struct cma *cma)
> > +{
> > +     unsigned long used;
> > +
> > +     spin_lock_irq(&cma->lock);
> > +     used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
> > +     spin_unlock_irq(&cma->lock);
>
> This adds overhead to each allocation, even if debug outputs are ignored
> I assume?
>
> I wonder if we'd want to print these details only when our allocation
> failed?
>
> Alternatively, we could actually track how many pages are allocated in
> the cma, so we don't have to traverse the complete bitmap on every
> allocation.
>

Yep, that's what I did as part of
https://lore.kernel.org/all/20240724124845.614c03ad39f8af3729cebee6@linux-foundation.org/T/

That patch didn't make it in (yet). I'm happy for it to be combined
with this one if that's easier.

- Frank

回复: [External Mail]Re: [PATCH v2] mm/cma: print total and used count in cma_alloc()

Posted by 高翔 1 month, 3 weeks ago

-----邮件原件-----
发件人: Frank van der Linden <fvdl@google.com> 
发送时间: 2024年10月3日 3:50
收件人: David Hildenbrand <david@redhat.com>
抄送: Xiang Gao <gxxa03070307@gmail.com>; akpm@linux-foundation.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org; 高翔 <gaoxiang17@xiaomi.com>
主题: [External Mail]Re: [PATCH v2] mm/cma: print total and used count in cma_alloc()

[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈

On Mon, Sep 30, 2024 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 29.09.24 05:27, Xiang Gao wrote:
> > From: gaoxiang17 <gaoxiang17@xiaomi.com>
> >
>
> We should add here
>
> "To debug CMA allocations (especially failing ones), it is valuable to 
> know the state of CMA: how many pages out of the total ones are 
> allocated, and how many were requested to be allocated. Let's print 
> some more information."
>
> I assume Andrew can fix that up when applying.
>
> > before:
> > [   24.407814] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> > [   24.413397] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> > [   24.415886] cma: cma_alloc(cma (____ptrval____), name: reserved, count 1, align 0)
> >
> > after:
> > [   24.069738] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 64, request pages: 1, align 0)
> > [   24.075317] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 65, request pages: 1, align 0)
> > [   24.078455] cma: cma_alloc(cma (____ptrval____), name: reserved, total pages: 16384, used pages: 66, request pages: 1, align 0)
> >
> > Signed-off-by: gaoxiang17 <gaoxiang17@xiaomi.com>
> > ---
> >   mm/cma.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/cma.c b/mm/cma.c
> > index 2d9fae939283..90b3fdbac19c 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -403,6 +403,17 @@ static void cma_debug_show_areas(struct cma *cma)
> >       spin_unlock_irq(&cma->lock);
> >   }
> >
> > +static unsigned long cma_get_used_pages(struct cma *cma) {
> > +     unsigned long used;
> > +
> > +     spin_lock_irq(&cma->lock);
> > +     used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
> > +     spin_unlock_irq(&cma->lock);
>
> This adds overhead to each allocation, even if debug outputs are 
> ignored I assume?
>
> I wonder if we'd want to print these details only when our allocation 
> failed?
>
> Alternatively, we could actually track how many pages are allocated in 
> the cma, so we don't have to traverse the complete bitmap on every 
> allocation.
>

Yep, that's what I did as part of
https://lore.kernel.org/all/20240724124845.614c03ad39f8af3729cebee6@linux-foundation.org/T/

That patch didn't make it in (yet). I'm happy for it to be combined with this one if that's easier.

- Frank

Yes, I think it's simpler to add new members to the cma structure to manage. 
This does not add much overhead to the used count per print.

If this is possible, we can also add "caller" members to troubleshoot problems.

Re: 回复: [External Mail]Re: [PATCH v2] mm/cma: print total and used count in cma_alloc()

Posted by Andrew Morton 1 month, 3 weeks ago

On Fri, 4 Oct 2024 12:23:30 +0000 高翔 <gaoxiang17@xiaomi.com> wrote:

> > > +static unsigned long cma_get_used_pages(struct cma *cma) {
> > > +     unsigned long used;
> > > +
> > > +     spin_lock_irq(&cma->lock);
> > > +     used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
> > > +     spin_unlock_irq(&cma->lock);
> >
> > This adds overhead to each allocation, even if debug outputs are 
> > ignored I assume?
> >
> > I wonder if we'd want to print these details only when our allocation 
> > failed?
> >
> > Alternatively, we could actually track how many pages are allocated in 
> > the cma, so we don't have to traverse the complete bitmap on every 
> > allocation.
> >
> 
> Yep, that's what I did as part of
> https://lore.kernel.org/all/20240724124845.614c03ad39f8af3729cebee6@linux-foundation.org/T/
> 
> That patch didn't make it in (yet). I'm happy for it to be combined with this one if that's easier.

That patch has been forgotten about.  As I asked in July,
"I suggest a resend, and add some Cc:s for likely reviewers."

Re: 回复: [External Mail]Re: [PATCH v2] mm/cma: print total and used count in cma_alloc()

Posted by Frank van der Linden 1 month, 3 weeks ago

On Fri, Oct 4, 2024 at 3:39 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 4 Oct 2024 12:23:30 +0000 高翔 <gaoxiang17@xiaomi.com> wrote:
>
> > > > +static unsigned long cma_get_used_pages(struct cma *cma) {
> > > > +     unsigned long used;
> > > > +
> > > > +     spin_lock_irq(&cma->lock);
> > > > +     used = bitmap_weight(cma->bitmap, (int)cma_bitmap_maxno(cma));
> > > > +     spin_unlock_irq(&cma->lock);
> > >
> > > This adds overhead to each allocation, even if debug outputs are
> > > ignored I assume?
> > >
> > > I wonder if we'd want to print these details only when our allocation
> > > failed?
> > >
> > > Alternatively, we could actually track how many pages are allocated in
> > > the cma, so we don't have to traverse the complete bitmap on every
> > > allocation.
> > >
> >
> > Yep, that's what I did as part of
> > https://lore.kernel.org/all/20240724124845.614c03ad39f8af3729cebee6@linux-foundation.org/T/
> >
> > That patch didn't make it in (yet). I'm happy for it to be combined with this one if that's easier.
>
> That patch has been forgotten about.  As I asked in July,
> "I suggest a resend, and add some Cc:s for likely reviewers."

Indeed - I certainly wasn't suggesting that anyone else forgot about
it, it's up to me to follow up here, and I haven't yet.

- Frank