Re: [PATCH v3] lib: xarray: free unused spare node in xas_create_range()

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 12/4/25 15:15, Shardul Bankar wrote:
> On Mon, 2025-12-01 at 09:39 +0100, David Hildenbrand (Red Hat) wrote:
>> Please don't post new versions as reply to old versions.
>> ...
>>
>> ...
>> The first thing xas_destroy() does is check whether xa_alloc is set.
>>
>> I'd assume that the compiler is smart enough to inline xas_destroy()
>> completely here, so likely the xa_alloc check here can just be
>> dropped.
> 
> Got it, will share a v4 of the patch on a new chain with redundant
> xas_destroy() removed.
> 
>> Staring at xas_destroy() callers, we only have a single one outside
>> of
>> lib: mm/huge_memory.c:__folio_split()
>>
>> Is that one still required?
> 
> I checked the callers of xas_destroy(). Apart from the internal uses in
> lib/xarray.c and the unit tests in lib/test_xarray.c, the only external
> user is indeed mm/huge_memory.c:__folio_split().
> 
> That path is slightly different from the xas_nomem() retry loop I fixed
> in xas_create_range():
> 
> 	__folio_split() goes through xas_split_alloc() and then
> xas_split() / xas_try_split(), which allocate and consume nodes via
> xas->xa_alloc.
> 
> 	The final xas_destroy(&xas) in __folio_split() is there to
> drop any leftover split-allocation nodes, not the xas_nomem() spare
> node I handled in xas_create_range().
> 
> So with the current code I don’t think I can safely declare that
> xas_destroy() in __folio_split() is redundant- it still acts as the
> last cleanup for the split helpers.
> 
> For v4 I’d therefore like to keep the scope focused on the syzkaller
> leak and just drop the redundant "if (xa_alloc)" around xas_destroy()
> in xas_create_range() as you suggested.
> 
> Separately, I agree it would be cleaner if the split helpers guaranteed
> that xa_alloc is always cleared on return, so callers never have to
> think about xas_destroy(). I can take a closer look at xas_split() /
> xas_try_split() and, if it looks sound, propose a small follow-up
> series that makes their cleanup behaviour explicit and then removes the
> xas_destroy() from __folio_split().

That makes sense, thanks. Handling it internally is certainly harder to 
mess up by callers ...

-- 
Cheers

David