[PATCH] dma-contiguous: define proper name for global cma region

[PATCH] dma-contiguous: define proper name for global cma region

[Pintu Kumar]

The current global cma region name defined as "reserved"
which is misleading, creates confusion and too generic.

Also, the default cma allocation happens from global cma region,
so, if one has to figure out all allocations happening from
global cma region, this seems easier.

Thus, change the name from "reserved" to "global-cma-region".

Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com>
Signed-off-by: Pintu Agarwal <pintu.ping@gmail.com>
---
 kernel/dma/contiguous.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index 26a8e53..4628b62 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -237,7 +237,7 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
 	int ret;
 
 	ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed,
-					"reserved", res_cma);
+					"global-cma-region", res_cma);
 	if (ret)
 		return ret;
 
-- 
2.7.4

[PATCH v2] dma-contiguous: define proper name for global cma region

[Pintu Kumar]

The current global cma region name defined as "reserved"
which is misleading, creates confusion and too generic.

Also, the default cma allocation happens from global cma region,
so, if one has to figure out all allocations happening from
global cma region, this seems easier.

Thus, change the name from "reserved" to "global-cma-region".

Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com>
---
 kernel/dma/contiguous.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index 26a8e53..4628b62 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -237,7 +237,7 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
 	int ret;
 
 	ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed,
-					"reserved", res_cma);
+					"global-cma-region", res_cma);
 	if (ret)
 		return ret;
 
-- 
2.7.4

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[Christoph Hellwig]

Hi Pintu,

On Sat, Jul 29, 2023 at 08:05:15AM +0530, Pintu Kumar wrote:
> The current global cma region name defined as "reserved"
> which is misleading, creates confusion and too generic.
> 
> Also, the default cma allocation happens from global cma region,
> so, if one has to figure out all allocations happening from
> global cma region, this seems easier.
> 
> Thus, change the name from "reserved" to "global-cma-region".

I agree that reserved is not a very useful name.  Unfortuately the
name of the region leaks to userspace through cma_heap.

So I think we need prep patches to hardcode "reserved" in
add_default_cma_heap first, and then remove the cma_get_name
first.

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[Pintu Agarwal]

Hi Christoph,
Thank you so much for your review and comments.

On Mon, 31 Jul 2023 at 16:51, Christoph Hellwig <hch@lst.de> wrote:
>
> Hi Pintu,
>
> On Sat, Jul 29, 2023 at 08:05:15AM +0530, Pintu Kumar wrote:
> > The current global cma region name defined as "reserved"
> > which is misleading, creates confusion and too generic.
> >
> > Also, the default cma allocation happens from global cma region,
> > so, if one has to figure out all allocations happening from
> > global cma region, this seems easier.
> >
> > Thus, change the name from "reserved" to "global-cma-region".
>
> I agree that reserved is not a very useful name.  Unfortuately the
> name of the region leaks to userspace through cma_heap.
>
> So I think we need prep patches to hardcode "reserved" in
> add_default_cma_heap first, and then remove the cma_get_name
> first.

Sorry, but I could not fully understand your comments.
Can you please elaborate a little more what changes are required in
cma_heap if we change "reserved" to "global-cma-region" ?
You mean to say there are userspace tools that rely on this "reserved"
naming for global cma ?

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[Christoph Hellwig]

On Tue, Aug 01, 2023 at 10:42:42PM +0530, Pintu Agarwal wrote:
> > I agree that reserved is not a very useful name.  Unfortuately the
> > name of the region leaks to userspace through cma_heap.
> >
> > So I think we need prep patches to hardcode "reserved" in
> > add_default_cma_heap first, and then remove the cma_get_name
> > first.
> 
> Sorry, but I could not fully understand your comments.
> Can you please elaborate a little more what changes are required in
> cma_heap if we change "reserved" to "global-cma-region" ?

Step 1:

Instead of setting exp_info.name to cma_get_name(cma);
in __add_cma_heap just set it to "reserved", probably by passing a name
argument.  You can also remove the unused data argument to __add_cma_heap
and/or just fold that function into the only caller while you're at it.

Step 2:

Remove cma_get_name, as it is unused now.

Step 3:

The patch your previously sent.

> You mean to say there are userspace tools that rely on this "reserved"
> naming for global cma ?

Yes.

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[John Stultz]

On Tue, Aug 1, 2023 at 10:18 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Aug 01, 2023 at 10:42:42PM +0530, Pintu Agarwal wrote:
> > > I agree that reserved is not a very useful name.  Unfortuately the
> > > name of the region leaks to userspace through cma_heap.
> > >
> > > So I think we need prep patches to hardcode "reserved" in
> > > add_default_cma_heap first, and then remove the cma_get_name
> > > first.
> >
> > Sorry, but I could not fully understand your comments.
> > Can you please elaborate a little more what changes are required in
> > cma_heap if we change "reserved" to "global-cma-region" ?
>
> Step 1:
>
> Instead of setting exp_info.name to cma_get_name(cma);
> in __add_cma_heap just set it to "reserved", probably by passing a name
> argument.  You can also remove the unused data argument to __add_cma_heap
> and/or just fold that function into the only caller while you're at it.

