Thread (137 messages) 137 messages, 6 authors, 2021-05-31

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

From: Peter Zijlstra <peterz@infradead.org>
Date: 2021-05-14 11:22:13
Also in: lkml

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help