Message-ID: <a0da82bf-1462-4c4f-85bc-bfcccf714fc6@arm.com>
Date: Tue, 3 Jun 2025 19:37:27 +0530
From: Dev Jain <dev.jain@....com>
To: Zi Yan <ziy@...dia.com>
Cc: David Hildenbrand <david@...hat.com>, akpm@...ux-foundation.org,
 willy@...radead.org, linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, anshuman.khandual@....com, ryan.roberts@....com
Subject: Re: [PATCH] xarray: Add a BUG_ON() to ensure caller is not sibling


On 03/06/25 7:27 pm, Zi Yan wrote:
> On 3 Jun 2025, at 8:59, Dev Jain wrote:
>
>> On 03/06/25 5:47 pm, Zi Yan wrote:
>>> On 3 Jun 2025, at 3:58, David Hildenbrand wrote:
>>>
>>>> On 03.06.25 07:23, Dev Jain wrote:
>>>>> On 02/06/25 8:33 pm, Zi Yan wrote:
>>>>>> On 29 May 2025, at 23:44, Dev Jain wrote:
>>>>>>
>>>>>>> On 30/05/25 4:17 am, Zi Yan wrote:
>>>>>>>> On 28 May 2025, at 23:17, Dev Jain wrote:
>>>>>>>>
>>>>>>>>> On 28/05/25 10:42 pm, Zi Yan wrote:
>>>>>>>>>> On 28 May 2025, at 7:31, Dev Jain wrote:
>>>>>>>>>>
>>>>>>>>>>> Suppose xas is pointing somewhere near the end of the multi-entry batch.
>>>>>>>>>>> Then it may happen that the computed slot already falls beyond the batch,
>>>>>>>>>>> thus breaking the loop due to !xa_is_sibling(), and computing the wrong
>>>>>>>>>>> order. Thus ensure that the caller is aware of this by triggering a BUG
>>>>>>>>>>> when the entry is a sibling entry.
>>>>>>>>>> Is it possible to add a test case in lib/test_xarray.c for this?
>>>>>>>>>> You can compile the tests with “make -C tools/testing/radix-tree”
>>>>>>>>>> and run “./tools/testing/radix-tree/xarray”.
>>>>>>>>> Sorry forgot to Cc you.
>>>>>>>>> I can surely do that later, but does this patch look fine?
>>>>>>>> I am not sure the exact situation you are describing, so I asked you
>>>>>>>> to write a test case to demonstrate the issue. :)
>>>>>>> Suppose we have a shift-6 node having an order-9 entry => 8 - 1 = 7 siblings,
>>>>>>> so assume the slots are at offset 0 till 7 in this node. If xas->xa_offset is 6,
>>>>>>> then the code will compute order as 1 + xas->xa_node->shift = 7. So I mean to
>>>>>>> say that the order computation must start from the beginning of the multi-slot
>>>>>>> entries, that is, the non-sibling entry.
>>>>>> Got it. Thanks for the explanation. It will be great to add this explanation
>>>>>> to the commit log.
>>>>>>
>>>>>> I also notice that in the comment of xas_get_order() it says
>>>>>> “Called after xas_load()” and xas_load() returns NULL or an internal
>>>>>> entry for a sibling. So caller is responsible to make sure xas is not pointing
>>>>>> to a sibling entry. It is good to have a check here.
>>>>>>
>>>>>> In terms of the patch, we are moving away from BUG()/BUG_ON(), so I wonder
>>>>>> if there is a less disruptive way of handling this. Something like return
>>>>>> -EINVAL instead with modified function comments and adding a comment
>>>>>> at the return -EIVAL saying something like caller needs to pass
>>>>>> a non-sibling entry.
>>>>> What's the reason for moving away from BUG_ON()?
>>>> BUG_ON is in general a bad thing. See Documentation/process/coding-style.rst and the history on the related changes for details.
>>>>
>>>> Here, it is less critical than it looks.
>>>>
>>>> XA_NODE_BUG_ON is only active with XA_DEBUG.
>>>>
>>>> And XA_DEBUG is only defined in
>>>>
>>>> tools/testing/shared/xarray-shared.h:#define XA_DEBUG
>>>>
>>>> So IIUC, it's only active in selftests, and completely inactive in any kernel builds.
>>> Oh, I missed that. But that also means this patch becomes a nop in kernel
>> Yes, but given other places are there with XA_NODE_BUG_ON(), I believe
>> this patch has some value :)
> Sure. Can you please also add something like below to the function comment?
> “The xas cannot be a sibling entry, otherwise the result will be wrong”
> It saves other’s time to infer it from the added XA_NODE_BUG_ON().

Sure.

>
> Thanks.
>
> Best Regards,
> Yan, Zi

Powered by blists - more mailing lists