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

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190327004130.GA31035@tower.DHCP.thefacebook.com>
Date:   Wed, 27 Mar 2019 00:41:37 +0000
From:   Roman Gushchin <guro@...com>
To:     Uladzislau Rezki <urezki@...il.com>
CC:     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

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!