Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 21 Aug 2019 18:03:47 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Davidlohr Bueso <dave@stgolabs.net>
Cc:     mingo@kernel.org, tglx@linutronix.de, walken@google.com,
        akpm@linux-foundation.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, Davidlohr Bueso <dbueso@suse.de>
Subject: Re: [PATCH 1/3] x86,mm/pat: Use generic interval trees
Message-ID: <20190821160347.GB2332@hirez.programming.kicks-ass.net>
References: <20190813224620.31005-1-dave@stgolabs.net>
 <20190813224620.31005-2-dave@stgolabs.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190813224620.31005-2-dave@stgolabs.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 13, 2019 at 03:46:18PM -0700, Davidlohr Bueso wrote:

> diff --git a/arch/x86/mm/pat_rbtree.c b/arch/x86/mm/pat_rbtree.c
> index fa16036fa592..1be4d1856a9b 100644
> --- a/arch/x86/mm/pat_rbtree.c
> +++ b/arch/x86/mm/pat_rbtree.c

> @@ -34,68 +34,41 @@
>   * memtype_lock protects the rbtree.
>   */
>  
> +static struct rb_root_cached memtype_rbroot = RB_ROOT_CACHED;
> +
> +#define START(node) ((node)->start)
> +#define END(node)  ((node)->end)

Afaict these could actually be inlines, but I see other interval tree
users also use macros for them, albeit with END called LAST.

> +INTERVAL_TREE_DEFINE(struct memtype, rb, u64, subtree_max_end,
> +		     START, END, static, memtype_interval)
>  
>  static int is_node_overlap(struct memtype *node, u64 start, u64 end)
>  {
> +	/*
> +	 * Unlike generic interval trees, the memtype nodes are ]a, b[
> +	 * therefore we need to adjust the ranges accordingly. Missing
> +	 * an overlap can lead to incorrectly detecting conflicts,
> +	 * for example.
> +	 */
> +	if (node->start + 1 >= end || node->end - 1 <= start)
>  		return 0;
>  
>  	return 1;
>  }

I've not looked yet; but how horrible would it be to change that
weirdness?

> @@ -149,10 +116,8 @@ static int memtype_rb_check_conflict(struct rb_root *root,
>  	dprintk("Overlap at 0x%Lx-0x%Lx\n", match->start, match->end);
>  	found_type = match->type;
>  
> +        match = memtype_interval_iter_next(match, start, end);
> +	while (match) {

whilespace fail

>  		if (match->start >= end) /* Checked all possible matches */
>  			goto success;
>  

> @@ -176,43 +141,21 @@ static int memtype_rb_check_conflict(struct rb_root *root,
>  	return -EBUSY;
>  }
>  
>  int rbt_memtype_check_insert(struct memtype *new,
>  			     enum page_cache_mode *ret_type)
>  {
> +	int err;
>  
> +	err =  memtype_rb_check_conflict(&memtype_rbroot,
> +					 new->start, new->end,
> +					 new->type, ret_type);

whitespace fail

> +	if (err)
> +		return err;
>  
> +	if (ret_type)
> +		new->type = *ret_type;
>  
> +	memtype_interval_insert(new, &memtype_rbroot);
>  	return err;
>  }
>