On 11/25/20 4:51 PM, Alexandru Elisei wrote:
> check_acked() has several peculiarities: is the only function among the
> check_* functions which calls report() directly, it does two things
> (waits for interrupts and checks for misfired interrupts) and it also
> mixes printf, report_info and report calls.
>
> check_acked() also reports a pass and returns as soon all the target CPUs
> have received interrupts, However, a CPU not having received an interrupt
> *now* does not guarantee not receiving an eroneous interrupt if we wait
erroneous
> long enough.
>
> Rework the function by splitting it into two separate functions, each with
> a single responsability: wait_for_interrupts(), which waits for the
> expected interrupts to fire, and check_acked() which checks that interrupts
> have been received as expected.
>
> wait_for_interrupts() also waits an extra 100 milliseconds after the
> expected interrupts have been received in an effort to make sure we don't
> miss misfiring interrupts.
>
> Splitting check_acked() into two functions will also allow us to
> customize the behavior of each function in the future more easily
> without using an unnecessarily long list of arguments for check_acked().
>
> CC: Andre Przywara <[email protected]>
> Signed-off-by: Alexandru Elisei <[email protected]>
> ---
> arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 544c283f5f47..dcdab7d5f39a 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -62,41 +62,42 @@ static void stats_reset(void)
> }
> }
>
> -static void check_acked(const char *testname, cpumask_t *mask)
> +static void wait_for_interrupts(cpumask_t *mask)
> {
> - int missing = 0, extra = 0, unexpected = 0;
> int nr_pass, cpu, i;
> - bool bad = false;
>
> /* Wait up to 5s for all interrupts to be delivered */
> - for (i = 0; i < 50; ++i) {
> + for (i = 0; i < 50; i++) {
> mdelay(100);
> nr_pass = 0;
> for_each_present_cpu(cpu) {
> + /*
> + * A CPU having receied more than one interrupts will
received
> + * show up in check_acked(), and no matter how long we
> + * wait it cannot un-receive it. Consier at least one
consider
> + * interrupt as a pass.
> + */
> nr_pass += cpumask_test_cpu(cpu, mask) ?
> - acked[cpu] == 1 : acked[cpu] == 0;
> - smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> -
> - if (bad_sender[cpu] != -1) {
> - printf("cpu%d received IPI from wrong sender
> %d\n",
> - cpu, bad_sender[cpu]);
> - bad = true;
> - }
> -
> - if (bad_irq[cpu] != -1) {
> - printf("cpu%d received wrong irq %d\n",
> - cpu, bad_irq[cpu]);
> - bad = true;
> - }
> + acked[cpu] >= 1 : acked[cpu] == 0;
> }
> +
> if (nr_pass == nr_cpus) {
> - report(!bad, "%s", testname);
> if (i)
> - report_info("took more than %d ms", i * 100);
> + report_info("interrupts took more than %d ms",
> i * 100);
> + mdelay(100);
> return;
> }
> }
>
> + report_info("interrupts timed-out (5s)");
> +}
> +
> +static bool check_acked(cpumask_t *mask)
> +{
> + int missing = 0, extra = 0, unexpected = 0;
> + bool pass = true;
> + int cpu;
> +
> for_each_present_cpu(cpu) {
> if (cpumask_test_cpu(cpu, mask)) {
> if (!acked[cpu])
> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t
> *mask)
> if (acked[cpu])
> ++unexpected;
> }
> + smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> +
> + if (bad_sender[cpu] != -1) {
> + report_info("cpu%d received IPI from wrong sender %d",
> + cpu, bad_sender[cpu]);
> + pass = false;
> + }
> +
> + if (bad_irq[cpu] != -1) {
> + report_info("cpu%d received wrong irq %d",
> + cpu, bad_irq[cpu]);
> + pass = false;
> + }
> + }
> +
> + if (missing || extra || unexpected) {
> + report_info("ACKS: missing=%d extra=%d unexpected=%d",
> + missing, extra, unexpected);
> + pass = false;
> }
>
> - report(false, "%s", testname);
> - report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> - missing, extra, unexpected);
> + return pass;
> }
>
> static void check_spurious(void)
> @@ -300,7 +318,8 @@ static void ipi_test_self(void)
> cpumask_clear(&mask);
> cpumask_set_cpu(smp_processor_id(), &mask);
> gic->ipi.send_self();
> - check_acked("IPI: self", &mask);
> + wait_for_interrupts(&mask);
> + report(check_acked(&mask), "Interrupts received");
> report_prefix_pop();
> }
>
> @@ -315,7 +334,8 @@ static void ipi_test_smp(void)
> for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> cpumask_clear_cpu(i, &mask);
> gic_ipi_send_mask(IPI_IRQ, &mask);
> - check_acked("IPI: directed", &mask);
> + wait_for_interrupts(&mask);
> + report(check_acked(&mask), "Interrupts received");
both ipi_test_smp and ipi_test_self are called from the same test so
better to use different error messages like it was done originally.
> report_prefix_pop();
>
> report_prefix_push("broadcast");
> @@ -323,7 +343,8 @@ static void ipi_test_smp(void)
> cpumask_copy(&mask, &cpu_present_mask);
> cpumask_clear_cpu(smp_processor_id(), &mask);
> gic->ipi.send_broadcast();
> - check_acked("IPI: broadcast", &mask);
> + wait_for_interrupts(&mask);
> + report(check_acked(&mask), "Interrupts received");
> report_prefix_pop();
> }
>
>
Otherwise looks good to me
Thanks
Eric
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
