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 20:46:34 +0000
From:   Liam Howlett <liam.howlett@...cle.com>
To:     Peter Zijlstra <peterz@...radead.org>
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

* Peter Zijlstra <peterz@...radead.org> [210514 07:21]:
> 
> 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.
> 


You are correct, this is a mess..  I will re-examine all comments and
convert to C style and ensure a blank line start.