diff mbox series

mm: fix printk format within cma

Message ID 1681788789-19679-1-git-send-email-zhaoyang.huang@unisoc.com
State New
Headers
Series mm: fix printk format within cma |

Commit Message

zhaoyang.huang April 18, 2023, 3:33 a.m. UTC 
  From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

cma and page pointer printed via %p are hash value which make debug to be hard.
change them to %px.

[63321.482751] [c7] cma: cma_alloc(): memory range at 000000000b5e462c is busy, retrying
[63321.482786] [c7] cma: cma_alloc(): memory range at 000000000f7d6fae is busy, retrying
[63321.482823] [c7] cma: cma_alloc(): memory range at 00000000e653b59b is busy, retrying
[63322.378890] [c7] cma: cma_release(page 00000000dd53cf48)
[63322.378913] [c7] cma: cma_release(page 00000000315f703d)
[63322.378925] [c7] cma: cma_release(page 00000000791e3a5f)

Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
 mm/cma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox April 18, 2023, 3:38 a.m. UTC | #1 
On Tue, Apr 18, 2023 at 11:33:09AM +0800, zhaoyang.huang wrote:
> cma and page pointer printed via %p are hash value which make debug to be hard.
> change them to %px.

Why does printing the page pointer make any sense at all?  Surely the
PFN makes much more sense.

