diff mbox series

[kvm-unit-tests,08/10] arm/arm64: gic: Split check_acked() into two functions

Message ID 20201125155113.192079-9-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series GIC fixes and improvements | expand

Commit Message

Alexandru Elisei Nov. 25, 2020, 3:51 p.m. UTC
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
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 <andre.przywara@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 26 deletions(-)

Comments

Auger Eric Dec. 3, 2020, 1:39 p.m. UTC | #1
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 <andre.przywara@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  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
Alexandru Elisei Dec. 10, 2020, 2:45 p.m. UTC | #2
Hi Eric,

On 12/3/20 1:39 PM, Auger Eric wrote:
>
> 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 <andre.przywara@arm.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  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

Will fix all three typos, thanks.

>> +			 * 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.

I used the same error message because the tests have a different prefix
("target-list" versus "broadcast"). Do you think there are cases where that's not
enough?

Thanks,
Alex
Auger Eric Dec. 15, 2020, 1:58 p.m. UTC | #3
Hi Alexandru,

On 12/10/20 3:45 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/3/20 1:39 PM, Auger Eric wrote:
>>
>> 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 <andre.przywara@arm.com>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>>  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
> 
> Will fix all three typos, thanks.
> 
>>> +			 * 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.
> 
> I used the same error message because the tests have a different prefix
> ("target-list" versus "broadcast"). Do you think there are cases where that's not
> enough?
I think in "ipi" test,
ipi_test -> ipi_send -> ipi_test_self, ipi_test_smp

Thanks

Eric
> 
> Thanks,
> Alex
>
Alexandru Elisei Dec. 16, 2020, 11:40 a.m. UTC | #4
Hi Eric,

On 12/15/20 1:58 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 12/10/20 3:45 PM, Alexandru Elisei wrote:
>> Hi Eric,
>>
>> On 12/3/20 1:39 PM, Auger Eric wrote:
>>> [..]
>>>  
>>>  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.
>> I used the same error message because the tests have a different prefix
>> ("target-list" versus "broadcast"). Do you think there are cases where that's not
>> enough?
> I think in "ipi" test,
> ipi_test -> ipi_send -> ipi_test_self, ipi_test_smp

I'm afraid I don't understand what you are trying to say. This is the log for the
gicv3-ipi test:

$ cat logs/gicv3-ipi.log
timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine
virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device
virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none
-serial stdio -kernel arm/gic.flat -smp 6 -machine gic-version=3 -append ipi #
-initrd /tmp/tmp.trk6aAcaZx
WARNING: early print support may not work. Found uart at 0x9000000, but early base
is 0x3f8.
PASS: gicv3: ipi: self: Interrupts received
PASS: gicv3: ipi: target-list: Interrupts received
PASS: gicv3: ipi: broadcast: Interrupts received
SUMMARY: 3 tests

The warning is because I forgot to reconfigure the tests with --vmm=qemu.

Would you mind pointing out what you think is ambiguous?

Thanks,

Alex
Auger Eric Dec. 16, 2020, 12:37 p.m. UTC | #5
Hi Alexandru,

On 12/16/20 12:40 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/15/20 1:58 PM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> On 12/10/20 3:45 PM, Alexandru Elisei wrote:
>>> Hi Eric,
>>>
>>> On 12/3/20 1:39 PM, Auger Eric wrote:
>>>> [..]
>>>>  
>>>>  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.
>>> I used the same error message because the tests have a different prefix
>>> ("target-list" versus "broadcast"). Do you think there are cases where that's not
>>> enough?
>> I think in "ipi" test,
>> ipi_test -> ipi_send -> ipi_test_self, ipi_test_smp
> 
> I'm afraid I don't understand what you are trying to say. This is the log for the
> gicv3-ipi test:
> 
> $ cat logs/gicv3-ipi.log
> timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine
> virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device
> virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none
> -serial stdio -kernel arm/gic.flat -smp 6 -machine gic-version=3 -append ipi #
> -initrd /tmp/tmp.trk6aAcaZx
> WARNING: early print support may not work. Found uart at 0x9000000, but early base
> is 0x3f8.
> PASS: gicv3: ipi: self: Interrupts received
> PASS: gicv3: ipi: target-list: Interrupts received
> PASS: gicv3: ipi: broadcast: Interrupts received
> SUMMARY: 3 tests
> 
> The warning is because I forgot to reconfigure the tests with --vmm=qemu.
> 
> Would you mind pointing out what you think is ambiguous?
Hum sorry I did not pay attention to the report_prefix_push() within
ipi_test_self & ipi_test_smp. I had in mind those were only in the main().

Forgive me for the noise

Eric
> 
> Thanks,
> 
> Alex
>
diff mbox series

Patch

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
+			 * show up in check_acked(), and no matter how long we
+			 * wait it cannot un-receive it. Consier at least one
+			 * 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");
 	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();
 }