Re: [PATCH] lib: fix compare_delta parameter order in percpu_counter_tree

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2026-03-14 17:45, Andrew Morton wrote:
> On Fri, 13 Mar 2026 17:54:24 +0000 David Carlier <devnexen@gmail.com> wrote:
> 

Thanks Andrew for CCing me on this.

>> The compare_delta() helper takes (delta, accuracy_neg, accuracy_pos),
>> but every call site passes (delta, accuracy_pos, accuracy_neg) — the
>> last two arguments are consistently swapped.
>>
>> The documented invariant (include/linux/percpu_counter_tree.h) is:
>>
>>    (precise_sum - under) <= approx_sum <= (precise_sum + over)
>>
>> Which means precise_sum is in [approx_sum - over, approx_sum + under].

Yes, as also documented in the comment above
percpu_counter_tree_approximate_min_max_range().

>>
>> For a positive delta (v - approx_sum >= 0), accuracy_pos must be
>> "under" (the maximum amount precise_sum can exceed approx_sum).
>> For a negative delta, accuracy_neg must be "over".

Right, I mistakenly swapped the logic there. I did not consider that we
have "approximated value" as input and wish to compare precise ranges.

>> Since under > over
>> always (batch_size * M vs (batch_size - 1) * M), swapping them causes
>> false definitive results: the functions return 1 ("v > counter") when
>> the correct answer is 0 (indeterminate).
>>
>> This affects all comparison functions:
>> - percpu_counter_tree_approximate_compare_value()
>> - percpu_counter_tree_approximate_compare()
>> - percpu_counter_tree_precise_compare_value()
>> - percpu_counter_tree_precise_compare()
>>
>> The precise variants are also affected because their approximate
>> fast-path can short-circuit with a wrong result, skipping the precise
>> sum computation.

Yes.

>>
>> Fix by swapping the parameter order in compare_delta() itself, since
>> all call sites are consistently swapped.

That seems like a simple fix for that problem indeed. Did you run the
kunit tests on the fixed code ? Any thought on how to extend the kunit
test to cover this ?

> 
> This affects mm-unstable's "lib: introduce hierarchical per-cpu
> counters", so let's cc Mathieu.

Thanks!

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> 
>> --- a/lib/percpu_counter_tree.c
>> +++ b/lib/percpu_counter_tree.c
>> @@ -458,7 +458,7 @@ long percpu_counter_tree_precise_sum(struct percpu_counter_tree *counter)
>>   EXPORT_SYMBOL_GPL(percpu_counter_tree_precise_sum);
>>   
>>   static
>> -int compare_delta(long delta, unsigned long accuracy_neg, unsigned long accuracy_pos)
>> +int compare_delta(long delta, unsigned long accuracy_pos, unsigned long accuracy_neg)
>>   {
>>   	if (delta >= 0) {
>>   		if (delta <= accuracy_pos)
> 


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com