Re: [PATCH v3 1/2] mm: vmalloc: Remove a global vmap_blocks xarray

[Uladzislau Rezki]

On Wed, Mar 29, 2023 at 12:33:05PM +0800, Baoquan He wrote:
> On 03/28/23 at 02:34pm, Uladzislau Rezki wrote:
> ......  
> > > > @@ -2003,8 +2037,8 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask)
> > > >  	bitmap_set(vb->used_map, 0, (1UL << order));
> > > >  	INIT_LIST_HEAD(&vb->free_list);
> > > >  
> > > > -	vb_idx = addr_to_vb_idx(va->va_start);
> > > > -	err = xa_insert(&vmap_blocks, vb_idx, vb, gfp_mask);
> > > > +	vbq = addr_to_vbq(va->va_start);
> > > > +	err = xa_insert(&vbq->vmap_blocks, va->va_start, vb, gfp_mask);
> > > 
> > > Using va->va_start as index to access xarray may cost extra memory.
> > > Imagine we got a virtual address at VMALLOC_START, its region is
> > > [VMALLOC_START, VMALLOC_START+4095]. In the xarray, its sequence order
> > > is 0. While with va->va_start, it's 0xffffc90000000000UL on x86_64 with
> > > level4 paging mode. That means for the first page size vmalloc area,
> > > storing it into xarray need about 10 levels of xa_node, just for the one
> > > page size. With the old addr_to_vb_idx(), its index is 0. Only one level
> > > height is needed. One xa_node is about 72bytes, it could take more time
> > > and memory to access va->va_start. Not sure if my understanding is correct.
> > > 
> > > static unsigned long addr_to_vb_idx(unsigned long addr)
> > > {
> > >         addr -= VMALLOC_START & ~(VMAP_BLOCK_SIZE-1);
> > >         addr /= VMAP_BLOCK_SIZE;
> > >         return addr;
> > > }
> > > 
> > If the size of array depends on index "length", then, indeed it will require
> > more memory. From the other hand we can keep the old addr_to_vb_idx() function 
> > in order to "cut" a va->va_start index.
> 
> Yeah, the extra 10 levels of xa_node is unnecessary if we keep the old
> addr_to_vb_idx(). And the prolonged path will cost more time to reach the 
> wanted leaf node. E.g on x86_64 with 4 level paging mode, vmalloc area
> is 32TB. With the old calculation, its index range is [0, 8M], 4 level
> heights of xa_node at most is enough to cover.
> 
Good! I have not analyzed how xarray stores its indexes. I will update
the patch to cut indexes so we stay the same as we used to be before.

--
Uladzislau Rezki