On Tue, Aug 22, 2023 at 10:27 PM Z qiang <qiang.zhang1211@xxxxxxxxx> wrote:
>
> >
> > On Thu, Aug 17, 2023 at 2:34 AM Zqiang <qiang.zhang1211@xxxxxxxxx> wrote:
> > >
> > > Currently, for double invoke call_rcu(), will dump rcu_head objects
> > > memory info, if the objects is not allocated from the slab allocator,
> > > the vmalloc_dump_obj() will be invoke and the vmap_area_lock spinlock
> > > need to be held, since the call_rcu() can be invoked in interrupt context,
> > > therefore, there is a possibility of spinlock deadlock scenarios.
[..]
> > > -
> > > - if (virt_addr_valid(object))
> > > + if (is_vmalloc_addr(object))
> > > + type = "vmalloc memory";
> > > + else if (virt_addr_valid(object))
> > > type = "non-slab/vmalloc memory";
> > > else if (object == NULL)
> > > type = "NULL pointer";
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 93cf99aba335..224af955bcb5 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -4271,20 +4271,6 @@ void pcpu_free_vm_areas(struct vm_struct **vms, int nr_vms)
> > > }
> > > #endif /* CONFIG_SMP */
> > >
> > > -#ifdef CONFIG_PRINTK
> > > -bool vmalloc_dump_obj(void *object)
> > > -{
> > > - struct vm_struct *vm;
> > > - void *objp = (void *)PAGE_ALIGN((unsigned long)object);
> > > -
> > > - vm = find_vm_area(objp);
> >
> > What about adding something like the following to vmalloc.c. Would it work?
> >
> > +static struct vmap_area *find_vmap_area_trylock(unsigned long addr)
> > +{
> > + struct vmap_area *va;
> > +
> > + if (!spin_trylock(&vmap_area_lock))
> > + return NULL;
> > + va = __find_vmap_area(addr);
> > + spin_unlock(&vmap_area_lock);
> > +
> > + return va;
> > +}
> >
> > and call it from vmalloc_dump_obj():
> >
> > vm = find_vm_area_trylock(objp);
> >
> > And then keep the rest of your patch as it is but add back the call to
> > vmalloc_dump_obj() in mem_dump_obj():
> >
> > if (vmalloc_dump_obj(object))
> > return;
> >
> > - if (virt_addr_valid(object))
> > + if (is_vmalloc_addr(object))
> > + type = "vmalloc memory";
> > + else if (virt_addr_valid(object))
> > type = "non-slab/vmalloc memory";
> >
> >
> > If that still throws "sleeping function called" type warnings, then
> > that is a preempt RT issue because spin_trylock() on conversion to a
> > sleeping lock should not be sleeping as it is uncontended.
> >
>
> Hello, Joel
>
> I did a test in the RT kernel with the following modifications, and
> didn't trigger
> sleeping function called warning.
>
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 93cf99aba335..e07b0e34249b 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -4272,12 +4272,24 @@ void pcpu_free_vm_areas(struct vm_struct
> **vms, int nr_vms)
> #endif /* CONFIG_SMP */
>
> #ifdef CONFIG_PRINTK
> +static struct vm_struct *find_vmap_area_trylock(unsigned long addr)
> +{
> + struct vmap_area *va;
> +
> + if (!spin_trylock(&vmap_area_lock))
> + return NULL;
> + va = __find_vmap_area(addr, &vmap_area_root);
> + spin_unlock(&vmap_area_lock);
> +
> + return va ? va->vm : NULL;
> +}
> +
> bool vmalloc_dump_obj(void *object)
> {
> struct vm_struct *vm;
> void *objp = (void *)PAGE_ALIGN((unsigned long)object);
>
> - vm = find_vm_area(objp);
> + vm = find_vmap_area_trylock((unsigned long)objp);
> if (!vm)
> return false;
> pr_cont(" %u-page vmalloc region starting at %#lx allocated at %pS\n",
>
>
> If double call_rcu() appears in different threads at the same time,
> It is possible to dump only one of the vmalloc information.
Great to hear and thanks for checking! So what I'll do is create 2
patches -- one for the vmalloc change, and one for the patch you
submitted to use the vmalloc change. For the second patch, I'll keep
your authorship for attribution. I'll also queue it on my staging
trees for further testing. Let me know if that does not sound okay.
Today is a lighter work day for me, but allow me 1-2 days for sending
this out for review. Thanks,
- Joel
