| Message ID | 20240907084927.1547-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] maple_tree: remove maple_big_node.parent | expand |
* Wei Yang <richard.weiyang@gmail.com> [240907 04:50]: > In maple_big_node, we define slot and padding/gap in a union. And based > on current definition of MAPLE_BIG_NODE_SLOTS/GAPS, padding is always > less then slot and part of the gap is overlapped by slot. ^^^^- than > > For example on 64bit system: > > MAPLE_BIG_NODE_SLOT is 34 > MAPLE_BIG_NODE_GAP is 21 > > With this knowledge, current code actually clear the whole > maple_big_node and even clear some space twice. > > Instead of clear maple_big_node each field separately, let's clear it in > one memset. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > CC: Liam R. Howlett <Liam.Howlett@Oracle.com> > > --- > Liam: > > This looks obvious, so I just run the ./maple test to see it doesn't > break anything. > The big node also includes the type, which isn't cleared. However it is unconditionally set below, so your change log is not correct in the statement that it is fully cleared, but the code will work and it is worth fixing what you have found. If you are sending more than one patch, it is better to make a cover letter to explain your series. It is probably worth re-spinning the series to fix the comment, but these changes look good. I'll have a closer look later. > Do you think I need to add a benchmark and run a perf for this kind of > change? No, you are doing less work so this should be better as long as it is correct. Zeroing can be expensive on some archs so this is good. > --- > lib/maple_tree.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c > index d8f10773e451..911c5e04e634 100644 > --- a/lib/maple_tree.c > +++ b/lib/maple_tree.c > @@ -3134,10 +3134,7 @@ static inline void mast_fill_bnode(struct maple_subtree_state *mast, > bool cp = true; > unsigned char split; > > - memset(mast->bn->gap, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->gap)); > - memset(mast->bn->slot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->slot)); > - memset(mast->bn->pivot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->pivot)); > - mast->bn->b_end = 0; > + memset(mast->bn, 0, sizeof(struct maple_big_node)); > > if (mte_is_root(mas->node)) { > cp = false; > -- > 2.34.1 >
On Sat, Sep 07, 2024 at 09:29:01AM -0400, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [240907 04:50]: >> In maple_big_node, we define slot and padding/gap in a union. And based >> on current definition of MAPLE_BIG_NODE_SLOTS/GAPS, padding is always >> less then slot and part of the gap is overlapped by slot. > ^^^^- than > >> >> For example on 64bit system: >> >> MAPLE_BIG_NODE_SLOT is 34 >> MAPLE_BIG_NODE_GAP is 21 >> >> With this knowledge, current code actually clear the whole >> maple_big_node and even clear some space twice. >> >> Instead of clear maple_big_node each field separately, let's clear it in >> one memset. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> CC: Liam R. Howlett <Liam.Howlett@Oracle.com> >> >> --- >> Liam: >> >> This looks obvious, so I just run the ./maple test to see it doesn't >> break anything. >> > >The big node also includes the type, which isn't cleared. However it is >unconditionally set below, so your change log is not correct in the >statement that it is fully cleared, but the code will work and it is >worth fixing what you have found. > I will mention this next time. >If you are sending more than one patch, it is better to make a >cover letter to explain your series. > Ok, I will add a cover letter next time. >It is probably worth re-spinning the series to fix the comment, but >these changes look good. I'll have a closer look later. > Thank, I will re-arrange the change log and add a cover letter for v2. >> Do you think I need to add a benchmark and run a perf for this kind of >> change? > >No, you are doing less work so this should be better as long as it is >correct. Zeroing can be expensive on some archs so this is good. > >> --- >> lib/maple_tree.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/lib/maple_tree.c b/lib/maple_tree.c >> index d8f10773e451..911c5e04e634 100644 >> --- a/lib/maple_tree.c >> +++ b/lib/maple_tree.c >> @@ -3134,10 +3134,7 @@ static inline void mast_fill_bnode(struct maple_subtree_state *mast, >> bool cp = true; >> unsigned char split; >> >> - memset(mast->bn->gap, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->gap)); >> - memset(mast->bn->slot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->slot)); >> - memset(mast->bn->pivot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->pivot)); >> - mast->bn->b_end = 0; >> + memset(mast->bn, 0, sizeof(struct maple_big_node)); >> >> if (mte_is_root(mas->node)) { >> cp = false; >> -- >> 2.34.1 >>
diff --git a/lib/maple_tree.c b/lib/maple_tree.c index d8f10773e451..911c5e04e634 100644 --- a/lib/maple_tree.c +++ b/lib/maple_tree.c @@ -3134,10 +3134,7 @@ static inline void mast_fill_bnode(struct maple_subtree_state *mast, bool cp = true; unsigned char split; - memset(mast->bn->gap, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->gap)); - memset(mast->bn->slot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->slot)); - memset(mast->bn->pivot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->pivot)); - mast->bn->b_end = 0; + memset(mast->bn, 0, sizeof(struct maple_big_node)); if (mte_is_root(mas->node)) { cp = false;
In maple_big_node, we define slot and padding/gap in a union. And based on current definition of MAPLE_BIG_NODE_SLOTS/GAPS, padding is always less then slot and part of the gap is overlapped by slot. For example on 64bit system: MAPLE_BIG_NODE_SLOT is 34 MAPLE_BIG_NODE_GAP is 21 With this knowledge, current code actually clear the whole maple_big_node and even clear some space twice. Instead of clear maple_big_node each field separately, let's clear it in one memset. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Liam R. Howlett <Liam.Howlett@Oracle.com> --- Liam: This looks obvious, so I just run the ./maple test to see it doesn't break anything. Do you think I need to add a benchmark and run a perf for this kind of change? --- lib/maple_tree.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)