linux-kernel - Re: [PATCH 26/94] Maple Tree: Add new data structure

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]

Date:   Fri, 14 May 2021 13:20:18 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Liam Howlett <liam.howlett@...cle.com>
Cc:     "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Song Liu <songliubraving@...com>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        David Rientjes <rientjes@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Rik van Riel <riel@...riel.com>,
        Michel Lespinasse <walken.cr@...il.com>
Subject: Re: [PATCH 26/94] Maple Tree: Add new data structure


On Wed, Apr 28, 2021 at 03:36:02PM +0000, Liam Howlett wrote:
> +/* ma_free_rcu() - Use rcu callback to free a maple node
> + * @node: The node to free
> + *
> + * The maple tree uses the parent pointer to indicate this node is no longer in
> + * use and will be freed.
> + */

If this was supposed to be a kernel doc, then it would need to start
with /**, if it was not then it's an inconsistent comment style; by far
the majority of comments in this file have the regular:

 /*
  * multiline-
  *   comment
  */

style.

Like

> +/*
> + * We also reserve values with the bottom two bits set to '10' which are
> + * below 4096
> + */

> +/*
> + * mte_to_mat() - Convert a maple encoded node to a maple topiary node.
> + * @entry: The maple encoded node
> + *
> + * Return: a maple topiary pointer
> + */

And:

> +/*
> + * mas_mn() - Get the maple state node.
> + * @mas: The maple state
> + *
> + * Return: the maple node (not encoded - bare pointer).
> + */

But then you also have:

> +	// Removing the pivot overflow optimizes the loop below.
> +	// Check the first implied pivot.

> +	// Check end implied pivot which can only be a gap on the right most
> +	// node.

And:

> +		/* If the split is less than the max slot && the right side will
> +		 * still be sufficient, then increment the split on NULL.
> +		 */

> +		/* Avoid having a range less than the slot count unless it
> +		 * causes one node to be deficient.
> +		 * NOTE: mt_min_slots is 1 based, b_end and split are zero.
> +		 */


Single line comments are also an inconsistent mess:

> +	/* Avoid ending a node on a NULL entry */

> +	// Possible underflow of piv will wrap back to 0 before use.

> +	// Copy start data up to insert.

Even in a single function, you can't be consistent:

> +static inline void mast_topiary(struct maple_subtree_state *mast)
> +{
> +	unsigned char l_off, r_off, offset;
> +	unsigned long l_index,  range_min, range_max;
> +	struct maple_enode *child;
> +	void __rcu **slots;
> +	enum maple_type mt;
> +
> +	// The left node is consumed, so add to the free list.
> +	l_index = mast->orig_l->index;
> +	mast->orig_l->index = mast->orig_l->last;
> +	mt = mte_node_type(mast->orig_l->node);
> +	mas_node_walk(mast->orig_l, mt, &range_min, &range_max);
> +	mast->orig_l->index = l_index;
> +	l_off = mast->orig_l->offset;
> +	r_off = mast->orig_r->offset;
> +	if (mast->orig_l->node == mast->orig_r->node) {
> +		slots = ma_slots(mte_to_node(mast->orig_l->node), mt);
> +		for (offset = l_off + 1; offset < r_off; offset++)
> +			mat_add(mast->destroy, mas_slot_locked(mast->orig_l,
> +							slots, offset));
> +
> +		return;
> +	}
> +	/* mast->orig_r is different and consumed. */
> +	if (mte_is_leaf(mast->orig_r->node))
> +		return;
> +
> +	/* Now destroy l_off + 1 -> end and 0 -> r_off - 1 */
> +	offset = l_off + 1;
> +	slots = ma_slots(mte_to_node(mast->orig_l->node), mt);
> +	while (offset < mt_slots[mt]) {
> +		child = mas_slot_locked(mast->orig_l, slots, offset++);
> +		if (!child)
> +			break;
> +
> +		mat_add(mast->destroy, child);
> +	}
> +
> +	slots = ma_slots(mte_to_node(mast->orig_r->node),
> +			     mte_node_type(mast->orig_r->node));
> +	for (offset = 0; offset < r_off; offset++)
> +		mat_add(mast->destroy,
> +				mas_slot_locked(mast->orig_l, slots, offset));
> +}

This mixing of C and C++ style comments is a mess.