[PATCH] mm: cma: print cma name as well in cma_alloc debug

[PATCH] mm: cma: print cma name as well in cma_alloc debug

[Pintu Kumar]

CMA allocation can happen either from global cma or from
dedicated cma region.

Thus it is helpful to print cma name as well during initial
debugging to confirm cma regions were getting initialized or not.

Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com>
Signed-off-by: Pintu Agarwal <pintu.ping@gmail.com>
---
 mm/cma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/cma.c b/mm/cma.c
index a4cfe99..96718b53 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -436,8 +436,8 @@ struct page *cma_alloc(struct cma *cma, unsigned long count,
 	if (!cma || !cma->count || !cma->bitmap)
 		goto out;
 
-	pr_debug("%s(cma %p, count %lu, align %d)\n", __func__, (void *)cma,
-		 count, align);
+	pr_info("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
+		(void *)cma, cma->name, count, align);
 
 	if (!count)
 		goto out;
-- 
2.7.4

[PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Pintu Kumar]

CMA allocation can happen either from global cma or from
dedicated cma region.

Thus it is helpful to print cma name as well during initial
debugging to confirm cma regions were getting initialized or not.

Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com>
Signed-off-by: Pintu Agarwal <pintu.ping@gmail.com>
---
 mm/cma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/cma.c b/mm/cma.c
index a4cfe99..4880f72 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -436,8 +436,8 @@ struct page *cma_alloc(struct cma *cma, unsigned long count,
 	if (!cma || !cma->count || !cma->bitmap)
 		goto out;
 
-	pr_debug("%s(cma %p, count %lu, align %d)\n", __func__, (void *)cma,
-		 count, align);
+	pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
+		(void *)cma, cma->name, count, align);
 
 	if (!count)
 		goto out;
-- 
2.7.4

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Anshuman Khandual]

On 7/7/23 00:03, Pintu Kumar wrote:
> CMA allocation can happen either from global cma or from
> dedicated cma region.
> 
> Thus it is helpful to print cma name as well during initial
> debugging to confirm cma regions were getting initialized or not.
> 
> Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com>
> Signed-off-by: Pintu Agarwal <pintu.ping@gmail.com>
> ---
>  mm/cma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index a4cfe99..4880f72 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -436,8 +436,8 @@ struct page *cma_alloc(struct cma *cma, unsigned long count,
>  	if (!cma || !cma->count || !cma->bitmap)
>  		goto out;
>  
> -	pr_debug("%s(cma %p, count %lu, align %d)\n", __func__, (void *)cma,
> -		 count, align);
> +	pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
> +		(void *)cma, cma->name, count, align);
>  
>  	if (!count)
>  		goto out;

LGTM, cma->name is an identifying attribute for the region for which the allocation
request was made. But how about using cma_get_name() helper instead ? Very few call
sites have been using the helper.

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Matthew Wilcox]

On Fri, Jul 07, 2023 at 03:57:42PM +0530, Anshuman Khandual wrote:
> LGTM, cma->name is an identifying attribute for the region for which the allocation
> request was made. But how about using cma_get_name() helper instead ? Very few call
> sites have been using the helper.

It's not really a "helper", is it?  The function name is longer than
its implementation.

cma_get_name(cma)
vs
cma->name

Plus there's the usual question about whether a "got" name needs to be
"put" (does it grab a refcount?)

I think it's useful that this function exists since it lets us not expose
struct cma outside of mm/, but it really should be called cma_name()
and I don't think we should be encouraging its use within cma.c.

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Pintu Agarwal]

On Fri, 7 Jul 2023 at 18:16, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Jul 07, 2023 at 03:57:42PM +0530, Anshuman Khandual wrote:
> > LGTM, cma->name is an identifying attribute for the region for which the allocation
> > request was made. But how about using cma_get_name() helper instead ? Very few call
> > sites have been using the helper.
>
> It's not really a "helper", is it?  The function name is longer than
> its implementation.
>
> cma_get_name(cma)
> vs
> cma->name
>
> Plus there's the usual question about whether a "got" name needs to be
> "put" (does it grab a refcount?)
>
> I think it's useful that this function exists since it lets us not expose
> struct cma outside of mm/, but it really should be called cma_name()
> and I don't think we should be encouraging its use within cma.c.

