| Message ID | 20230522110849.2921-3-urezki@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Mitigate a vmap lock contention | expand |
On Mon, May 22, 2023 at 01:08:42PM +0200, Uladzislau Rezki (Sony) wrote: > static __always_inline int > -adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > - struct vmap_area *va, unsigned long nva_start_addr, > - unsigned long size) > +va_clip(struct rb_root *root, struct list_head *head, > + struct vmap_area *va, unsigned long nva_start_addr, > + unsigned long size) Same comment as before, not going to repeat it for futher reviews. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, May 22, 2023 at 11:06:31PM -0700, Christoph Hellwig wrote: > On Mon, May 22, 2023 at 01:08:42PM +0200, Uladzislau Rezki (Sony) wrote: > > static __always_inline int > > -adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > > - struct vmap_area *va, unsigned long nva_start_addr, > > - unsigned long size) > > +va_clip(struct rb_root *root, struct list_head *head, > > + struct vmap_area *va, unsigned long nva_start_addr, > > + unsigned long size) > > Same comment as before, not going to repeat it for futher reviews. > Fixed! > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Thanks! -- Uladzislau Rezki
* Uladzislau Rezki (Sony) <urezki@gmail.com> [230522 07:09]: > This patch renames the adjust_va_to_fit_type() function > to a shorter variant and more expressive. A new name is > va_clip(). > > There is no a functional change as a result of this patch. > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > mm/vmalloc.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 409285b68a67..5f900efec6a9 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -1383,9 +1383,9 @@ classify_va_fit_type(struct vmap_area *va, > } > > static __always_inline int > -adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > - struct vmap_area *va, unsigned long nva_start_addr, > - unsigned long size) > +va_clip(struct rb_root *root, struct list_head *head, > + struct vmap_area *va, unsigned long nva_start_addr, > + unsigned long size) > { > struct vmap_area *lva = NULL; > enum fit_type type = classify_va_fit_type(va, nva_start_addr, size); > @@ -1501,7 +1501,7 @@ va_alloc(struct vmap_area *va, > return vend; > > /* Update the free vmap_area. */ > - ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); > + ret = va_clip(root, head, va, nva_start_addr, size); > if (WARN_ON_ONCE(ret)) ret is only used in this WARN_ON_ONCE() check (from patch 1), since you have shortened adjust_va_to_fit_type(), you can probably drop the ret variable all together here? > return vend; > > @@ -3979,9 +3979,8 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets, > /* It is a BUG(), but trigger recovery instead. */ > goto recovery; > > - ret = adjust_va_to_fit_type(&free_vmap_area_root, > - &free_vmap_area_list, > - va, start, size); > + ret = va_clip(&free_vmap_area_root, > + &free_vmap_area_list, va, start, size); > if (WARN_ON_ONCE(unlikely(ret))) > /* It is a BUG(), but trigger recovery instead. */ > goto recovery; > -- > 2.30.2 >
On Tue, May 23, 2023 at 01:24:09PM -0400, Liam R. Howlett wrote: > * Uladzislau Rezki (Sony) <urezki@gmail.com> [230522 07:09]: > > This patch renames the adjust_va_to_fit_type() function > > to a shorter variant and more expressive. A new name is > > va_clip(). > > > > There is no a functional change as a result of this patch. > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > --- > > mm/vmalloc.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index 409285b68a67..5f900efec6a9 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -1383,9 +1383,9 @@ classify_va_fit_type(struct vmap_area *va, > > } > > > > static __always_inline int > > -adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > > - struct vmap_area *va, unsigned long nva_start_addr, > > - unsigned long size) > > +va_clip(struct rb_root *root, struct list_head *head, > > + struct vmap_area *va, unsigned long nva_start_addr, > > + unsigned long size) > > { > > struct vmap_area *lva = NULL; > > enum fit_type type = classify_va_fit_type(va, nva_start_addr, size); > > @@ -1501,7 +1501,7 @@ va_alloc(struct vmap_area *va, > > return vend; > > > > /* Update the free vmap_area. */ > > - ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); > > + ret = va_clip(root, head, va, nva_start_addr, size); > > if (WARN_ON_ONCE(ret)) > > ret is only used in this WARN_ON_ONCE() check (from patch 1), since you > have shortened adjust_va_to_fit_type(), you can probably drop the ret > variable all together here? > I can move the WARN_ON_ONCE() check into the va_clip(). So it makes sense. As for a return value, we should check it, at least here: ret = va_clip(&free_vmap_area_root, &free_vmap_area_list, va, start, size); if (WARN_ON_ONCE(unlikely(ret))) /* It is a BUG(), but trigger recovery instead. */ goto recovery; i t can fail, though it rather goes toward almost zero%. Thank you for looking at it, Liam! -- Uladzuslau Rezki
On Mon, May 22, 2023 at 01:08:42PM +0200, Uladzislau Rezki (Sony) wrote: > This patch renames the adjust_va_to_fit_type() function > to a shorter variant and more expressive. A new name is > va_clip(). > > There is no a functional change as a result of this patch. Small nit - I think:- 'This patch renames the adjust_va_to_fit_type() function to va_clip() which is shorter and more expressive.' Reads better here. > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > mm/vmalloc.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 409285b68a67..5f900efec6a9 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -1383,9 +1383,9 @@ classify_va_fit_type(struct vmap_area *va, > } > > static __always_inline int > -adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, > - struct vmap_area *va, unsigned long nva_start_addr, > - unsigned long size) > +va_clip(struct rb_root *root, struct list_head *head, > + struct vmap_area *va, unsigned long nva_start_addr, > + unsigned long size) > { > struct vmap_area *lva = NULL; > enum fit_type type = classify_va_fit_type(va, nva_start_addr, size); > @@ -1501,7 +1501,7 @@ va_alloc(struct vmap_area *va, > return vend; > > /* Update the free vmap_area. */ > - ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); > + ret = va_clip(root, head, va, nva_start_addr, size); > if (WARN_ON_ONCE(ret)) > return vend; > > @@ -3979,9 +3979,8 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets, > /* It is a BUG(), but trigger recovery instead. */ > goto recovery; > > - ret = adjust_va_to_fit_type(&free_vmap_area_root, > - &free_vmap_area_list, > - va, start, size); > + ret = va_clip(&free_vmap_area_root, > + &free_vmap_area_list, va, start, size); > if (WARN_ON_ONCE(unlikely(ret))) > /* It is a BUG(), but trigger recovery instead. */ > goto recovery; > -- > 2.30.2 > Otherwise, Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>
On Sat, May 27, 2023 at 10:50:22PM +0100, Lorenzo Stoakes wrote: > On Mon, May 22, 2023 at 01:08:42PM +0200, Uladzislau Rezki (Sony) wrote: > > This patch renames the adjust_va_to_fit_type() function > > to a shorter variant and more expressive. A new name is > > va_clip(). > > > > There is no a functional change as a result of this patch. > > Small nit - I think:- > > 'This patch renames the adjust_va_to_fit_type() function to va_clip() which > is shorter and more expressive.' > > Reads better here. > I will update. Thanks! -- Uladzislau Rezki
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 409285b68a67..5f900efec6a9 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1383,9 +1383,9 @@ classify_va_fit_type(struct vmap_area *va, } static __always_inline int -adjust_va_to_fit_type(struct rb_root *root, struct list_head *head, - struct vmap_area *va, unsigned long nva_start_addr, - unsigned long size) +va_clip(struct rb_root *root, struct list_head *head, + struct vmap_area *va, unsigned long nva_start_addr, + unsigned long size) { struct vmap_area *lva = NULL; enum fit_type type = classify_va_fit_type(va, nva_start_addr, size); @@ -1501,7 +1501,7 @@ va_alloc(struct vmap_area *va, return vend; /* Update the free vmap_area. */ - ret = adjust_va_to_fit_type(root, head, va, nva_start_addr, size); + ret = va_clip(root, head, va, nva_start_addr, size); if (WARN_ON_ONCE(ret)) return vend; @@ -3979,9 +3979,8 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets, /* It is a BUG(), but trigger recovery instead. */ goto recovery; - ret = adjust_va_to_fit_type(&free_vmap_area_root, - &free_vmap_area_list, - va, start, size); + ret = va_clip(&free_vmap_area_root, + &free_vmap_area_list, va, start, size); if (WARN_ON_ONCE(unlikely(ret))) /* It is a BUG(), but trigger recovery instead. */ goto recovery;
This patch renames the adjust_va_to_fit_type() function to a shorter variant and more expressive. A new name is va_clip(). There is no a functional change as a result of this patch. Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> --- mm/vmalloc.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)