Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] lib/tests: unlocked cpu_online_mask read passed to work_on_cpu() - [Medium] lib/tests: missing release/acquire ordering on sh.started in contention test - [Low] lib/tests: unlocked num_online_cpus() check in do_hpcc_test_saturated_compare() --
commit 48b3ce088047ee1ccff5c20155903c1a143e78df Author: Mathieu Desnoyers <[email protected]> lib: test hierarchical per-cpu counters Introduce KUnit tests for hierarchical per-CPU counters covering single-threaded updates, concurrent multi-threaded updates, contended carries, and saturated counter comparisons. Link: https://lkml.kernel.org/r/[email protected] > diff --git a/lib/tests/percpu_counter_tree_kunit.c > b/lib/tests/percpu_counter_tree_kunit.c > new file mode 100644 > index 0000000000000..58b2e488e3c6b > --- /dev/null > +++ b/lib/tests/percpu_counter_tree_kunit.c [ ... ] > +static void do_hpcc_test_single_thread(struct kunit *test, int _cpu0, int > _cpu1, > + enum up_test_inc_type type) > +{ > + unsigned long batch_size_order = 5; > + int cpu0 = _cpu0; > + int cpu1 = _cpu1; > + unsigned long deadline = jiffies + SINGLE_THREAD_MAX_SECONDS * HZ; > + int ret, i; > + > + ret = init_counters(1UL << batch_size_order); > + KUNIT_EXPECT_EQ(test, ret, 0); > + if (ret) > + return; > + cpus_read_lock(); > + for (i = 0; i < 10000 && time_before(jiffies, deadline); i++) { > + struct single_thread_update u0 = { .counter_index = 0 }; > + struct single_thread_update u1 = { .counter_index = 1 }; > + long increment; > + > + switch (type) { > + case INC_ONE: > + increment = 1; > + break; > + case INC_MINUS_ONE: > + increment = -1; > + break; > + case INC_RANDOM: > + increment = (long)get_random_long() % 50000; > + break; > + default: > + WARN_ON_ONCE(1); > + increment = 0; > + } > + if (_cpu0 < 0) > + cpu0 = cpumask_any_distribute(cpu_online_mask); > + if (_cpu1 < 0) > + cpu1 = cpumask_any_distribute(cpu_online_mask); > + u0.increment = increment; > + u1.increment = increment; > + work_on_cpu(cpu0, single_thread_update_fn, &u0); > + work_on_cpu(cpu1, single_thread_update_fn, &u1); > + check_counters(test); > + } > + cpus_read_unlock(); > + fini_counters(); > +} > + > +static void hpcc_test_single_thread_first(struct kunit *test) > +{ > + int cpu = cpumask_first(cpu_online_mask); > + > + do_hpcc_test_single_thread(test, cpu, cpu, INC_ONE); > + do_hpcc_test_single_thread(test, cpu, cpu, INC_MINUS_ONE); > + do_hpcc_test_single_thread(test, cpu, cpu, INC_RANDOM); > +} > + > +static void hpcc_test_single_thread_first_random(struct kunit *test) > +{ > + int cpu = cpumask_first(cpu_online_mask); > + > + do_hpcc_test_single_thread(test, cpu, -1, INC_ONE); > + do_hpcc_test_single_thread(test, cpu, -1, INC_MINUS_ONE); > + do_hpcc_test_single_thread(test, cpu, -1, INC_RANDOM); > +} [Severity: Medium] Can cpu go offline before or between the calls to do_hpcc_test_single_thread()? Both hpcc_test_single_thread_first() and hpcc_test_single_thread_first_random() sample cpumask_first(cpu_online_mask) without holding cpus_read_lock(), and then pass that cpu across three separate calls to do_hpcc_test_single_thread(), each of which acquires and releases cpus_read_lock() on its own. Because _cpu0 >= 0 (and _cpu1 >= 0 in hpcc_test_single_thread_first()), do_hpcc_test_single_thread() never re-reads or validates cpu0 or cpu1 under cpus_read_lock() before passing them to work_on_cpu(), which requires the caller to ensure the target CPU cannot go offline. If CPU hot-unplug runs concurrently under CONFIG_HOTPLUG_CPU=y, could work_on_cpu() be called on an offline CPU? [ ... ] > +static int contention_worker_fn(void *data) > +{ > + struct contention_data *cd = data; > + struct contention_shared *sh = cd->sh; > + int round; > + > + wait_event(contention_wq, READ_ONCE(sh->started)); > + for (round = 1; round <= CONTENTION_NR_ROUNDS; round++) { > + if (cd->coordinator) { > + atomic_set_release(&sh->round, round); > + contention_add(cd); > + while (atomic_read_acquire(&sh->nr_done) < round * > (sh->nr_workers - 1)) > + cond_resched(); > +#ifdef CONFIG_SMP > + if (percpu_counter_tree_check_residuals(&counter[0])) > + sh->nr_errors++; > +#endif > + } else { > + while (atomic_read_acquire(&sh->round) < round) > + cond_resched(); > + contention_add(cd); > + /* Order the additions before the coordinator check. */ > + atomic_inc_return_release(&sh->nr_done); > + } > + } > + complete_work(); > + return 0; > +} > + > +static void do_hpcc_test_contended_carries(struct kunit *test, unsigned long > batch_size) > +{ [ ... ] > + for_each_cpu(cpu, cpus) { > + cd[cpu].sh = &sh; > + cd[cpu].coordinator = first; > + cd[cpu].seed = get_random_u32() | 1; > + task[cpu] = kthread_create_on_cpu(contention_worker_fn, > &cd[cpu], cpu, > + "kunit_hpcc_contention/%u"); > + KUNIT_EXPECT_FALSE(test, IS_ERR(task[cpu])); > + if (IS_ERR(task[cpu])) > + break; > + atomic_inc(&kernel_threads_to_run); > + get_task_struct(task[cpu]); > + wake_up_process(task[cpu]); > + sh.nr_workers++; > + first = false; > + } > + /* Start the workers once their number is known. */ > + WRITE_ONCE(sh.started, true); > + wake_up_all(&contention_wq); [Severity: Medium] On weakly-ordered architectures, can the coordinator thread in contention_worker_fn() observe sh->started == true before seeing all increments to sh.nr_workers? In do_hpcc_test_contended_carries(), each worker kthread (starting with the coordinator on the first iteration) is woken via wake_up_process(task[cpu]) inside the loop while sh.nr_workers++ is still being incremented on subsequent iterations, and completion is published with WRITE_ONCE(sh.started, true). In contention_worker_fn(), if the already-running coordinator hits the wait_event(contention_wq, READ_ONCE(sh->started)) fastpath before wake_up_all() is called, it does not go through the waitqueue spinlock and there is no release/acquire pairing ordering the updates to sh.nr_workers before the coordinator reads sh->nr_workers in: while (atomic_read_acquire(&sh->nr_done) < round * (sh->nr_workers - 1)) Could this cause the coordinator to wait for fewer workers than were created and call percpu_counter_tree_check_residuals(&counter[0]) while other workers are still updating counter[0]? Should sh.started use smp_store_release() and smp_load_acquire()? [ ... ] > +static void do_hpcc_test_saturated_compare(struct kunit *test, unsigned long > batch_size, > + long inc) > +{ [ ... ] > + cpus_read_lock(); > + for_each_online_cpu(cpu) { > + work_on_cpu(cpu, saturate_worker_fn, &sd); > + KUNIT_EXPECT_FALSE(test, sd.overrun); > + if (sd.overrun) > + break; > + } > + cpus_read_unlock(); > + > + precise = percpu_counter_tree_precise_sum(c); > + approx = percpu_counter_tree_approximate_sum(c); > + kunit_info(test, "batch_size=%lu inc=%ld precise=%ld approx=%ld > accuracy=%lu\n", > + batch_size, inc, precise, approx, accuracy); > + > + check_residuals(test, c); > + check_residuals(test, &probe); > + > + /* Accuracy contract. */ > + KUNIT_EXPECT_GE(test, approx, precise - (long)accuracy); > + KUNIT_EXPECT_LE(test, approx, precise + (long)accuracy); > + /* Saturating every possible CPU reaches the accuracy exactly. */ > + if (num_online_cpus() == nr_cpu_ids) > + KUNIT_EXPECT_EQ(test, (precise - approx) * inc, (long)accuracy); [Severity: Low] In do_hpcc_test_saturated_compare(), what happens if a CPU is offline during the for_each_online_cpu() saturation loop and comes online after cpus_read_unlock()? Because num_online_cpus() == nr_cpu_ids is checked after cpus_read_unlock() is dropped, a CPU brought online in that window would make num_online_cpus() == nr_cpu_ids true even though saturate_worker_fn() did not run on it, causing the KUNIT_EXPECT_EQ(test, (precise - approx) * inc, (long)accuracy) assertion to fail spuriously. Should num_online_cpus() == nr_cpu_ids be recorded while cpus_read_lock() is still held? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
