On 03/30/24 at 01:55pm, Uladzislau Rezki wrote:
> On Fri, Mar 29, 2024 at 03:44:40PM +0800, Baoquan He wrote:
> > On 03/28/24 at 03:03pm, Uladzislau Rezki (Sony) wrote:
......snip
> > How about below change about va_start_lowest? Personal preference, not
> > strong opinion.
> >
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 9b1a41e12d70..bd6a66c54ad2 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -1046,19 +1046,19 @@ __find_vmap_area_exceed_addr(unsigned long addr, struct rb_root *root)
> > static struct vmap_node *
> > find_vmap_area_exceed_addr_lock(unsigned long addr, struct vmap_area **va)
> > {
> > - unsigned long va_start_lowest;
> > + unsigned long va_start_lowest = ULONG_MAX;
> > struct vmap_node *vn;
> > int i;
> >
> > repeat:
> > - for (i = 0, va_start_lowest = 0; i < nr_vmap_nodes; i++) {
> > + for (i = 0; i < nr_vmap_nodes; i++) {
> > vn = &vmap_nodes[i];
> >
> > spin_lock(&vn->busy.lock);
> > *va = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> >
> > if (*va)
> > - if (!va_start_lowest || (*va)->va_start < va_start_lowest)
> > + if ((*va)->va_start < va_start_lowest)
> > va_start_lowest = (*va)->va_start;
> > spin_unlock(&vn->busy.lock);
> > }
> > @@ -1069,7 +1069,7 @@ find_vmap_area_exceed_addr_lock(unsigned long addr, struct vmap_area **va)
> > * been removed concurrently thus we need to proceed
> > * with next one what is a rare case.
> > */
> > - if (va_start_lowest) {
> > + if (va_start_lowest != ULONG_MAX) {
> > vn = addr_to_node(va_start_lowest);
> >
> > spin_lock(&vn->busy.lock);
> >
> >
> To me it looks as incomplete. The "va_start_lowest" should be initialized
> when repeat. Otherwise we can end up with an infinite repeating because
> va_start_lowest != ULONG_MAX.
You are right. Anyway, it's just a suggestion from a different code
style, please feel free to adjust it in or leave the patch as is.
>
> > > }
> > >
> > > - return va_node;
> > > -}
> > > -
> > > -static struct vmap_area *__find_vmap_area(unsigned long addr, struct rb_root *root)
> > > -{
> > > - struct rb_node *n = root->rb_node;
> > > + /*
> > > + * Check if found VA exists, it might it is gone away.
> > ~~~~ grammer mistake?
> > > + * In this case we repeat the search because a VA has
> > > + * been removed concurrently thus we need to proceed
> > > + * with next one what is a rare case.
> > ~~~~ typo, which?
> > > + */
> > > + if (va_start_lowest) {
> > > + vn = addr_to_node(va_start_lowest);
> > >
> > > - addr = (unsigned long)kasan_reset_tag((void *)addr);
> > > + spin_lock(&vn->busy.lock);
> > > + *va = __find_vmap_area(va_start_lowest, &vn->busy.root);
> > >
> > > - while (n) {
> > > - struct vmap_area *va;
> > > + if (*va)
> > > + return vn;
> > >
> > > - va = rb_entry(n, struct vmap_area, rb_node);
> > > - if (addr < va->va_start)
> > > - n = n->rb_left;
> > > - else if (addr >= va->va_end)
> > > - n = n->rb_right;
> > > - else
> > > - return va;
> > > + spin_unlock(&vn->busy.lock);
> > > + goto repeat;
> > > }
> >
> > Other than above nickpick concerns, this looks good to me.
> >
> > Reviewed-by: Baoquan He <bhe@xxxxxxxxxx>
> >
> Thank you!
>
> --
> Uladzislau Rezki
>