So, forgive me, I've not had a chance to look into this, but my
recollection was "reserved" is the name we see on x86, but other names
are possibly provided via the dts node?

I believe on the hikey board its "linux,cma" is the name, so forcing
it to reserved would break that.

Maybe instead add a compat config option to force the cma name (so x86
can set it to "default" if needed)?

thanks
-john

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[Christoph Hellwig]

On Tue, Aug 01, 2023 at 10:39:04PM -0700, John Stultz wrote:
> So, forgive me, I've not had a chance to look into this, but my
> recollection was "reserved" is the name we see on x86, but other names
> are possibly provided via the dts node?

Indeed, dma_contiguous_default_area can also be set through
rmem_cma_setup, which then takes the name from DT.

> I believe on the hikey board its "linux,cma" is the name, so forcing
> it to reserved would break that.
> 
> Maybe instead add a compat config option to force the cma name (so x86
> can set it to "default" if needed)?


I think we'll just need to leave it as-is.  I with dma-heaps had never
exposed the name to userspace, but we'll have to lіve with it now.

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[Pintu Agarwal]

Hi,

On Wed, 2 Aug 2023 at 15:17, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Aug 01, 2023 at 10:39:04PM -0700, John Stultz wrote:
> > So, forgive me, I've not had a chance to look into this, but my
> > recollection was "reserved" is the name we see on x86, but other names
> > are possibly provided via the dts node?
>
No, I think "reserved" is the name hard-coded (for all arch) in Kernel
for global-cma.
So, I don't think this is x86 specific. I am checking on arm32 itself.
When we can dma_alloc_coherent we see these in the logs (if dts region
is not present).
cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
Now, with this change we will see this:
cma: cma_alloc(cma (ptrval), name: global-cma-region, count 64, align 6)

> Indeed, dma_contiguous_default_area can also be set through
> rmem_cma_setup, which then takes the name from DT.
>
I think this is a different case. If DT entry is present we get this:
Reserved memory: created CMA memory pool at 0x98000000, name: name:
linux,cma, size 128 MiB
cma: cma_alloc(cma (ptrval), name: linux,cma, count 64, align 6)

Here we are talking about the default hard-coded name in Kernel code
if DT is not defined.
So, in one of the boards, this DT entry was not present and it shows
as "reserved".

> > I believe on the hikey board its "linux,cma" is the name, so forcing
> > it to reserved would break that.
> >
Yes, everywhere in the DT it's defined as "linux,cma".
You mean this also should be changed to "linux,cma-global-region"
everywhere with this change ?

> > Maybe instead add a compat config option to force the cma name (so x86
> > can set it to "default" if needed)?
>
Yes, having it in config is also a good option instead of hard-coding in Kernel.
>
> I think we'll just need to leave it as-is.  I with dma-heaps had never
> exposed the name to userspace, but we'll have to lіve with it now.

Can you point me to the userspace utility we are talking about here ?
I think we should not worry much about userspace name exposure.
I guess it should fetch whatever is declared in Kernel or DTS, right ?

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[Pintu Agarwal]

Hi,

On Thu, 3 Aug 2023 at 23:04, Pintu Agarwal <pintu.ping@gmail.com> wrote:
>
> Hi,
>
> On Wed, 2 Aug 2023 at 15:17, Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Aug 01, 2023 at 10:39:04PM -0700, John Stultz wrote:
> > > So, forgive me, I've not had a chance to look into this, but my
> > > recollection was "reserved" is the name we see on x86, but other names
> > > are possibly provided via the dts node?
> >
> No, I think "reserved" is the name hard-coded (for all arch) in Kernel
> for global-cma.
> So, I don't think this is x86 specific. I am checking on arm32 itself.
> When we can dma_alloc_coherent we see these in the logs (if dts region
> is not present).
> cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
> Now, with this change we will see this:
> cma: cma_alloc(cma (ptrval), name: global-cma-region, count 64, align 6)
>
> > Indeed, dma_contiguous_default_area can also be set through
> > rmem_cma_setup, which then takes the name from DT.
> >
> I think this is a different case. If DT entry is present we get this:
> Reserved memory: created CMA memory pool at 0x98000000, name: name:
> linux,cma, size 128 MiB
> cma: cma_alloc(cma (ptrval), name: linux,cma, count 64, align 6)
>
> Here we are talking about the default hard-coded name in Kernel code
> if DT is not defined.
> So, in one of the boards, this DT entry was not present and it shows
> as "reserved".
>
> > > I believe on the hikey board its "linux,cma" is the name, so forcing
> > > it to reserved would break that.
> > >
> Yes, everywhere in the DT it's defined as "linux,cma".
> You mean this also should be changed to "linux,cma-global-region"
> everywhere with this change ?
>
> > > Maybe instead add a compat config option to force the cma name (so x86
> > > can set it to "default" if needed)?
> >
> Yes, having it in config is also a good option instead of hard-coding in Kernel.
> >
> > I think we'll just need to leave it as-is.  I with dma-heaps had never
> > exposed the name to userspace, but we'll have to lіve with it now.
>
> Can you point me to the userspace utility we are talking about here ?
> I think we should not worry much about userspace name exposure.
> I guess it should fetch whatever is declared in Kernel or DTS, right ?

