lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 26 Mar 2019 15:51:53 +0100
From:   Uladzislau Rezki <urezki@...il.com>
To:     Uladzislau Rezki <urezki@...il.com>
Cc:     Roman Gushchin <guro@...com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.com>,
        Matthew Wilcox <willy@...radead.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joel Fernandes <joelaf@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...e.hu>, Tejun Heo <tj@...nel.org>
Subject: Re: [RFC PATCH v2 1/1] mm/vmap: keep track of free blocks for vmap
 allocation

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.

Thanks!

--
Vlad Rezki

Powered by blists - more mailing lists