Also, cma_get_name() is a trivial assignment.
And in one of the previous patches we avoided function calls with
trivial assignments.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/kernel/dma/contiguous.c?h=next-20230705&id=5af638931eb374aa0894d8343cee72f50307ef20
dma-contiguous: remove dev_set_cma_area

One more question from here:
pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
                (void *)cma, cma->name, count, align);

Do we really need this "cma %p" printing ?
I hardly check it and simply rely on name and count.

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Matthew Wilcox]

On Fri, Jul 07, 2023 at 07:36:20PM +0530, Pintu Agarwal wrote:
> On Fri, 7 Jul 2023 at 18:16, Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, Jul 07, 2023 at 03:57:42PM +0530, Anshuman Khandual wrote:
> > > LGTM, cma->name is an identifying attribute for the region for which the allocation
> > > request was made. But how about using cma_get_name() helper instead ? Very few call
> > > sites have been using the helper.
> >
> > It's not really a "helper", is it?  The function name is longer than
> > its implementation.
> >
> > cma_get_name(cma)
> > vs
> > cma->name
> >
> > Plus there's the usual question about whether a "got" name needs to be
> > "put" (does it grab a refcount?)
> >
> > I think it's useful that this function exists since it lets us not expose
> > struct cma outside of mm/, but it really should be called cma_name()
> > and I don't think we should be encouraging its use within cma.c.
> 
> Also, cma_get_name() is a trivial assignment.
> And in one of the previous patches we avoided function calls with
> trivial assignments.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/kernel/dma/contiguous.c?h=next-20230705&id=5af638931eb374aa0894d8343cee72f50307ef20
> dma-contiguous: remove dev_set_cma_area
> 
> One more question from here:
> pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
>                 (void *)cma, cma->name, count, align);
> 
> Do we really need this "cma %p" printing ?
> I hardly check it and simply rely on name and count.

Printing pointers is almost always a bad idea.  Printing the base_pfn
might be a good idea to distinguish CMAs which happen to have the
same name?

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Pintu Agarwal]

On Fri, 7 Jul 2023 at 19:40, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Jul 07, 2023 at 07:36:20PM +0530, Pintu Agarwal wrote:
> > On Fri, 7 Jul 2023 at 18:16, Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Fri, Jul 07, 2023 at 03:57:42PM +0530, Anshuman Khandual wrote:
> > > > LGTM, cma->name is an identifying attribute for the region for which the allocation
> > > > request was made. But how about using cma_get_name() helper instead ? Very few call
> > > > sites have been using the helper.
> > >
> > > It's not really a "helper", is it?  The function name is longer than
> > > its implementation.
> > >
> > > cma_get_name(cma)
> > > vs
> > > cma->name
> > >
> > > Plus there's the usual question about whether a "got" name needs to be
> > > "put" (does it grab a refcount?)
> > >
> > > I think it's useful that this function exists since it lets us not expose
> > > struct cma outside of mm/, but it really should be called cma_name()
> > > and I don't think we should be encouraging its use within cma.c.
> >
> > Also, cma_get_name() is a trivial assignment.
> > And in one of the previous patches we avoided function calls with
> > trivial assignments.
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/kernel/dma/contiguous.c?h=next-20230705&id=5af638931eb374aa0894d8343cee72f50307ef20
> > dma-contiguous: remove dev_set_cma_area
> >
> > One more question from here:
> > pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
> >                 (void *)cma, cma->name, count, align);
> >
> > Do we really need this "cma %p" printing ?
> > I hardly check it and simply rely on name and count.
>
> Printing pointers is almost always a bad idea.  Printing the base_pfn
> might be a good idea to distinguish CMAs which happen to have the
> same name?
>
No there is no name there, it's just a ptrval
cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Matthew Wilcox]

On Fri, Jul 07, 2023 at 07:46:31PM +0530, Pintu Agarwal wrote:
> On Fri, 7 Jul 2023 at 19:40, Matthew Wilcox <willy@infradead.org> wrote:
> > > One more question from here:
> > > pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
> > >                 (void *)cma, cma->name, count, align);
> > >
> > > Do we really need this "cma %p" printing ?
> > > I hardly check it and simply rely on name and count.
> >
> > Printing pointers is almost always a bad idea.  Printing the base_pfn
> > might be a good idea to distinguish CMAs which happen to have the
> > same name?
> >
> No there is no name there, it's just a ptrval
> cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)

