Re: [RFC PATCH] cma: make number of CMA areas dynamic, remove CONFIG_CMA_AREAS

[Mike Kravetz]

On 9/3/20 6:58 PM, Song Bao Hua (Barry Song) wrote:
> 
>> -----Original Message-----
>> From: Mike Kravetz [mailto:mike.kravetz@xxxxxxxxxx]
>> Sent: Thursday, September 3, 2020 3:02 PM
>> To: linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-mips@xxxxxxxxxxxxxxx
>> Cc: Roman Gushchin <guro@xxxxxx>; Song Bao Hua (Barry Song)
>> <song.bao.hua@xxxxxxxxxxxxx>; Joonsoo Kim <js1304@xxxxxxxxx>; Rik van
>> Riel <riel@xxxxxxxxxxx>; Aslan Bakirov <aslan@xxxxxx>; Michal Hocko
>> <mhocko@xxxxxxxxxx>; Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>; Mike
>> Kravetz <mike.kravetz@xxxxxxxxxx>
>> Subject: [RFC PATCH] cma: make number of CMA areas dynamic, remove
>> CONFIG_CMA_AREAS
>>
>> The number of distinct CMA areas is limited by the constant
>> CONFIG_CMA_AREAS.  In most environments, this was set to a default
>> value of 7.  Not too long ago, support was added to allocate hugetlb
>> gigantic pages from CMA.  More recent changes to make dma_alloc_coherent
>> NUMA-aware on arm64 added more potential users of CMA areas.  Along
>> with the dma_alloc_coherent changes, the default value of CMA_AREAS
>> was bumped up to 19 if NUMA is enabled.
>>
>> It seems that the number of CMA users is likely to grow.  Instead of
>> using a static array for cma areas, use a simple linked list.  These
>> areas are used before normal memory allocators, so use the memblock
>> allocator.
> 
> Hello Mike, It seems it is a good idea. Thanks for addressing this.
> 
> I was focusing on per-numa cma feature in my patchset and I didn't take care of this
> while I thought we should do something for the number of cma areas.
> 

Thanks for taking a look.

One area where I could use some help is testing/verifying on arm.  See the
changes to arch/arm/mm/dma-mapping.c.  I have tested the generic changes on
my x86 platform, but do not have an arm platform for easy testing.

>>  void __init dma_contiguous_early_fixup(phys_addr_t base, unsigned long
>> size)
>>  {
>> -	dma_mmu_remap[dma_mmu_remap_num].base = base;
>> -	dma_mmu_remap[dma_mmu_remap_num].size = size;
>> -	dma_mmu_remap_num++;
>> +	struct dma_contig_early_reserve *d;
>> +
>> +	d = memblock_alloc(sizeof(struct dma_contig_early_reserve),
> 
> sizeof(*d)?

Yes.  thanks.

>> @@ -172,15 +173,14 @@ int __init cma_init_reserved_mem(phys_addr_t
>> base, phys_addr_t size,
>>  	struct cma *cma;
>>  	phys_addr_t alignment;
>>
>> -	/* Sanity checks */
>> -	if (cma_area_count == ARRAY_SIZE(cma_areas)) {
>> -		pr_err("Not enough slots for CMA reserved regions!\n");
>> -		return -ENOSPC;
>> -	}
>> +	/* Do not attempt allocations after memblock allocator is torn down */
>> +	if (slab_is_available())
>> +		return -EINVAL;
>>
>>  	if (!size || !memblock_is_region_reserved(base, size))
>>  		return -EINVAL;
>>
>> +
> 
> Is this empty line relevant?

No, added by mistake.

>> @@ -192,12 +192,17 @@ int __init cma_init_reserved_mem(phys_addr_t
>> base, phys_addr_t size,
>>  	if (ALIGN(base, alignment) != base || ALIGN(size, alignment) != size)
>>  		return -EINVAL;
>>
>> +	cma = memblock_alloc(sizeof(struct cma), sizeof(long));
> 
> sizeof(*cma)?

Yes, thanks.

> It seems we are going to write cma-> count, order_per_bit, debugfs fields.
> To avoid false sharing of the cacheline of struct cma, it is better to align with
> SMP_CACHE_BYTES.
> 
> On the other hand, it seems we are unlikely to write the cma 

I thought about using SMP_CACHE_BYTES, but the structures are simply defined
as an array today.  This should not be any worse.  I do not believe access
to the structures is performance sensitive.

Thanks,
-- 
Mike Kravetz