Just to follow-up on this.
For now, can we change the Kernel hard-coded value from "reserved" to
"global-cma-region" ?
Later, for the DTS defined name let it be "linux,cma" or change that
also to "linux,global-cma-region" ?

Will this make sense ?

Re: [PATCH v2] dma-contiguous: define proper name for global cma region

[John Stultz]

On Wed, Aug 9, 2023 at 8:04 AM Pintu Agarwal <pintu.ping@gmail.com> wrote:
>
> Hi,
>
> On Thu, 3 Aug 2023 at 23:04, Pintu Agarwal <pintu.ping@gmail.com> wrote:
> >
> > Hi,
> >
> > On Wed, 2 Aug 2023 at 15:17, Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > On Tue, Aug 01, 2023 at 10:39:04PM -0700, John Stultz wrote:
> > > > So, forgive me, I've not had a chance to look into this, but my
> > > > recollection was "reserved" is the name we see on x86, but other names
> > > > are possibly provided via the dts node?
> > >
> > No, I think "reserved" is the name hard-coded (for all arch) in Kernel
> > for global-cma.
> > So, I don't think this is x86 specific. I am checking on arm32 itself.
> > When we can dma_alloc_coherent we see these in the logs (if dts region
> > is not present).
> > cma: cma_alloc(cma (ptrval), name: reserved, count 64, align 6)
> > Now, with this change we will see this:
> > cma: cma_alloc(cma (ptrval), name: global-cma-region, count 64, align 6)
> >
> > > Indeed, dma_contiguous_default_area can also be set through
> > > rmem_cma_setup, which then takes the name from DT.
> > >
> > I think this is a different case. If DT entry is present we get this:
> > Reserved memory: created CMA memory pool at 0x98000000, name: name:
> > linux,cma, size 128 MiB
> > cma: cma_alloc(cma (ptrval), name: linux,cma, count 64, align 6)
> >
> > Here we are talking about the default hard-coded name in Kernel code
> > if DT is not defined.
> > So, in one of the boards, this DT entry was not present and it shows
> > as "reserved".
> >
> > > > I believe on the hikey board its "linux,cma" is the name, so forcing
> > > > it to reserved would break that.
> > > >
> > Yes, everywhere in the DT it's defined as "linux,cma".
> > You mean this also should be changed to "linux,cma-global-region"
> > everywhere with this change ?
> >
> > > > Maybe instead add a compat config option to force the cma name (so x86
> > > > can set it to "default" if needed)?
> > >
> > Yes, having it in config is also a good option instead of hard-coding in Kernel.
> > >
> > > I think we'll just need to leave it as-is.  I with dma-heaps had never
> > > exposed the name to userspace, but we'll have to lіve with it now.
> >
> > Can you point me to the userspace utility we are talking about here ?
> > I think we should not worry much about userspace name exposure.
> > I guess it should fetch whatever is declared in Kernel or DTS, right ?
>
> Just to follow-up on this.
> For now, can we change the Kernel hard-coded value from "reserved" to
> "global-cma-region" ?
> Later, for the DTS defined name let it be "linux,cma" or change that
> also to "linux,global-cma-region" ?
>
> Will this make sense ?

Apologies, sorry for not responding to your earlier message, it slipped by.

So, the concern is there may be allocators (like gralloc in Android)
that allocate from the CMA region via the dma-buf heaps interface.

So by changing the name (either hardcoded or DTS names), you'll change
the user-visible heap name, potentially breaking those userland
allocators.

Now, the dmabuf heaps are designed to be like other dynamic devices
(like disks or partitions), which may be different from device to
device. However, changing the name would still be an inconvenience for
folks who have hard-coded that name in their userland allocator which
was designed for a single device.  This would be similar to the old
issue of an existing fstab breaking from the ide (hda) to sata (sda)
driver transition.  Or similar to what folks went through a while back
with network device names changing from eth0 -> ens0 or whatever.

That said, most android devices historically haven't upreved to new
kernel versions wihtout major userspace changes, so the impact might
be minimal, but that is likely to change in the future so we should be
careful here.

What I'd propose instead is to either leave it alone as Christoph
suggested, or have a build option/boot argument so folks can preserve
the legacy name if they need.

thanks
-john