Re: [PATCH 2/2] maple_tree: memset maple_big_node as a whole

[Wei Yang <richard.weiyang@gmail.com>]

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
>> 

-- 
Wei Yang
Help you, Help me