Re: [PATCH v5 2/2] selftests/cgroup: test_zswap: fix implicit unsigned promotion bug in test_no_kmem_bypass

[Wilson Felipe Pereira <wfelipe@google.com>]

Michal Koutný <mkoutny@suse.com> writes:

> On Tue, Sep 01, 2026 at 05:22:28AM +0000, Wilson Felipe Pereira <wfelipe@google.com> wrote:
>> In test_no_kmem_bypass(), delta (stored_pages * page_size - zswapped) is
>> checked against stored_pages * page_size / 4 to verify that the pages
>> pushed to zswap belong to the test memory cgroup.
>>
>> Due to slight stat update timing differences, delta can evaluate to a small
>> negative number (e.g. -5MB out of 1GB). Because delta is declared as a
>> signed int and stored_pages is an unsigned size_t, C's usual arithmetic
>> conversions implicitly promote a negative delta to a large unsigned 64-bit
>> integer, causing `delta < stored_pages * page_size / 4` to falsely evaluate
>> to 0 and fail the test.
>>
>> Fix this by declaring zswapped and delta as signed long long and comparing
>> against a signed threshold, ensuring negative deltas correctly evaluate
>> to true.
>>
>> Fixes: a549f9f31561a ("selftests: cgroup: add test_zswap with no kmem bypass test")
>> Signed-off-by: Wilson Felipe Pereira <wfelipe@google.com>
>> Acked-by: Michal Koutný <mkoutny@suse.com>
>
> (The long long helper is a non-trivial change, so the ack should be
> stripped on this version (unless you can convince me ;-).)

I'm sorry for that! Thanks for the review and for the patience.

>
> I'm afraid this bitness propagated too broadly.
>
> The long vs long long difference is only relevant on 32b, right?
> And the bit-width issue only appears because of the product, not the
> values read from memory.stat [1].
>
> I'd consider three ways forward:
> a) Consolidate the helpers into one cg_read_key_s64() (same explicit size
>    regardless of arch),
> b) keep single arch-dependent helper and tackle product(s) only,
> c) do not bother with 32b in these selftests (I have a hunch that
>    they're not so relevant on such archs).
>
> WDYT?

I like this option a), any thoughts on the sequence here?

I thought the following steps:
1) Add cg_read_key_s64() here and make use of it;
2) In another patch (probably 3/3), replace all uses of cg_read_key_long
   by cg_read_key_s64. There are 41 uses currently.

I believe the tests could be flaky in 32b already (just a guess), at
least option a) would make all be more deterministic.

>
> Michal
>
>
> [1] At least the values derived from available memory, memory events
>     could grow indefinitely. It's true that kernel uses explicit u64 for
>     all of the entries.