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

[Liam Howlett]

* Peter Zijlstra <peterz@xxxxxxxxxxxxx> [210514 09:42]:
> On Wed, Apr 28, 2021 at 03:36:02PM +0000, Liam Howlett wrote:
> > +/*
> > + * mte_set_parent() - Set the parent node and encode the slot.
> > + * @enode: The encoded maple node.
> > + * @parent: The encoded maple node that is the parent of @enode.
> > + * @slot: The slot that @enode resides in @parent.
> > + *
> > + * Type is encoded in the enode->parent
> > + * bit 0: 1 = root, 0 otherwise
> > + * bit 1: Reserved.
> > + * bit 2: 0 = range 32, 1 = [a]range 64
> > + *
> > + * Slot number is encoded in the enode->parent
> > + * range_32, slot number is encoded in bits 3-6
> > + * [a]range_64, slot number is encoded in bits 3-6
> > + */
> > +static inline void mte_set_parent(struct maple_enode *enode,
> > +				 const struct maple_enode *parent,
> > +				 unsigned char slot)
> > +{
> > +	unsigned long bitmask = 0x78;
> > +	unsigned long val = (unsigned long) parent;
> > +	unsigned long type = 0;
> > +
> > +	switch (mte_node_type(parent)) {
> > +	case maple_range_64:
> > +	case maple_arange_64:
> > +		type = 6;
> 
> 6 = 4 + 2, which has bit1 set, but the above sayeth bit1 is reserved.

Yeah, that's not good.

> 
> It is also mighty confusing to have two different type fields, is there
> no way we can merge the types into a single (shared) space?
> 
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	val &= ~bitmask; // Remove any old slot number.
> > +	val |= (slot << MAPLE_PARENT_SHIFT); // Set the slot.
> > +	val |= type;
> > +	mte_to_node(enode)->parent = ma_parent_ptr(val);
> > +}
> 


Sorry, this is more fallout from removing types for the initial merge..
I will re-examine the mte_set_paret/mte_parent_slot stuff and fix the
comments (note range 32 above is not even in this code base either).

The type space maple_range_64/maple_arange_64 cannot be merged into a
single type as they have different slot and pivot counts.  The type of
node it is depends on the immutable tree flag for alloc on creation
time.  Since both node types cannot exist in the same type of tree,
reusing the same value here allows for more node types to exist..
Merging them would cause multiple checks of the tree flag during a walk
and function pointers are not as fast as they used to be.

I can just set type to range/arange right now as it's the only option.

Thanks for highlighting this as confusing.  I'll rework this to be
easier to understand.