Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 12 Dec 2023 20:57:48 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Sidhartha Kumar <sidhartha.kumar@oracle.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        maple-tree@lists.infradead.org, akpm@linux-foundation.org,
        liam.howlett@oracle.com, zhangpeng.00@bytedance.com
Subject: Re: [PATCH] maple_tree: do not preallocate nodes for slot stores
Message-ID: <ZXjJTM0pDksnslgo@casper.infradead.org>
References: <20231212194640.217966-1-sidhartha.kumar@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231212194640.217966-1-sidhartha.kumar@oracle.com>
Precedence: bulk

On Tue, Dec 12, 2023 at 11:46:40AM -0800, Sidhartha Kumar wrote:
> +	/* Slot store, does not require additional nodes */
> +	if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree))
> +		|| (wr_mas.offset_end - mas->offset == 1)))
> +		return 0;

Should we refactor this into a mas_is_slot_store() predicate?

A few coding-style problems with it as it's currently written:

1. The indentation on the second line is wrong.  It makes the
continuation of the condition look like part of the statement.  Use
extra whitespace to indent.  eg:

	if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree))
			|| (wr_mas.offset_end - mas->offset == 1)))
		return 0;

2. The operator goes last on the line, not at the beginning of the
continuation line.  ie:

	if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree)) ||
			(wr_mas.offset_end - mas->offset == 1)))
		return 0;

3. You don't need parens around the !mt_in_rcu(mas->tree).  There's
no ambiguity to solve here:

	if ((node_size == mas->end) && (!mt_in_rcu(mas->tree) ||
			(wr_mas.offset_end - mas->offset == 1)))
		return 0;

But I'd write it as:

	if ((node_size == mas->end) &&
	    (!mt_in_rcu(mas->tree) || (wr_mas.offset_end - mas->offset == 1)))
		return 0;

because then the whitespace matches how you're supposed to parse the
condition, and so the next person to read this code will have an easier
time of it.