> [63321.482751] [c7] cma: cma_alloc(): memory range at 000000000b5e462c is busy, retrying
> [63321.482786] [c7] cma: cma_alloc(): memory range at 000000000f7d6fae is busy, retrying
> [63321.482823] [c7] cma: cma_alloc(): memory range at 00000000e653b59b is busy, retrying
> [63322.378890] [c7] cma: cma_release(page 00000000dd53cf48)
> [63322.378913] [c7] cma: cma_release(page 00000000315f703d)
> [63322.378925] [c7] cma: cma_release(page 00000000791e3a5f)
> 
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
>  mm/cma.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index 4a978e0..dfe9813 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -435,7 +435,7 @@ 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,
> +	pr_debug("%s(cma %px, count %lu, align %d)\n", __func__, (void *)cma,
>  		 count, align);
>  
>  	if (!count)
> @@ -534,7 +534,7 @@ bool cma_pages_valid(struct cma *cma, const struct page *pages,
>  	pfn = page_to_pfn(pages);
>  
>  	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count) {
> -		pr_debug("%s(page %p, count %lu)\n", __func__,
> +		pr_debug("%s(page %px, count %lu)\n", __func__,
>  						(void *)pages, count);
>  		return false;
>  	}
> @@ -560,7 +560,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
>  	if (!cma_pages_valid(cma, pages, count))
>  		return false;
>  
> -	pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
> +	pr_debug("%s(page %px, count %lu)\n", __func__, (void *)pages, count);
>  
>  	pfn = page_to_pfn(pages);
>  
> -- 
> 1.9.1
> 
>
Zhaoyang Huang April 18, 2023, 4:16 a.m. UTC | #2 
On Tue, Apr 18, 2023 at 11:38 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Apr 18, 2023 at 11:33:09AM +0800, zhaoyang.huang wrote:
> > cma and page pointer printed via %p are hash value which make debug to be hard.
> > change them to %px.
>
> Why does printing the page pointer make any sense at all?  Surely the
> PFN makes much more sense.
either pfn or a correct page pointer makes sense for debugging, while
page could be more safe than pfn which expose the paddr directly
>
> > [63321.482751] [c7] cma: cma_alloc(): memory range at 000000000b5e462c is busy, retrying
> > [63321.482786] [c7] cma: cma_alloc(): memory range at 000000000f7d6fae is busy, retrying
> > [63321.482823] [c7] cma: cma_alloc(): memory range at 00000000e653b59b is busy, retrying
> > [63322.378890] [c7] cma: cma_release(page 00000000dd53cf48)
> > [63322.378913] [c7] cma: cma_release(page 00000000315f703d)
> > [63322.378925] [c7] cma: cma_release(page 00000000791e3a5f)
> >
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > ---
> >  mm/cma.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/cma.c b/mm/cma.c
> > index 4a978e0..dfe9813 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -435,7 +435,7 @@ 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,
> > +     pr_debug("%s(cma %px, count %lu, align %d)\n", __func__, (void *)cma,
> >                count, align);
> >
> >       if (!count)
> > @@ -534,7 +534,7 @@ bool cma_pages_valid(struct cma *cma, const struct page *pages,
> >       pfn = page_to_pfn(pages);
> >
> >       if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count) {
> > -             pr_debug("%s(page %p, count %lu)\n", __func__,
> > +             pr_debug("%s(page %px, count %lu)\n", __func__,
> >                                               (void *)pages, count);
> >               return false;
> >       }
> > @@ -560,7 +560,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
> >       if (!cma_pages_valid(cma, pages, count))
> >               return false;
> >
> > -     pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
> > +     pr_debug("%s(page %px, count %lu)\n", __func__, (void *)pages, count);
> >
> >       pfn = page_to_pfn(pages);
> >
> > --
> > 1.9.1
> >
> >
Huang, Ying April 18, 2023, 5:24 a.m. UTC | #3 
Zhaoyang Huang <huangzhaoyang@gmail.com> writes:

> On Tue, Apr 18, 2023 at 11:38 AM Matthew Wilcox <willy@infradead.org> wrote:
>>
>> On Tue, Apr 18, 2023 at 11:33:09AM +0800, zhaoyang.huang wrote:
>> > cma and page pointer printed via %p are hash value which make debug to be hard.
>> > change them to %px.
>>
>> Why does printing the page pointer make any sense at all?  Surely the
>> PFN makes much more sense.
> either pfn or a correct page pointer makes sense for debugging, while
> page could be more safe than pfn which expose the paddr directly

You can specify "no_hash_pointers" in kernel command line to print
pointer value for debug.  IIUC, this take care of both security and
debuggability.

Best Regards,
Huang, Ying

>>
>> > [63321.482751] [c7] cma: cma_alloc(): memory range at 000000000b5e462c is busy, retrying
>> > [63321.482786] [c7] cma: cma_alloc(): memory range at 000000000f7d6fae is busy, retrying
>> > [63321.482823] [c7] cma: cma_alloc(): memory range at 00000000e653b59b is busy, retrying
>> > [63322.378890] [c7] cma: cma_release(page 00000000dd53cf48)
>> > [63322.378913] [c7] cma: cma_release(page 00000000315f703d)
>> > [63322.378925] [c7] cma: cma_release(page 00000000791e3a5f)
>> >
>> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
>> > ---
>> >  mm/cma.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/mm/cma.c b/mm/cma.c
>> > index 4a978e0..dfe9813 100644
>> > --- a/mm/cma.c
>> > +++ b/mm/cma.c
>> > @@ -435,7 +435,7 @@ 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,
>> > +     pr_debug("%s(cma %px, count %lu, align %d)\n", __func__, (void *)cma,
>> >                count, align);
>> >
>> >       if (!count)
>> > @@ -534,7 +534,7 @@ bool cma_pages_valid(struct cma *cma, const struct page *pages,
>> >       pfn = page_to_pfn(pages);
>> >
>> >       if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count) {
>> > -             pr_debug("%s(page %p, count %lu)\n", __func__,
>> > +             pr_debug("%s(page %px, count %lu)\n", __func__,
>> >                                               (void *)pages, count);
>> >               return false;
>> >       }
>> > @@ -560,7 +560,7 @@ bool cma_release(struct cma *cma, const struct page *pages,
>> >       if (!cma_pages_valid(cma, pages, count))
>> >               return false;
>> >
>> > -     pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
>> > +     pr_debug("%s(page %px, count %lu)\n", __func__, (void *)pages, count);
>> >
>> >       pfn = page_to_pfn(pages);
>> >
>> > --
>> > 1.9.1
>> >
>> >
Andrew Morton April 18, 2023, 7:33 p.m. UTC | #4 
On Tue, 18 Apr 2023 04:38:24 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Apr 18, 2023 at 11:33:09AM +0800, zhaoyang.huang wrote:
> > cma and page pointer printed via %p are hash value which make debug to be hard.
> > change them to %px.
> 
> Why does printing the page pointer make any sense at all?  Surely the
> PFN makes much more sense.

I suppose one could correlate a particular hashed pointer with other
debug output, see "ah, that's the same page".  In which case one
doesn't really care whether or not the address is hashed - it's just a
cookie.  This sounds thin.

I doubt if a lot of thought went into the printk.  If the page pointer
isn't useful then how about we simply remove it from the message?
Matthew Wilcox April 28, 2023, 2:35 p.m. UTC | #5 
On Tue, Apr 18, 2023 at 12:33:38PM -0700, Andrew Morton wrote:
> On Tue, 18 Apr 2023 04:38:24 +0100 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Tue, Apr 18, 2023 at 11:33:09AM +0800, zhaoyang.huang wrote:
> > > cma and page pointer printed via %p are hash value which make debug to be hard.
> > > change them to %px.
> > 
> > Why does printing the page pointer make any sense at all?  Surely the
> > PFN makes much more sense.
> 
> I suppose one could correlate a particular hashed pointer with other
> debug output, see "ah, that's the same page".  In which case one
> doesn't really care whether or not the address is hashed - it's just a
> cookie.  This sounds thin.
> 
> I doubt if a lot of thought went into the printk.  If the page pointer
> isn't useful then how about we simply remove it from the message?

If something about it weren't useful, I don't think we'd be seeing a patch
to transform it from a hashed pointer to an unhashed pointer?  I'm pretty
sure the PFN is the real information that's wanted here, since you can
look at the hardware RAM layout to determine why that's not eligible.
diff mbox series

Patch

diff --git a/mm/cma.c b/mm/cma.c
index 4a978e0..dfe9813 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -435,7 +435,7 @@  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,
+	pr_debug("%s(cma %px, count %lu, align %d)\n", __func__, (void *)cma,
 		 count, align);
 
 	if (!count)
@@ -534,7 +534,7 @@  bool cma_pages_valid(struct cma *cma, const struct page *pages,
 	pfn = page_to_pfn(pages);
 
 	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count) {
-		pr_debug("%s(page %p, count %lu)\n", __func__,
+		pr_debug("%s(page %px, count %lu)\n", __func__,
 						(void *)pages, count);
 		return false;
 	}
@@ -560,7 +560,7 @@  bool cma_release(struct cma *cma, const struct page *pages,
 	if (!cma_pages_valid(cma, pages, count))
 		return false;
 
-	pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
+	pr_debug("%s(page %px, count %lu)\n", __func__, (void *)pages, count);
 
 	pfn = page_to_pfn(pages);