On Tue, 25. Jun 11:30, Baoquan He wrote:
> On 06/24/24 at 02:16pm, Uladzislau Rezki wrote:
> > On Fri, Jun 21, 2024 at 10:02:50PM +0800, Baoquan He wrote:
> > > On 06/21/24 at 11:44am, Uladzislau Rezki wrote:
> > > > On Fri, Jun 21, 2024 at 03:07:16PM +0800, Baoquan He wrote:
> > > > > On 06/21/24 at 11:30am, Hailong Liu wrote:
> > > > > > On Thu, 20. Jun 14:02, Nick Bowler wrote:
> > > > > > > On 2024-06-20 02:19, Nick Bowler wrote:
> > > ......
> > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > > index be2dd281ea76..18e87cafbaf2 100644
> > > > > --- a/mm/vmalloc.c
> > > > > +++ b/mm/vmalloc.c
> > > > > @@ -2542,7 +2542,7 @@ static DEFINE_PER_CPU(struct vmap_block_queue, vmap_block_queue);
> > > > > static struct xarray *
> > > > > addr_to_vb_xa(unsigned long addr)
> > > > > {
> > > > > - int index = (addr / VMAP_BLOCK_SIZE) % num_possible_cpus();
> > > > > + int index = (addr / VMAP_BLOCK_SIZE) % nr_cpu_ids;
> > > > >
> > > > > return &per_cpu(vmap_block_queue, index).vmap_blocks;
> > > > > }
> > > > >
> > > > The problem i see is about not-initializing of the:
> > > > <snip>
> > > > for_each_possible_cpu(i) {
> > > > struct vmap_block_queue *vbq;
> > > > struct vfree_deferred *p;
> > > >
> > > > vbq = &per_cpu(vmap_block_queue, i);
> > > > spin_lock_init(&vbq->lock);
> > > > INIT_LIST_HEAD(&vbq->free);
> > > > p = &per_cpu(vfree_deferred, i);
> > > > init_llist_head(&p->list);
> > > > INIT_WORK(&p->wq, delayed_vfree_work);
> > > > xa_init(&vbq->vmap_blocks);
> > > > }
> > > > <snip>
> > > >
> > > > correctly or fully. It is my bad i did not think that CPUs in a possible mask
> > > > can be non sequential :-/
> > > >
> > > > nr_cpu_ids - is not the max possible CPU. For example, in Nick case,
> > > > when he has two CPUs, num_possible_cpus() and nr_cpu_ids are the same.
> > >
> > > I checked the generic version of setup_nr_cpu_ids(), from codes, they
> > > are different with my understanding.
> > >
> > > kernel/smp.c
> > > void __init setup_nr_cpu_ids(void)
> > > {
> > > set_nr_cpu_ids(find_last_bit(cpumask_bits(cpu_possible_mask), NR_CPUS) + 1);
> > > }
> > >
> > I see that it is not a weak function, so it is generic, thus the
> > behavior can not be overwritten, which is great. This does what we
> > need.
> >
> > Thank you for checking this you are right!
>
> Thanks for confirming this.
>
> >
> > Then it is just a matter of proper initialization of the hash:
> >
> > <snip>
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 5d3aa2dc88a8..1733946f7a12 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -5087,7 +5087,13 @@ void __init vmalloc_init(void)
> > */
> > vmap_area_cachep = KMEM_CACHE(vmap_area, SLAB_PANIC);
> >
> > - for_each_possible_cpu(i) {
> > + /*
> > + * We use "nr_cpu_ids" here because some architectures
> > + * may have "gaps" in cpu-possible-mask. It is OK for
> > + * per-cpu approaches but is not OK for cases where it
> > + * can be used as hashes also.
> > + */
> > + for (i = 0; i < nr_cpu_ids; i++) {
>
> I was wrong about earlier comments. Percpu variables are only available
> on possible CPUs. For those nonexistent possible CPUs of static percpu
> variable vmap_block_queue, there isn't memory allocated and mapped for
> them. So accessing into them will cause problem.
Thansk for pointing out. Indeed, I have very limited knowledge about
CPU-related :)
>
> In Nick's case, there are only CPU0, CPU2. If you access
> &per_cpu(vmap_block_queue, 1), problem occurs. So I think we may need to
> change to take other way for vbq. E.g:
> 1) Storing the vb in the nearest neighbouring vbq on possible CPU as
> below draft patch;
> 2) create an normal array to store vbq of size nr_cpu_ids, then we can
> store/fetch each vbq on non-possible CPU?
>
> The way 1) is simpler, the existing code can be adapted a little just as
> below.
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 633363997dec..59a8951cc6c0 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2542,7 +2542,10 @@ static DEFINE_PER_CPU(struct vmap_block_queue, vmap_block_queue);
> static struct xarray *
> addr_to_vb_xa(unsigned long addr)
> {
> - int index = (addr / VMAP_BLOCK_SIZE) % num_possible_cpus();
> + int index = (addr / VMAP_BLOCK_SIZE) % nr_cpu_ids;
> +
> + if (!cpu_possible(idex))
> + index = cpumask_next(index, cpu_possible_mask);
If cpumask is b1001, more addr would located in cpu3.
>
> return &per_cpu(vmap_block_queue, index).vmap_blocks;
> }
> @@ -2556,9 +2559,15 @@ addr_to_vb_xa(unsigned long addr)
>
> static unsigned long addr_to_vb_idx(unsigned long addr)
> {
> + int id = (addr / VMAP_BLOCK_SIZE) % nr_cpu_ids;
> + int id_dest = id;
> +
> + if (!cpu_possible(id))
> + id_dest = cpumask_next(id, cpu_possible_mask);
> +
> addr -= VMALLOC_START & ~(VMAP_BLOCK_SIZE-1);
> addr /= VMAP_BLOCK_SIZE;
> - return addr;
> + return addr + (id_dest - id);
> }
How about using cpumask_nth to get cpu id ? It looks simpler and more
straightforward, it may waste some cputimes :)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 11fe5ea208aa..e1e63ffb9c57 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1994,8 +1994,9 @@ static struct xarray *
addr_to_vb_xa(unsigned long addr)
{
int index = (addr / VMAP_BLOCK_SIZE) % num_possible_cpus();
+ int cpu = cpumask_nth(index, cpu_possible_mask);
- return &per_cpu(vmap_block_queue, index).vmap_blocks;
+ return &per_cpu(vmap_block_queue, cpu).vmap_blocks;
}
>
--
help you, help me,
Hailong.