You misunderstand me.  I don't know how CMAs get their name.  Is it not
possible for two CMAs to have the same name as each other?

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Pintu Agarwal]

On Fri, 7 Jul 2023 at 19:52, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Jul 07, 2023 at 07:46:31PM +0530, Pintu Agarwal wrote:
> > On Fri, 7 Jul 2023 at 19:40, Matthew Wilcox <willy@infradead.org> wrote:
> > > > One more question from here:
> > > > pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
> > > >                 (void *)cma, cma->name, count, align);
> > > >
> > > > Do we really need this "cma %p" printing ?
> > > > I hardly check it and simply rely on name and count.
> > >
> > > Printing pointers is almost always a bad idea.  Printing the base_pfn
> > > might be a good idea to distinguish CMAs which happen to have the
> > > same name?
> > >
> > No there is no name there, it's just a ptrval
> > cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
>
> You misunderstand me.  I don't know how CMAs get their name.  Is it not
> possible for two CMAs to have the same name as each other?
>
Oh yah that is possible, for example multiple players can use the same
global cma region.

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Pintu Agarwal]

On Fri, 7 Jul 2023 at 20:03, Pintu Agarwal <pintu.ping@gmail.com> wrote:
>
> On Fri, 7 Jul 2023 at 19:52, Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, Jul 07, 2023 at 07:46:31PM +0530, Pintu Agarwal wrote:
> > > On Fri, 7 Jul 2023 at 19:40, Matthew Wilcox <willy@infradead.org> wrote:
> > > > > One more question from here:
> > > > > pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
> > > > >                 (void *)cma, cma->name, count, align);
> > > > >
> > > > > Do we really need this "cma %p" printing ?
> > > > > I hardly check it and simply rely on name and count.
> > > >
> > > > Printing pointers is almost always a bad idea.  Printing the base_pfn
> > > > might be a good idea to distinguish CMAs which happen to have the
> > > > same name?
> > > >
> > > No there is no name there, it's just a ptrval
> > > cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
> >
> > You misunderstand me.  I don't know how CMAs get their name.  Is it not
> > possible for two CMAs to have the same name as each other?
> >
> Oh yah that is possible, for example multiple players can use the same
> global cma region.

Yes, I think it's a good idea to include base_pfn instead of printing pointers.
Let's complete the cma->name inclusion first.
Later I will check about base_pfn and push in another patch.
Thanks

Re: [PATCH v2] mm: cma: print cma name as well in cma_alloc debug

[Pintu Agarwal]

On Sat, 8 Jul 2023 at 12:22, Pintu Agarwal <pintu.ping@gmail.com> wrote:
>
> On Fri, 7 Jul 2023 at 20:03, Pintu Agarwal <pintu.ping@gmail.com> wrote:
> >
> > On Fri, 7 Jul 2023 at 19:52, Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Fri, Jul 07, 2023 at 07:46:31PM +0530, Pintu Agarwal wrote:
> > > > On Fri, 7 Jul 2023 at 19:40, Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > One more question from here:
> > > > > > pr_debug("%s(cma %p, name: %s, count %lu, align %d)\n", __func__,
> > > > > >                 (void *)cma, cma->name, count, align);
> > > > > >
> > > > > > Do we really need this "cma %p" printing ?
> > > > > > I hardly check it and simply rely on name and count.
> > > > >
> > > > > Printing pointers is almost always a bad idea.  Printing the base_pfn
> > > > > might be a good idea to distinguish CMAs which happen to have the
> > > > > same name?
> > > > >
> > > > No there is no name there, it's just a ptrval
> > > > cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
> > >
> > > You misunderstand me.  I don't know how CMAs get their name.  Is it not
> > > possible for two CMAs to have the same name as each other?
> > >
> > Oh yah that is possible, for example multiple players can use the same
> > global cma region.
>
> Yes, I think it's a good idea to include base_pfn instead of printing pointers.
> Let's complete the cma->name inclusion first.
> Later I will check about base_pfn and push in another patch.
> Thanks

I hope we are good with printing cma->name here ?