Re: [PATCH 2/2] lib/tests: errseq: add a concurrent check_and_advance test

[Jan Kara]

On Sun 27-09-26 10:47:26, Shashank Mohan Jain wrote:
> Add a test case that races errseq_check_and_advance() on one CPU
> against errseq_set() of a different error on another CPU, then records
> the first error again and lets a second cursor consume it. The second
> -EIO is recorded after the first cursor's check returned, so that
> cursor's next check must report an error, whichever way the race with
> -ENOSPC went.
> 
> Without the previous commit 1-6% of 2 million rounds lose the error
> on UML with 4 CPUs (seccomp=on ncpus=4); with it none do. The case is
> skipped on a single CPU and stops after 5 seconds.
> 
> Assisted-by: LLM
> Signed-off-by: Shashank Mohan Jain <jain.sm@xxxxxxxxx>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Needs the errseq KUnit suite from commit b52f5c1605f2 ("lib/tests: add
> KUnit tests for errseq"), which is in mm-nonmm-unstable. The case is
> skipped unless the kernel has at least two CPUs (on UML:
> --kernel_args seccomp=on --kernel_args ncpus=4).
> 
>  lib/tests/errseq_kunit.c | 110 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 105 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/tests/errseq_kunit.c b/lib/tests/errseq_kunit.c
> index 8f39ebc4a248..5ebc411ea749 100644
> --- a/lib/tests/errseq_kunit.c
> +++ b/lib/tests/errseq_kunit.c
> @@ -2,21 +2,26 @@
>  /*
>   * KUnit tests for the errseq_t error-tracking infrastructure.
>   *
> - * These exercise the documented single-threaded semantics of the errseq
> - * API (see Documentation/core-api/errseq.rst and lib/errseq.c): error
> + * Most cases exercise the documented single-threaded semantics of the
> + * errseq API (see Documentation/core-api/errseq.rst and lib/errseq.c): error
>   * recording and overwriting, the "seen" handoff between errseq_sample()
>   * and errseq_check_and_advance(), and the re-reporting of an error that
>   * is recorded again after it has been seen.
>   *
> - * The lockless properties of errseq_t under concurrent updates are
> - * outside the scope of these deterministic tests, as is the WARN path
> - * for invalid error values.
> + * The WARN path for invalid error values is not tested.  The last case
> + * races errseq_set() against errseq_check_and_advance() on another CPU.
>   */
>  #include <kunit/test.h>
>  
> +#include <linux/atomic.h>
> +#include <linux/cpumask.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/errseq.h>
> +#include <linux/jiffies.h>
> +#include <linux/kthread.h>
> +#include <linux/random.h>
> +#include <linux/sched.h>
>  
>  /*
>   * A zeroed errseq_t is the "no error has ever occurred" epoch: it
> @@ -209,6 +214,100 @@ static void errseq_test_two_subscribers_independent(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, errseq_check_and_advance(&eseq, &cursor_b), 0);
>  }
>  
> +/*
> + * errseq_check_and_advance() racing with errseq_set() on another CPU.
> + *
> + * Each round, cursor @f checks an unseen -EIO while another CPU records
> + * -ENOSPC.  Afterwards -EIO is recorded again and cursor @g checks.  The
> + * second -EIO is recorded after @f's check returned, so @f's next check
> + * must report an error, whichever way the race with -ENOSPC went.
> + */
> +struct errseq_race {
> +	errseq_t eseq;
> +	atomic_t go;		/* round the writer should run */
> +	atomic_t done;		/* last round the writer completed */
> +	unsigned int wdelay;
> +};
> +
> +static void errseq_race_spin(unsigned int n)
> +{
> +	while (n--)
> +		cpu_relax();
> +}
> +
> +static int errseq_race_writer(void *data)
> +{
> +	struct errseq_race *r = data;
> +	int seen = 0;
> +
> +	while (!kthread_should_stop()) {
> +		int round = atomic_read_acquire(&r->go);
> +
> +		if (round == seen) {
> +			cond_resched();
> +			continue;
> +		}
> +		seen = round;
> +		errseq_race_spin(READ_ONCE(r->wdelay));
> +		errseq_set(&r->eseq, -ENOSPC);
> +		atomic_set_release(&r->done, round);
> +	}
> +	return 0;
> +}
> +
> +#define ERRSEQ_RACE_ROUNDS	2000000
> +#define ERRSEQ_RACE_SECONDS	5
> +
> +static void errseq_test_check_and_advance_race(struct kunit *test)
> +{
> +	struct errseq_race *r;
> +	struct task_struct *writer;
> +	unsigned long deadline;
> +	int round, unreported = 0, missed = 0;
> +
> +	if (num_online_cpus() < 2)
> +		kunit_skip(test, "needs at least two CPUs");
> +
> +	r = kunit_kzalloc(test, sizeof(*r), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, r);
> +
> +	writer = kthread_run(errseq_race_writer, r, "errseq_race");
> +	KUNIT_ASSERT_FALSE(test, IS_ERR(writer));
> +
> +	deadline = jiffies + ERRSEQ_RACE_SECONDS * HZ;
> +	for (round = 1; round <= ERRSEQ_RACE_ROUNDS; round++) {
> +		errseq_t f = 0, g = 0;
> +
> +		WRITE_ONCE(r->eseq, 0);
> +		errseq_set(&r->eseq, -EIO);
> +		WRITE_ONCE(r->wdelay, get_random_u32_below(64));
> +		atomic_set_release(&r->go, round);
> +
> +		errseq_race_spin(get_random_u32_below(64));
> +		if (!errseq_check_and_advance(&r->eseq, &f))
> +			unreported++;
> +
> +		while (atomic_read_acquire(&r->done) != round)
> +			cond_resched();
> +
> +		/* Recorded after f's check returned: f must hear about it. */
> +		errseq_set(&r->eseq, -EIO);
> +		errseq_check_and_advance(&r->eseq, &g);
> +		if (!errseq_check_and_advance(&r->eseq, &f))
> +			missed++;
> +
> +		if (time_after(jiffies, deadline))
> +			break;
> +		if (!(round & 1023))
> +			cond_resched();
> +	}
> +	kthread_stop(writer);
> +
> +	kunit_info(test, "%d rounds, %d missed errors\n", round - 1, missed);
> +	KUNIT_EXPECT_EQ(test, unreported, 0);
> +	KUNIT_EXPECT_EQ(test, missed, 0);
> +}
> +
>  static struct kunit_case errseq_test_cases[] = {
>  	KUNIT_CASE(errseq_test_zero_epoch_reports_no_error),
>  	KUNIT_CASE(errseq_test_set_records_error),
> @@ -223,6 +322,7 @@ static struct kunit_case errseq_test_cases[] = {
>  	KUNIT_CASE(errseq_test_repeat_error_visible_to_all_cursors),
>  	KUNIT_CASE(errseq_test_advance_stable_when_unchanged),
>  	KUNIT_CASE(errseq_test_two_subscribers_independent),
> +	KUNIT_CASE_SLOW(errseq_test_check_and_advance_race),
>  	{}
>  };
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR