From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Auger Eric <eric.auger@redhat.com>,
kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
drjones@redhat.com
Cc: andre.przywara@arm.com
Subject: Re: [kvm-unit-tests PATCH 08/10] arm/arm64: gic: Split check_acked() into two functions
Date: Thu, 10 Dec 2020 14:45:37 +0000 [thread overview]
Message-ID: <2b8d774e-9398-e24b-6989-8643f5dd2492@arm.com> (raw)
In-Reply-To: <0eb98cb0-835c-e257-484e-8210f1279f2c@redhat.com>
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
next prev parent reply other threads:[~2020-12-10 15:15 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-25 15:51 [kvm-unit-tests PATCH 00/10] GIC fixes and improvements Alexandru Elisei
2020-11-25 15:51 ` [kvm-unit-tests PATCH 01/10] lib: arm/arm64: gicv3: Add missing barrier when sending IPIs Alexandru Elisei
2020-12-01 16:37 ` Auger Eric
2020-12-01 17:37 ` Alexandru Elisei
2020-11-25 15:51 ` [kvm-unit-tests PATCH 02/10] lib: arm/arm64: gicv2: " Alexandru Elisei
2020-12-01 16:37 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 03/10] arm/arm64: gic: Remove memory synchronization from ipi_clear_active_handler() Alexandru Elisei
2020-12-01 16:37 ` Auger Eric
2020-12-02 14:02 ` Alexandru Elisei
2020-12-02 14:14 ` Alexandru Elisei
2020-12-03 9:41 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 04/10] arm/arm64: gic: Remove unnecessary synchronization with stats_reset() Alexandru Elisei
2020-12-01 16:48 ` Auger Eric
2020-12-02 14:06 ` Alexandru Elisei
2020-12-03 13:10 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 05/10] arm/arm64: gic: Use correct memory ordering for the IPI test Alexandru Elisei
2020-12-03 13:10 ` Auger Eric
2020-12-03 13:21 ` Alexandru Elisei
2020-11-25 15:51 ` [kvm-unit-tests PATCH 06/10] arm/arm64: gic: Check spurious and bad_sender in the active test Alexandru Elisei
2020-12-03 13:10 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 07/10] arm/arm64: gic: Wait for writes to acked or spurious to complete Alexandru Elisei
2020-12-03 13:21 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 08/10] arm/arm64: gic: Split check_acked() into two functions Alexandru Elisei
2020-12-03 13:39 ` Auger Eric
2020-12-10 14:45 ` Alexandru Elisei [this message]
2020-12-15 13:58 ` Auger Eric
2020-12-16 11:40 ` Alexandru Elisei
2020-12-16 12:37 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 09/10] arm/arm64: gic: Make check_acked() more generic Alexandru Elisei
2020-12-03 14:59 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 10/10] arm64: gic: Use IPI test checking for the LPI tests Alexandru Elisei
2020-11-26 9:30 ` Zenghui Yu
2020-11-27 14:50 ` Alexandru Elisei
2020-11-30 13:59 ` Zenghui Yu
2020-11-30 14:19 ` Alexandru Elisei
2020-12-01 15:09 ` Alexandru Elisei
2020-11-30 17:48 ` Auger Eric
2020-12-03 14:59 ` Auger Eric
2020-12-09 10:29 ` Alexandru Elisei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2b8d774e-9398-e24b-6989-8643f5dd2492@arm.com \
--to=alexandru.elisei@arm.com \
--cc=andre.przywara@arm.com \
--cc=drjones@redhat.com \
--cc=eric.auger@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).