[PATCH] maple_tree: do not preallocate nodes for slot stores
Liam R. Howlett
Liam.Howlett at Oracle.com
Tue Dec 12 13:41:04 PST 2023
* Matthew Wilcox <willy at infradead.org> [231212 15:58]:
> On Tue, Dec 12, 2023 at 11:46:40AM -0800, Sidhartha Kumar wrote:
> > + /* Slot store, does not require additional nodes */
> > + if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree))
> > + || (wr_mas.offset_end - mas->offset == 1)))
> > + return 0;
>
> Should we refactor this into a mas_is_slot_store() predicate?
I'm not sure it's worth it as some of these are deciding factors on how
the store is executed so I would expect this to live in a single place,
long term.
Although, long-term this could be two store types: slot store rcu and
slot store so that the check only happens once.
>
> A few coding-style problems with it as it's currently written:
>
> 1. The indentation on the second line is wrong. It makes the
> continuation of the condition look like part of the statement. Use
> extra whitespace to indent. eg:
>
> if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree))
> || (wr_mas.offset_end - mas->offset == 1)))
> return 0;
>
> 2. The operator goes last on the line, not at the beginning of the
> continuation line. ie:
>
> if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree)) ||
> (wr_mas.offset_end - mas->offset == 1)))
> return 0;
>
> 3. You don't need parens around the !mt_in_rcu(mas->tree). There's
> no ambiguity to solve here:
>
> if ((node_size == mas->end) && (!mt_in_rcu(mas->tree) ||
> (wr_mas.offset_end - mas->offset == 1)))
> return 0;
>
> But I'd write it as:
>
> if ((node_size == mas->end) &&
> (!mt_in_rcu(mas->tree) || (wr_mas.offset_end - mas->offset == 1)))
> return 0;
>
> because then the whitespace matches how you're supposed to parse the
> condition, and so the next person to read this code will have an easier
> time of it.
>
>
> --
> maple-tree mailing list
> maple-tree at lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/maple-tree
More information about the maple-tree
mailing list