Messages in this thread | | | Date | Tue, 12 Dec 2023 13:48:56 -0800 | Subject | Re: [PATCH] maple_tree: do not preallocate nodes for slot stores | From | Sidhartha Kumar <> |
| |
On 12/12/23 12:57 PM, Matthew Wilcox wrote: > 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? yes, I think we should add helper functions to identify the different type of stores. Thanks for the pointers to code style this is what I think the slot store identifying helper function would look like:
static inline bool mas_wr_is_slot_store(struct ma_wr_state *wr_mas) { struct ma_state *mas = wr_mas->mas; unsigned char node_size = mas_wr_new_end(wr_mas);
if ((node_size == mas->end) && (!mt_in_rcu(mas->tree) || (wr_mas->offset_end - mas->offset == 1))) return true;
return false; }
thanks, Sid > 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. >
| |