| Message ID | 1681788789-19679-1-git-send-email-zhaoyang.huang@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: fix printk format within cma | expand |
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 > >
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 > > > >
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 >> > >> >
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?
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 --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);