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.