Re: [PATCH v4 2/5] Xarray: move forward index correctly in xas_pause()

[Kemeng Shi <shikemeng@huaweicloud.com>]

on 1/28/2025 12:21 AM, Geert Uytterhoeven wrote:
> Hi Kemeng,
> 
> On Wed, 18 Dec 2024 at 07:58, Kemeng Shi <shikemeng@huaweicloud.com> wrote:
>> After xas_load(), xas->index could point to mid of found multi-index entry
>> and xas->index's bits under node->shift maybe non-zero. The afterward
>> xas_pause() will move forward xas->index with xa->node->shift with bits
>> under node->shift un-masked and thus skip some index unexpectedly.
>>
>> Consider following case:
>> Assume XA_CHUNK_SHIFT is 4.
>> xa_store_range(xa, 16, 31, ...)
>> xa_store(xa, 32, ...)
>> XA_STATE(xas, xa, 17);
>> xas_for_each(&xas,...)
>> xas_load(&xas)
>> /* xas->index = 17, xas->xa_offset = 1, xas->xa_node->xa_shift = 4 */
>> xas_pause()
>> /* xas->index = 33, xas->xa_offset = 2, xas->xa_node->xa_shift = 4 */
>> As we can see, index of 32 is skipped unexpectedly.
>>
>> Fix this by mask bit under node->xa_shift when move forward index in
>> xas_pause().
>>
>> For now, this will not cause serious problems. Only minor problem
>> like cachestat return less number of page status could happen.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> Thanks for your patch, which is now commit c9ba5249ef8b080c ("Xarray:
> move forward index correctly in xas_pause()") upstream.
> 
>> --- a/lib/test_xarray.c
>> +++ b/lib/test_xarray.c
>> @@ -1448,6 +1448,41 @@ static noinline void check_pause(struct xarray *xa)
>>         XA_BUG_ON(xa, count != order_limit);
>>
>>         xa_destroy(xa);
>> +
>> +       index = 0;
>> +       for (order = XA_CHUNK_SHIFT; order > 0; order--) {
>> +               XA_BUG_ON(xa, xa_store_order(xa, index, order,
>> +                                       xa_mk_index(index), GFP_KERNEL));
>> +               index += 1UL << order;
>> +       }
>> +
>> +       index = 0;
>> +       count = 0;
>> +       xas_set(&xas, 0);
>> +       rcu_read_lock();
>> +       xas_for_each(&xas, entry, ULONG_MAX) {
>> +               XA_BUG_ON(xa, entry != xa_mk_index(index));
>> +               index += 1UL << (XA_CHUNK_SHIFT - count);
>> +               count++;
>> +       }
>> +       rcu_read_unlock();
>> +       XA_BUG_ON(xa, count != XA_CHUNK_SHIFT);
>> +
>> +       index = 0;
>> +       count = 0;
>> +       xas_set(&xas, XA_CHUNK_SIZE / 2 + 1);
>> +       rcu_read_lock();
>> +       xas_for_each(&xas, entry, ULONG_MAX) {
>> +               XA_BUG_ON(xa, entry != xa_mk_index(index));
>> +               index += 1UL << (XA_CHUNK_SHIFT - count);
>> +               count++;
>> +               xas_pause(&xas);
>> +       }
>> +       rcu_read_unlock();
>> +       XA_BUG_ON(xa, count != XA_CHUNK_SHIFT);
>> +
>> +       xa_destroy(xa);
>> +
>>  }
> 
> On m68k, the last four XA_BUG_ON() checks above are triggered when
> running the test.  With extra debug prints added:
> 
>     entry = 00000002 xa_mk_index(index) = 000000c1
>     entry = 00000002 xa_mk_index(index) = 000000e1
>     entry = 00000002 xa_mk_index(index) = 000000f1
>     ...
>     entry = 000000e2 xa_mk_index(index) = fffff0ff
>     entry = 000000f9 xa_mk_index(index) = fffff8ff
>     entry = 000000f2 xa_mk_index(index) = fffffcff
>     count = 63 XA_CHUNK_SHIFT = 6
>     entry = 00000081 xa_mk_index(index) = 00000001
>     entry = 00000002 xa_mk_index(index) = 00000081
>     entry = 00000002 xa_mk_index(index) = 000000c1
>     ...
>     entry = 000000e2 xa_mk_index(index) = ffffe0ff
>     entry = 000000f9 xa_mk_index(index) = fffff0ff
>     entry = 000000f2 xa_mk_index(index) = fffff8ff
>      count = 62 XA_CHUNK_SHIFT = 6
> 
> On arm32, the test succeeds, so it's probably not a 32-vs-64-bit issue.
> Perhaps a big-endian or alignment issue (alignof(int/long) = 2)?
Hi Geert,
Sorry for late reply. After check the debug info and the code, I think
the test is failed because CONFIG_XARRAY_MULTI is off. I guess
CONFIG_XARRAY_MULTI is on arm32 and is off on m68k so the test result
diffs. Luckly it's only a problem of of test code.
I will send patch to correct the test code soon. Thanks!

Kemeng

> 
>> --- a/lib/xarray.c
>> +++ b/lib/xarray.c
>> @@ -1147,6 +1147,7 @@ void xas_pause(struct xa_state *xas)
>>                         if (!xa_is_sibling(xa_entry(xas->xa, node, offset)))
>>                                 break;
>>                 }
>> +               xas->xa_index &= ~0UL << node->shift;
>>                 xas->xa_index += (offset - xas->xa_offset) << node->shift;
>>                 if (xas->xa_index == 0)
>>                         xas->xa_node = XAS_BOUNDS;
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>