Re: [PATCH v3 1/2] mm/filemap: use xas_try_split() in __filemap_add_folio()

[Zi Yan <ziy@nvidia.com>]

On 8 Mar 2025, at 13:32, Zi Yan wrote:

> On 8 Mar 2025, at 13:14, SeongJae Park wrote:
>
>> Hello,
>>
>> On Wed, 26 Feb 2025 16:08:53 -0500 Zi Yan <ziy@nvidia.com> wrote:
>>
>>> During __filemap_add_folio(), a shadow entry is covering n slots and a
>>> folio covers m slots with m < n is to be added.  Instead of splitting all
>>> n slots, only the m slots covered by the folio need to be split and the
>>> remaining n-m shadow entries can be retained with orders ranging from m to
>>> n-1.  This method only requires
>>>
>>> 	(n/XA_CHUNK_SHIFT) - (m/XA_CHUNK_SHIFT)
>>>
>>> new xa_nodes instead of
>>> 	(n % XA_CHUNK_SHIFT) * ((n/XA_CHUNK_SHIFT) - (m/XA_CHUNK_SHIFT))
>>>
>>> new xa_nodes, compared to the original xas_split_alloc() + xas_split()
>>> one.  For example, to insert an order-0 folio when an order-9 shadow entry
>>> is present (assuming XA_CHUNK_SHIFT is 6), 1 xa_node is needed instead of
>>> 8.
>>>
>>> xas_try_split_min_order() is introduced to reduce the number of calls to
>>> xas_try_split() during split.
>>>
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> Cc: Hugh Dickins <hughd@google.com>
>>> Cc: Kairui Song <kasong@tencent.com>
>>> Cc: Miaohe Lin <linmiaohe@huawei.com>
>>> Cc: Mattew Wilcox <willy@infradead.org>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> Cc: Kirill A. Shuemov <kirill.shutemov@linux.intel.com>
>>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>>> Cc: Yang Shi <yang@os.amperecomputing.com>
>>> Cc: Yu Zhao <yuzhao@google.com>
>>> ---
>>>  include/linux/xarray.h |  7 +++++++
>>>  lib/xarray.c           | 25 +++++++++++++++++++++++
>>>  mm/filemap.c           | 45 +++++++++++++++++-------------------------
>>>  3 files changed, 50 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
>>> index 4010195201c9..78eede109b1a 100644
>>> --- a/include/linux/xarray.h
>>> +++ b/include/linux/xarray.h
>>> @@ -1556,6 +1556,7 @@ int xas_get_order(struct xa_state *xas);
>>>  void xas_split(struct xa_state *, void *entry, unsigned int order);
>>>  void xas_split_alloc(struct xa_state *, void *entry, unsigned int order, gfp_t);
>>>  void xas_try_split(struct xa_state *xas, void *entry, unsigned int order);
>>> +unsigned int xas_try_split_min_order(unsigned int order);
>>>  #else
>>>  static inline int xa_get_order(struct xarray *xa, unsigned long index)
>>>  {
>>> @@ -1582,6 +1583,12 @@ static inline void xas_try_split(struct xa_state *xas, void *entry,
>>>  		unsigned int order)
>>>  {
>>>  }
>>> +
>>> +static inline unsigned int xas_try_split_min_order(unsigned int order)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>>  #endif
>>>
>>>  /**
>>> diff --git a/lib/xarray.c b/lib/xarray.c
>>> index bc197c96d171..8067182d3e43 100644
>>> --- a/lib/xarray.c
>>> +++ b/lib/xarray.c
>>> @@ -1133,6 +1133,28 @@ void xas_split(struct xa_state *xas, void *entry, unsigned int order)
>>>  }
>>>  EXPORT_SYMBOL_GPL(xas_split);
>>>
>>> +/**
>>> + * xas_try_split_min_order() - Minimal split order xas_try_split() can accept
>>> + * @order: Current entry order.
>>> + *
>>> + * xas_try_split() can split a multi-index entry to smaller than @order - 1 if
>>> + * no new xa_node is needed. This function provides the minimal order
>>> + * xas_try_split() supports.
>>> + *
>>> + * Return: the minimal order xas_try_split() supports
>>> + *
>>> + * Context: Any context.
>>> + *
>>> + */
>>> +unsigned int xas_try_split_min_order(unsigned int order)
>>> +{
>>> +	if (order % XA_CHUNK_SHIFT == 0)
>>> +		return order == 0 ? 0 : order - 1;
>>> +
>>> +	return order - (order % XA_CHUNK_SHIFT);
>>> +}
>>> +EXPORT_SYMBOL_GPL(xas_try_split_min_order);
>>> +
>>
>> I found this makes build fails when CONFIG_XARRAY_MULTI is unset, like below.
>>
>>     /linux/lib/xarray.c:1251:14: error: redefinition of ‘xas_try_split_min_order’
>>      1251 | unsigned int xas_try_split_min_order(unsigned int order)
>>           |              ^~~~~~~~~~~~~~~~~~~~~~~
>>     In file included from /linux/lib/xarray.c:13:
>>     /linux/include/linux/xarray.h:1587:28: note: previous definition of ‘xas_try_split_min_order’ with type ‘unsigned int(unsigned int)’
>>      1587 | static inline unsigned int xas_try_split_min_order(unsigned int order)
>>           |                            ^~~~~~~~~~~~~~~~~~~~~~~
>>
>> I think we should have the definition only when CONFIG_XARRAY_MULTI?
>
> I think it might be a merge issue, since my original patch[1] places
> xas_try_split_min_order() above xas_try_split(), both of which are
> in #ifdef CONFIG_XARRAY_MULTI #endif. But mm-everything-2025-03-08-00-43
> seems to move xas_try_split_min_order() below xas_try_split() and
> out of CONFIG_XARRAY_MULTI guard.
>
> [1] https://lore.kernel.org/linux-mm/20250226210854.2045816-2-ziy@nvidia.com/

In addition, the new comment for xas_try_split() is added to xas_split() comment.
See https://web.git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/lib/xarray.c?h=mm-everything-2025-03-08-00-43#n1084

Something went wrong when this patch was applied.

--
Best Regards,
Yan, Zi