| Message ID | 1688668414-12350-1-git-send-email-quic_pintu@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm: cma: print cma name as well in cma_alloc debug | expand |
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.
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.
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.
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?
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)
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?
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.
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
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 ?
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;