On 06/25/24 at 05:33pm, Uladzislau Rezki wrote:
> On Tue, Jun 25, 2024 at 09:02:43PM +0800, Baoquan He wrote:
> > On 06/25/24 at 02:40pm, Uladzislau Rezki wrote:
> > > On Tue, Jun 25, 2024 at 07:40:21PM +0800, Baoquan He wrote:
> > > > On 06/25/24 at 12:32pm, Uladzislau Rezki wrote:
> > > > > On Tue, Jun 25, 2024 at 11:30:33AM +0800, 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.
> > > > > >
> > > > > > 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?
> > > > > >
> > > > > A correct way, i think, is to create a normal array. A quick fix can be
> > > > > to stick to a next 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);
> > > > > >
> > > > > cpumask_next() can return nr_cpu_ids if no next bits set.
> > > >
> > > > It won't. nr_cpu_ids is the largest index + 1, the hashed index will
> > > > be: 0 =< index <= (nr_cpu_ids - 1) e.g cpu_possible_mask is
> > > > b10001111, the nr_cpu_ids is 8, the largest bit is cpu7.
> > > > cpu_possible(index) will check that. So the largest bit of cpumask_next()
> > > > returns is (nr_cpu_ids - 1).
> > > >
> > > /**
> > > * cpumask_next - get the next cpu in a cpumask
> > > * @n: the cpu prior to the place to search (i.e. return will be > @n)
> > > * @srcp: the cpumask pointer
> > > *
> > > * Return: >= nr_cpu_ids if no further cpus set.
> >
> > Ah, I got what you mean. In the vbq case, it may not have chance to get
> > a return number as nr_cpu_ids. Becuase the hashed index limits the
> > range to [0, nr_cpu_ids-1], and cpu_possible(index) will guarantee it
> > won't be the highest cpu number [nr_cpu_ids-1] since CPU[nr_cpu_ids-1] must
> > be possible CPU.
> >
> > Do I miss some corner cases?
> >
> Right. We guarantee that a highest CPU is available by doing: % nr_cpu_ids.
> So we do not need to use *next_wrap() variant. You do not miss anything :)
>
> Hailong Liu has proposed more simpler version:
>
> <snip>
> 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;
> <snip>
>
> which just takes a next CPU if an index is not set in the cpu_possible_mask.
>
> The only thing that can be updated in the patch is to replace num_possible_cpu()
> by the nr_cpu_ids.
>
> Any thoughts? I think we need to fix it by a minor change so it is
> easier to back-port on stable kernels.
Yeah, sounds good since the regresson commit is merged in v6.3.
Please feel free to post this and the hash array patch separately for
formal reviewing.
By the way, when I am replying this mail, I check the cpumask_nth()
again. I doubt it may take more checking then cpu_possible(), given most
of systems don't have gaps in cpu_possible_mask. I could be dizzy at
this moment.
static inline unsigned int cpumask_nth(unsigned int cpu, const struct cpumask *srcp)
{
return find_nth_bit(cpumask_bits(srcp), small_cpumask_bits, cpumask_check(cpu));
}
