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

[David CARLIER]

On Sun, 15 Mar 2026 at 22:00, Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> On 2026-03-14 17:45, Andrew Morton wrote:
> > On Fri, 13 Mar 2026 17:54:24 +0000 David Carlier <devnexen@xxxxxxxxx> 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 ?

Yes.

Any thought on how to extend the kunit
> test to cover this ?

Can something like this be applicable ?

static void hpcc_test_compare_value_boundaries(struct kunit *test)
  {
      struct percpu_counter_tree pct;
      struct percpu_counter_tree_level_item *counter_items;
      unsigned long under = 0, over = 0;
      int ret;

      counter_items = kzalloc(percpu_counter_tree_items_size(), GFP_KERNEL);
      KUNIT_ASSERT_PTR_NE(test, counter_items, NULL);
      ret = percpu_counter_tree_init(&pct, counter_items, 32, GFP_KERNEL);
      KUNIT_ASSERT_EQ(test, ret, 0);

      percpu_counter_tree_set(&pct, 0);
      percpu_counter_tree_approximate_accuracy_range(&pct, &under, &over);

      /* Skip if under == over (no gap to test) */
      if (under == over)
          goto out;

      /* Positive boundary: v = under should be indeterminate (precise
could be up to +under) */
      KUNIT_EXPECT_EQ(test, 0,
          percpu_counter_tree_approximate_compare_value(&pct, (long)under));

      /* Positive boundary: v = over + 1 should be indeterminate */
      KUNIT_EXPECT_EQ(test, 0,
          percpu_counter_tree_approximate_compare_value(&pct,
(long)(over + 1)));

      /* Positive boundary: v = under + 1 should be definitively greater */
      KUNIT_EXPECT_EQ(test, 1,
          percpu_counter_tree_approximate_compare_value(&pct,
(long)(under + 1)));

      /* Negative boundary: -(over + 1) should be definitively less */
      KUNIT_EXPECT_EQ(test, -1,
          percpu_counter_tree_approximate_compare_value(&pct,
-(long)(over + 1)));

      /* Negative boundary: -under should be indeterminate */
      KUNIT_EXPECT_EQ(test, 0,
          percpu_counter_tree_approximate_compare_value(&pct, -(long)under));

      /* Negative boundary: -(under + 1) should be definitively less */
      KUNIT_EXPECT_EQ(test, -1,
          percpu_counter_tree_approximate_compare_value(&pct,
-(long)(under + 1)));

  out:
      percpu_counter_tree_destroy(&pct);
      kfree(counter_items);
  }


>
> >
> > This affects mm-unstable's "lib: introduce hierarchical per-cpu
> > counters", so let's cc Mathieu.
>
> Thanks!
>
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
>
> >
> >> --- 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