Re: [kvm-unit-tests PATCH v2 2/2] s390x: firq: floating interrupt test

Thomas Huth
2021-12-06T07:12:20Z

On 03/12/2021 19.23, David Hildenbrand wrote:
>
>>>> + if (smp_query_num_cpus() < 3) {
>>>> + report_skip("need at least 3 CPUs for this test");
>>>> + goto out;
>>>> + }
>>>> +
>>>> + if (stap()) {
>>>> + report_skip("need to start on CPU #0");
>>>> + goto out;
>>>> + }
>>>
>>> I think I'd rather turn this into an assert() instead ... no strong opinion
>>> about it, though.
>>
>> I agree, including the part about no strong opinions (which is why I
>> did not comment on it before)
>
> Would it be the case on any system we might end up running, even under
> LPAR ... and whoever could run these tests ?

Well, ok, since it likely doesn't happen in real life anyway, simply keep
the report_skip().

>>
>>>
>>>> +
>>>> + /*
>>>> + * We want CPU #2 to be stopped. This should be the case at this
>>>> + * point, however, we want to sense if it even exists as well.
>>>> + */
>>>> + ret = smp_cpu_stop(2);
>>>> + if (ret) {
>>>> + report_skip("CPU #2 not found");
>>>
>>> Since you already queried for the availablity of at least 3 CPUs above, I
>>> think you could turn this into a report_fail() instead?
>>
>> either that or an assert, but again, no strong opinions
>>
>
> Just because there are >= 3 CPUs doesn't imply that CPU #2 is around.

Ok, fair point. But if #2 is not around, it means that the test has been run
in the wrong way by the user... I wonder what's better in that case - to
skip this test or to go out with a bang. Skipping the test has the advantage
of looking a little bit more "polite", but it has the disadvantage that it
might get lost in automation, e.g. if somebody enabled the test in their CI,
but did something wrong in the settings, they might not notice that the test
is not run at all...

> What we could remove is the "if (smp_query_num_cpus() < 3) {" check, though!

Yes, that seems to be redundant, indeed.

Thomas