Message ID | 20241218154613.58754-3-shikemeng@huaweicloud.com (mailing list archive) |
---|---|
State | Handled Elsewhere, archived |
Headers | show |
Series | Fix and cleanups to xarray | expand |
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)? > --- 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
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 >
Hi Kemeng, On Thu, 6 Feb 2025 at 07:13, Kemeng Shi <shikemeng@huaweicloud.com> wrote: > on 1/28/2025 12:21 AM, Geert Uytterhoeven wrote: > > 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! You are right: CONFIG_XARRAY_MULTI is enabled in my arm32 build, but not in my m68k build. Gr{oetje,eeting}s, Geert
diff --git a/lib/test_xarray.c b/lib/test_xarray.c index d5c5cbba33ed..6932a26f4927 100644 --- 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); + } static noinline void check_move_tiny(struct xarray *xa) diff --git a/lib/xarray.c b/lib/xarray.c index fa87949719a0..d0732c5b8403 100644 --- 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;
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> --- lib/test_xarray.c | 35 +++++++++++++++++++++++++++++++++++ lib/xarray.c | 1 + 2 files changed, 36 insertions(+)