Date:   Fri, 18 Feb 2022 13:09:11 +0100
From:   Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
To:     Shuah Khan <skhan@...uxfoundation.org>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>
Cc:     Thomas Huth <thuth@...hat.com>,
        David Hildenbrand <david@...hat.com>, kvm@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] KVM: s390: selftests: Refactor memop test

On 2/17/22 18:36, Shuah Khan wrote:
> On 2/17/22 7:53 AM, Janis Schoetterl-Glausch wrote:
>> Introduce macro for performing MEM_OP ioctls in a concise way.
> 
> How does this help? What is the value in re-writing existing
> code and turning it into a macro?

I want invocations of the ioctl to be independent of each other, so the reader does not
have to keep track of the state of the struct kvm_s390_mem_op.

So you have to specify all arguments manually like so, which is rather noisy and makes it
hard to see what the relevant parameter is:

ksmo.gaddr = guest_mem1;
ksmo.flags = 0;
ksmo.size = maxsize;
ksmo.op = KVM_S390_MEMOP_LOGICAL_WRITE;
ksmo.buf = (uintptr_t)mem1;
ksmo.ar = 17;
rv = _vcpu_ioctl(vm, VCPU_ID, KVM_S390_MEM_OP, &ksmo);

Or you introduce an abstraction.
Previously I used lots of functions with repeated code which got chaotic.
I decided on the macro because it's more flexible, e.g. you don't have to pass default args.
For example, there is only one test that passes the access register arg, so you would want
to default it to 0 for all other test.
For the access key argument you need to pass both a flag and the key itself, so you'd probably
get rid of this redundancy also.
There also might be future extensions of the ioctl that work the same way
(not 100% but not purely theoretical either).

With the macro all that is orthogonal, you just pass the argument you need or you don't.
With functions you'd maybe add a memop_key() variant and a _ar() variant and a _key_ar()
variant if you need it (currently not necessary), doubling the number of functions with
each additional argument. Another example is GADDR_V and GADDR, the first takes care of
translating the address to an physical one, but sometimes you need to pass it untranslated,
and we need to combine that with passing a key or not.

A big improvement was making the target of the ioctl (vm/vcpu) and the operation arguments
instead of baking it into the function. Since they're mandatory arguments this is independent
of the macro vs functions question.

In the end there are multiple independent but interacting improvements and it is kinda
hard to make the call on how far to go along one dimension, e.g. I was unsure if I
wanted to introduce the DEFAULT_READ macro, but decided for it, since, as a reviewer,
you can see that it executes the same code with different arguments, instead of trying
to identify the difference between 5 copy-pasted and modified lines of code. On the other
hand you have the cost of introducing an indirection.
> 
> 
>> Split test cases into multiple host/guest pairs making them independent.
> 
> This is a good change.
> 
>> Make various minor improvements.
>> All in all this lays the groundwork for future extensions.
> 
> Also good if these changes make it easier to add test. Would be helpful
> to know the details of the groundwork.

Yeah I'm not too happy about the commit descriptions.
I was unsure how to structure the patches, since the new test motivate
the restructuring, e.g. if I put the _test_errors_common in the first patch
it's kinda weird since at that stage there is no commonality at all.
I ended up moving stuff around + since I'm not quite sure about stuff like the DEFAULT_
macros I left the description kinda vague.

Probably should have linked to the series this is a reply to, since linux-kselftest wasn't on cc:
https://lore.kernel.org/kvm/20220211182215.2730017-1-scgl@linux.ibm.com/

> 
> thanks,
> -- Shuah

Powered by blists - more mailing lists