| Message ID | 20181214180720.32040-3-guro@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vmalloc enhancements | expand |
On Fri, Dec 14, 2018 at 10:07:18AM -0800, Roman Gushchin wrote: > alloc_vmap_area() is allocating memory for the vmap_area, and > performing the actual lookup of the vm area and vmap_area > initialization. > > This prevents us from using a pre-allocated memory for the map_area > structure, which can be used in some cases to minimize the number > of required memory allocations. > > Let's keep the memory allocation part in alloc_vmap_area() and > separate everything else into init_vmap_area(). Oh good, I was considering doing something like this eventually. > +++ b/mm/vmalloc.c > @@ -395,16 +395,11 @@ static void purge_vmap_area_lazy(void); > > static BLOCKING_NOTIFIER_HEAD(vmap_notify_list); > > -/* > - * Allocate a region of KVA of the specified size and alignment, within the > - * vstart and vend. > - */ > -static struct vmap_area *alloc_vmap_area(unsigned long size, > - unsigned long align, > - unsigned long vstart, unsigned long vend, > - int node, gfp_t gfp_mask) > +static int init_vmap_area(struct vmap_area *va, unsigned long size, > + unsigned long align, unsigned long vstart, > + unsigned long vend, int node, gfp_t gfp_mask) > { > - struct vmap_area *va; > + Why the blank line? > struct rb_node *n; > unsigned long addr; > int purged = 0; > @@ -416,11 +411,6 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, > > might_sleep(); > > - va = kmalloc_node(sizeof(struct vmap_area), > - gfp_mask & GFP_RECLAIM_MASK, node); > - if (unlikely(!va)) > - return ERR_PTR(-ENOMEM); > - > /* > * Only scan the relevant parts containing pointers to other objects > * to avoid false negatives. > @@ -512,7 +502,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, > BUG_ON(va->va_start < vstart); > BUG_ON(va->va_end > vend); > > - return va; > + return 0; > > overflow: > spin_unlock(&vmap_area_lock); > @@ -534,10 +524,38 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, > if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) > pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n", > size); > - kfree(va); > - return ERR_PTR(-EBUSY); > + > + return -EBUSY; > +} > + > +/* > + * Allocate a region of KVA of the specified size and alignment, within the > + * vstart and vend. > + */ > +static struct vmap_area *alloc_vmap_area(unsigned long size, > + unsigned long align, > + unsigned long vstart, > + unsigned long vend, > + int node, gfp_t gfp_mask) > +{ > + struct vmap_area *va; > + int ret; > + > + va = kmalloc_node(sizeof(struct vmap_area), > + gfp_mask & GFP_RECLAIM_MASK, node); > + if (unlikely(!va)) > + return ERR_PTR(-ENOMEM); > + > + ret = init_vmap_area(va, size, align, vstart, vend, node, gfp_mask); > + if (ret) { > + kfree(va); > + return ERR_PTR(ret); > + } > + > + return va; > } > > + Another spurious blank line? With these two fixed, Reviewed-by: Matthew Wilcox <willy@infradead.org>
On Fri, 2018-12-14 at 10:13 -0800, Matthew Wilcox wrote: > On Fri, Dec 14, 2018 at 10:07:18AM -0800, Roman Gushchin wrote: > > +/* > > + * Allocate a region of KVA of the specified size and alignment, within the > > + * vstart and vend. > > + */ > > +static struct vmap_area *alloc_vmap_area(unsigned long size, > > + unsigned long align, > > + unsigned long vstart, > > + unsigned long vend, > > + int node, gfp_t gfp_mask) > > +{ > > + struct vmap_area *va; > > + int ret; > > + > > + va = kmalloc_node(sizeof(struct vmap_area), > > + gfp_mask & GFP_RECLAIM_MASK, node); > > + if (unlikely(!va)) > > + return ERR_PTR(-ENOMEM); > > + > > + ret = init_vmap_area(va, size, align, vstart, vend, node, gfp_mask); > > + if (ret) { > > + kfree(va); > > + return ERR_PTR(ret); > > + } > > + > > + return va; > > } > > > > + > > Another spurious blank line? I don't think so. I think it is the better style to separate the error return from the normal return. > With these two fixed, > Reviewed-by: Matthew Wilcox <willy@infradead.org> >
On Fri, Dec 14, 2018 at 11:40:45AM -0800, Joe Perches wrote: > On Fri, 2018-12-14 at 10:13 -0800, Matthew Wilcox wrote: > > On Fri, Dec 14, 2018 at 10:07:18AM -0800, Roman Gushchin wrote: > > > +/* > > > + * Allocate a region of KVA of the specified size and alignment, within the > > > + * vstart and vend. > > > + */ > > > +static struct vmap_area *alloc_vmap_area(unsigned long size, > > > + unsigned long align, > > > + unsigned long vstart, > > > + unsigned long vend, > > > + int node, gfp_t gfp_mask) > > > +{ > > > + struct vmap_area *va; > > > + int ret; > > > + > > > + va = kmalloc_node(sizeof(struct vmap_area), > > > + gfp_mask & GFP_RECLAIM_MASK, node); > > > + if (unlikely(!va)) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + ret = init_vmap_area(va, size, align, vstart, vend, node, gfp_mask); > > > + if (ret) { > > > + kfree(va); > > > + return ERR_PTR(ret); > > > + } > > > + > > > + return va; > > > } > > > > > > + > > > > Another spurious blank line? > > I don't think so. > > I think it is the better style to separate > the error return from the normal return. Umm ... this blank line changed the file from having one blank line after the function to having two blank lines after the function.
On Fri, Dec 14, 2018 at 11:45:00AM -0800, Matthew Wilcox wrote: > On Fri, Dec 14, 2018 at 11:40:45AM -0800, Joe Perches wrote: > > On Fri, 2018-12-14 at 10:13 -0800, Matthew Wilcox wrote: > > > On Fri, Dec 14, 2018 at 10:07:18AM -0800, Roman Gushchin wrote: > > > > +/* > > > > + * Allocate a region of KVA of the specified size and alignment, within the > > > > + * vstart and vend. > > > > + */ > > > > +static struct vmap_area *alloc_vmap_area(unsigned long size, > > > > + unsigned long align, > > > > + unsigned long vstart, > > > > + unsigned long vend, > > > > + int node, gfp_t gfp_mask) > > > > +{ > > > > + struct vmap_area *va; > > > > + int ret; > > > > + > > > > + va = kmalloc_node(sizeof(struct vmap_area), > > > > + gfp_mask & GFP_RECLAIM_MASK, node); > > > > + if (unlikely(!va)) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + ret = init_vmap_area(va, size, align, vstart, vend, node, gfp_mask); > > > > + if (ret) { > > > > + kfree(va); > > > > + return ERR_PTR(ret); > > > > + } > > > > + > > > > + return va; > > > > } > > > > > > > > + > > > > > > Another spurious blank line? > > > > I don't think so. > > > > I think it is the better style to separate > > the error return from the normal return. > > Umm ... this blank line changed the file from having one blank line > after the function to having two blank lines after the function. > Yes, it's an odd free line (here and above), will remove them in v2. Thanks!
On Fri, 2018-12-14 at 11:45 -0800, Matthew Wilcox wrote: > On Fri, Dec 14, 2018 at 11:40:45AM -0800, Joe Perches wrote: > > On Fri, 2018-12-14 at 10:13 -0800, Matthew Wilcox wrote: > > > On Fri, Dec 14, 2018 at 10:07:18AM -0800, Roman Gushchin wrote: > > > > +/* > > > > + * Allocate a region of KVA of the specified size and alignment, within the > > > > + * vstart and vend. > > > > + */ > > > > +static struct vmap_area *alloc_vmap_area(unsigned long size, > > > > + unsigned long align, > > > > + unsigned long vstart, > > > > + unsigned long vend, > > > > + int node, gfp_t gfp_mask) > > > > +{ > > > > + struct vmap_area *va; > > > > + int ret; > > > > + > > > > + va = kmalloc_node(sizeof(struct vmap_area), > > > > + gfp_mask & GFP_RECLAIM_MASK, node); > > > > + if (unlikely(!va)) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + ret = init_vmap_area(va, size, align, vstart, vend, node, gfp_mask); > > > > + if (ret) { > > > > + kfree(va); > > > > + return ERR_PTR(ret); > > > > + } > > > > + > > > > + return va; > > > > } > > > > > > > > + > > > > > > Another spurious blank line? [wrong spacing noticed] > Umm ... this blank line changed the file from having one blank line > after the function to having two blank lines after the function. right. thanks.
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 24a84d584609..24c8ab28254d 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -395,16 +395,11 @@ static void purge_vmap_area_lazy(void); static BLOCKING_NOTIFIER_HEAD(vmap_notify_list); -/* - * Allocate a region of KVA of the specified size and alignment, within the - * vstart and vend. - */ -static struct vmap_area *alloc_vmap_area(unsigned long size, - unsigned long align, - unsigned long vstart, unsigned long vend, - int node, gfp_t gfp_mask) +static int init_vmap_area(struct vmap_area *va, unsigned long size, + unsigned long align, unsigned long vstart, + unsigned long vend, int node, gfp_t gfp_mask) { - struct vmap_area *va; + struct rb_node *n; unsigned long addr; int purged = 0; @@ -416,11 +411,6 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, might_sleep(); - va = kmalloc_node(sizeof(struct vmap_area), - gfp_mask & GFP_RECLAIM_MASK, node); - if (unlikely(!va)) - return ERR_PTR(-ENOMEM); - /* * Only scan the relevant parts containing pointers to other objects * to avoid false negatives. @@ -512,7 +502,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, BUG_ON(va->va_start < vstart); BUG_ON(va->va_end > vend); - return va; + return 0; overflow: spin_unlock(&vmap_area_lock); @@ -534,10 +524,38 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n", size); - kfree(va); - return ERR_PTR(-EBUSY); + + return -EBUSY; +} + +/* + * Allocate a region of KVA of the specified size and alignment, within the + * vstart and vend. + */ +static struct vmap_area *alloc_vmap_area(unsigned long size, + unsigned long align, + unsigned long vstart, + unsigned long vend, + int node, gfp_t gfp_mask) +{ + struct vmap_area *va; + int ret; + + va = kmalloc_node(sizeof(struct vmap_area), + gfp_mask & GFP_RECLAIM_MASK, node); + if (unlikely(!va)) + return ERR_PTR(-ENOMEM); + + ret = init_vmap_area(va, size, align, vstart, vend, node, gfp_mask); + if (ret) { + kfree(va); + return ERR_PTR(ret); + } + + return va; } + int register_vmap_purge_notifier(struct notifier_block *nb) { return blocking_notifier_chain_register(&vmap_notify_list, nb);