Re: [PATCH v2 13/16] maple_tree: have mas_store() allocate nodes if needed

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

On 6/13/24 8:00 AM, Liam R. Howlett wrote:
> * Sidhartha Kumar <sidhartha.kumar@oracle.com> [240607 14:53]:
>> Not all users of mas_store() enter with nodes already preallocated.
>> Check for the MA_STATE_PREALLOC flag to decide whether to preallocate nodes
>> within mas_store() rather than relying on future write helper functions
>> to perform the allocations. This allows the write helper functions to be
>> simplified as they do not have to do checks to make sure there are
>> enough allocated nodes to perform the write.
>>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>>   lib/maple_tree.c | 32 ++++++++++++++++++++++++++++----
>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
>> index 8b17768db5f2..92f133ea5f00 100644
>> --- a/lib/maple_tree.c
>> +++ b/lib/maple_tree.c
>> @@ -5528,6 +5528,20 @@ static inline void mte_destroy_walk(struct maple_enode *enode,
>>   		mt_destroy_walk(enode, mt, true);
>>   	}
>>   }
>> +
>> +static inline void mas_wr_store_prealloc(struct ma_wr_state *wr_mas, void *entry)
>> +{
>> +	struct ma_state *mas = wr_mas->mas;
>> +	int request;
>> +
>> +	mas_wr_prealloc_setup(wr_mas);
>> +	mas_wr_store_type(wr_mas);
>> +	request = mas_prealloc_calc(mas, entry);
>> +	if (!request)
>> +		return;
>> +
>> +	mas_node_count(mas, request);
>> +}
>>   /* Interface */
>>   
>>   /**
>> @@ -5536,8 +5550,6 @@ static inline void mte_destroy_walk(struct maple_enode *enode,
>>    * @entry: The entry to store.
>>    *
>>    * The @mas->index and @mas->last is used to set the range for the @entry.
>> - * Note: The @mas should have pre-allocated entries to ensure there is memory to
>> - * store the entry.  Please see mas_expected_entries()/mas_destroy() for more details.
>>    *
>>    * Return: the first entry between mas->index and mas->last or %NULL.
>>    */
>> @@ -5563,9 +5575,21 @@ void *mas_store(struct ma_state *mas, void *entry)
>>   	 * want to examine what happens if a single store operation was to
>>   	 * overwrite multiple entries within a self-balancing B-Tree.
>>   	 */
>> -	mas_wr_prealloc_setup(&wr_mas);
>> -	mas_wr_store_type(&wr_mas);
>> +	if (mas->mas_flags & MA_STATE_PREALLOC) {
>> +		mas_wr_prealloc_setup(&wr_mas);
>> +		mas_wr_store_type(&wr_mas);
>> +		mas_wr_store_entry(&wr_mas);
>> +		MAS_WR_BUG_ON(&wr_mas, mas_is_err(mas));
>> +		return wr_mas.content;
>> +	}
>> +
>> +	mas_wr_store_prealloc(&wr_mas, entry);
> 
> Since this is the only place mas_wr_store_prealloc() is called and the
> first two things it does is the same as the if() statement above, then
> both calls can be moved outside of the if() statement.  That also means
> the helper function is somewhat small, which makes it seem like it's
> probably not worth having?  Maybe I missed something though?
> 

I agree the helper function is not needed, I'll refactor this without using 
another function.

Thanks,
Sid

>> +	WARN_ON_ONCE(mas->store_type == wr_invalid);
>> +	if (mas_is_err(mas))
>> +		return NULL;
>> +
>>   	mas_wr_store_entry(&wr_mas);
>> +	mas_destroy(mas);
>>   	return wr_mas.content;
>>   }
>>   EXPORT_SYMBOL_GPL(mas_store);
>> -- 
>> 2.45.2
>>