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!
