| Message ID | 1690598115-26287-1-git-send-email-quic_pintu@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] dma-contiguous: define proper name for global cma region | expand |
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.
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 ?
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.
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
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.
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 ?
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 ?
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
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;
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(-)