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

[David CARLIER]

You're right. Here's the simplified version:

  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);

        /*
         * With approx_sum = precise_sum = 0, from the accuracy invariant:
         *   approx_sum - over <= precise_sum <= approx_sum + under
         * Positive deltas use 'under' as tolerance, negative use 'over'.
         */

        /* At boundary: indeterminate */
        KUNIT_EXPECT_EQ(test, 0,
                percpu_counter_tree_approximate_compare_value(&pct,
(long)under));
        KUNIT_EXPECT_EQ(test, 0,
                percpu_counter_tree_approximate_compare_value(&pct,
-(long)over));

        /* Beyond boundary: definitive */
        KUNIT_EXPECT_EQ(test, 1,
                percpu_counter_tree_approximate_compare_value(&pct,
                        (long)(under + 1)));
        KUNIT_EXPECT_EQ(test, -1,
                percpu_counter_tree_approximate_compare_value(&pct,
                        -(long)(over + 1)));

        /*
         * When ranges are asymmetric, test a value just past the smaller
         * range but within the larger. The side with the larger tolerance
         * must return indeterminate, the other must return definitive.
         * This catches any swap of the accuracy parameters.
         */
        if (under != over) {
                if (under > over) {
                        /* Positive side has larger tolerance */
                        KUNIT_EXPECT_EQ(test, 0,
                                percpu_counter_tree_approximate_compare_value(
                                        &pct, (long)(over + 1)));
                        KUNIT_EXPECT_EQ(test, -1,
                                percpu_counter_tree_approximate_compare_value(
                                        &pct, -(long)(over + 1)));
                } else {
                        /* Negative side has larger tolerance */
                        KUNIT_EXPECT_EQ(test, 1,
                                percpu_counter_tree_approximate_compare_value(
                                        &pct, (long)(under + 1)));
                        KUNIT_EXPECT_EQ(test, 0,
                                percpu_counter_tree_approximate_compare_value(
                                        &pct, -(long)(under + 1)));
                }
        }

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

  Thanks,
  David

On Mon, 16 Mar 2026 at 00:41, Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> On 2026-03-15 20:05, David CARLIER wrote:
> > Ok what about this revised version ?
> >
> > 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);
> >
> >          /*
> >           * With approx_sum = precise_sum = 0, from the accuracy invariant:
> >           *   approx_sum - over <= precise_sum <= approx_sum + under
> >           * Positive deltas use 'under' as tolerance, negative use 'over'.
> >           */
> >
> >          /* At boundary: indeterminate */
> >          KUNIT_EXPECT_EQ(test, 0,
> >                  percpu_counter_tree_approximate_compare_value(&pct,
> > (long)under));
> >          KUNIT_EXPECT_EQ(test, 0,
> >                  percpu_counter_tree_approximate_compare_value(&pct,
> > -(long)over));
> >
> >          /* Beyond boundary: definitive */
> >          KUNIT_EXPECT_EQ(test, 1,
> >                  percpu_counter_tree_approximate_compare_value(&pct,
> >                          (long)(under + 1)));
> >          KUNIT_EXPECT_EQ(test, -1,
> >                  percpu_counter_tree_approximate_compare_value(&pct,
> >                          -(long)(over + 1)));
> >
> >          /*
> >           * When ranges are asymmetric, test values in the gap between the
> >           * smaller and larger range to catch swapped accuracy parameters.
> >           * Determine which range is larger at runtime to avoid assuming a
> >           * specific relationship between under and over.
> >           */
> >          if (under != over) {
> >                  unsigned long a = max(under, over);
> >                  unsigned long b = min(under, over);
>
> Looking at the resulting code, I wonder if the a/b variables are
> useful after all.
>
> >
> >                  /*
> >                   * b + 1 is beyond the smaller range but within the larger.
> >                   * The side with the larger tolerance must return indeterminate,
> >                   * the side with the smaller tolerance must return definitive.
> >                   */
> >                  if (a == under) {
>
> This can become if (under > over) {
>
> >                          /* Positive side has larger tolerance */
> >                          KUNIT_EXPECT_EQ(test, 0,
> >                                  percpu_counter_tree_approximate_compare_value(
> >                                          &pct, (long)(b + 1)));
>
> "b" becomes "under"
>
> >                          KUNIT_EXPECT_EQ(test, -1,
> >                                  percpu_counter_tree_approximate_compare_value(
> >                                          &pct, -(long)(b + 1)));
>
> same.
>
> >                  } else {
> >                          /* Negative side has larger tolerance */
> >                          KUNIT_EXPECT_EQ(test, 1,
> >                                  percpu_counter_tree_approximate_compare_value(
> >                                          &pct, (long)(b + 1)));
>
> "b" becomes "over".
>
> >                          KUNIT_EXPECT_EQ(test, 0,
> >                                  percpu_counter_tree_approximate_compare_value(
> >                                          &pct, -(long)(b + 1)));
>
> same.
>
> Thanks,
>
> Mathieu
>
> >                  }
> >          }
> >
> >          percpu_counter_tree_destroy(&pct);
> >          kfree(counter_items);
> >    }
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com