Re: [RFC PATCH v2 1/1] mm/vmap: keep track of free blocks for vmap allocation

[Roman Gushchin <guro@fb.com>]

On Tue, Mar 26, 2019 at 03:51:53PM +0100, Uladzislau Rezki wrote:
> Hello, Roman.
> 
> > > 
> > > So, does it mean that this function always returns two following elements?
> > > Can't it return a single element using the return statement instead?
> > > The second one can be calculated as ->next?
> > > 
> > Yes, they follow each other and if you return "prev" for example you can easily
> > refer to next. But you will need to access "next" anyway. I would rather keep
> > implementation, because it strictly clear what it return when you look at this
> > function.
> > 
> > But if there are some objections and we can simplify, let's discuss :)
> > 
> > > > +		}
> > > > +	} else {
> > > > +		/*
> > > > +		 * The red-black tree where we try to find VA neighbors
> > > > +		 * before merging or inserting is empty, i.e. it means
> > > > +		 * there is no free vmap space. Normally it does not
> > > > +		 * happen but we handle this case anyway.
> > > > +		 */
> > > > +		*prev = *next = &free_vmap_area_list;
> > > 
> > > And for example, return NULL in this case.
> > > 
> > Then we will need to check in the __merge_or_add_vmap_area() that
> > next/prev are not NULL and not head. But i do not like current implementation
> > as well, since it is hardcoded to specific list head.
> > 
> Like you said, it is more clever to return only one element, for example next.
> After that just simply access to the previous one. If nothing is found return
> NULL.
> 
> static inline struct list_head *
> __get_va_next_sibling(struct rb_node *parent, struct rb_node **link)
> {
> 	struct list_head *list;
> 
> 	if (likely(parent)) {
> 		list = &rb_entry(parent, struct vmap_area, rb_node)->list;
> 		return (&parent->rb_right == link ? list->next:list);
> 	}
> 
> 	/*
> 	 * The red-black tree where we try to find VA neighbors
> 	 * before merging or inserting is empty, i.e. it means
> 	 * there is no free vmap space. Normally it does not
> 	 * happen but we handle this case anyway.
> 	 */
> 	return NULL;
> }
> ...
> static inline void
> __merge_or_add_vmap_area(struct vmap_area *va,
> 	struct rb_root *root, struct list_head *head)
> {
> ...
> 	/*
> 	 * Get next node of VA to check if merging can be done.
> 	 */
> 	next = __get_va_next_sibling(parent, link);
> 	if (unlikely(next == NULL))
> 		goto insert;
> ...
> }
> 
> Agree with your point and comment.

Hello, Uladzislau!

Yeah, the version above looks much simpler!
Looking forward for the next version of the patchset.

Thanks!