Re: [Xen-devel] [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/10/17 11:16, Jan Beulich wrote:
>>>> On 29.09.17 at 17:38, <Paul.Durrant@xxxxxxxxxx> wrote:
>>>  -----Original Message-----
>>> From: Andrew Cooper
>>> Sent: 29 September 2017 16:35
>>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx 
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>> Subject: Re: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
>>> servers rather than a list
>>>
>>> On 29/09/17 15:51, Paul Durrant wrote:
>>>> A subsequent patch will remove the current implicit limitation on creation
>>>> of ioreq servers which is due to the allocation of gfns for the ioreq
>>>> structures and buffered ioreq ring.
>>>>
>>>> It will therefore be necessary to introduce an explicit limit and, since
>>>> this limit should be small, it simplifies the code to maintain an array of
>>>> that size rather than using a list.
>>>>
>>>> Also, by reserving an array slot for the default server and populating
>>>> array slots early in create, the need to pass an 'is_default' boolean
>>>> to sub-functions can be avoided.
>>>>
>>>> Some function return values are changed by this patch: Specifically, in
>>>> the case where the id of the default ioreq server is passed in, -
>>> EOPNOTSUPP
>>>> is now returned rather than -ENOENT.
>>>>
>>>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>>> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>> ---
>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>
>>>> v8:
>>>>  - Addressed various comments from Jan.
>>>>
>>>> v7:
>>>>  - Fixed assertion failure found in testing.
>>>>
>>>> v6:
>>>>  - Updated according to comments made by Roger on v4 that I'd missed.
>>>>
>>>> v5:
>>>>  - Switched GET/SET_IOREQ_SERVER() macros to get/set_ioreq_server()
>>>>    functions to avoid possible double-evaluation issues.
>>>>
>>>> v4:
>>>>  - Introduced more helper macros and relocated them to the top of the
>>>>    code.
>>>>
>>>> v3:
>>>>  - New patch (replacing "move is_default into struct hvm_ioreq_server") in
>>>>    response to review comments.
>>>> ---
>>>>  xen/arch/x86/hvm/ioreq.c         | 525 ++++++++++++++++++++--------------
>>> -----
>>>>  xen/include/asm-x86/hvm/domain.h |  10 +-
>>>>  2 files changed, 270 insertions(+), 265 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>>>> index f2e0b3f74a..e655d2eab3 100644
>>>> --- a/xen/arch/x86/hvm/ioreq.c
>>>> +++ b/xen/arch/x86/hvm/ioreq.c
>>>> @@ -33,6 +33,41 @@
>>>>
>>>>  #include <public/hvm/ioreq.h>
>>>>
>>>> +static void set_ioreq_server(struct domain *d, unsigned int id,
>>>> +                             struct hvm_ioreq_server *s)
>>>> +{
>>>> +    ASSERT(id < MAX_NR_IOREQ_SERVERS);
>>>> +    ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]);
>>>> +
>>>> +    d->arch.hvm_domain.ioreq_server.server[id] = s;
>>>> +}
>>>> +
>>>> +#define GET_IOREQ_SERVER(d, id) \
>>>> +    (d)->arch.hvm_domain.ioreq_server.server[id]
>>>> +
>>>> +static struct hvm_ioreq_server *get_ioreq_server(const struct domain
>>> *d,
>>>> +                                                 unsigned int id)
>>>> +{
>>>> +    if ( id >= MAX_NR_IOREQ_SERVERS )
>>>> +        return NULL;
>>>> +
>>>> +    return GET_IOREQ_SERVER(d, id);
>>>> +}
>>>> +
>>>> +#define IS_DEFAULT(s) \
>>>> +    ((s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))
>>>> +
>>>> +/*
>>>> + * Iterate over all possible ioreq servers. The use of inline function
>>>> + * get_ioreq_server() in the increment is deliberate as use of the
>>>> + * GET_IOREQ_SERVER() macro will cause gcc to complain about an array
>>>> + * overflow.
>>>> + */
>>>> +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
>>>> +    for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
>>>> +          (id) < MAX_NR_IOREQ_SERVERS; \
>>>> +          (s) = get_ioreq_server(d, ++(id)) )
>>> I'm guessing from the various constructs, the list of ioreq servers
>>> might have embedded NULLs in the middle?
>>>
>>> If so, how about this?
>>>
>>> #define FOR_EACH_IOREQ_SERVER(d, id, s) \
>>>     for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
>>>           (id) < MAX_NR_IOREQ_SERVERS; \
>>>           (s) = get_ioreq_server(d, ++(id)) ) \
>>>     if ( !s ) \
>>>         continue; \
>>>     else
>> I'm ok with it but I'll wait for others opinion on whether this is taking 
>> the macro magic too far.
> I'm fine with Andrew's suggestion; I'm surprised though trickery
> like this is being suggested at all, as commonly games I happen
> to be trying to play once in a while don't seem to be really liked.

I don't specifically recall a previous example.  Either way, tricks like
this do need to be evaluated very carefully, based on how much they help
the code vs how much they hide.

In this case, the FOR_EACH doesn't imply sequential access over an
array, and code inside the loop which uses a break rather than a
continue will definitely cause subtle bugs.  Therefore IMO, its benefits
outweigh the costs.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel