Re: [Xen-devel] [PATCH v7 08/12] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.09.17 at 17:31, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -33,6 +33,32 @@
>  
>  #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;
> +}
> +
> +static struct hvm_ioreq_server *get_ioreq_server(struct domain *d,

const (for the parameter)?

> +                                                 unsigned int id)
> +{
> +    if ( id >= MAX_NR_IOREQ_SERVERS )
> +        return NULL;
> +
> +    return d->arch.hvm_domain.ioreq_server.server[id];
> +}
> +
> +#define IS_DEFAULT(s) \
> +    ((s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))

Is it really useful to go through get_ioreq_server() here?

> +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
> +    for ( (id) = 0, (s) = get_ioreq_server((d), (id)); \
> +          (id) < MAX_NR_IOREQ_SERVERS; \
> +          (s) = get_ioreq_server((d), ++(id)) )

There are three instances of stray pairs of parentheses here, each
time when a macro parameter gets passed unaltered to
get_ioreq_server().

> @@ -301,8 +333,9 @@ static void hvm_update_ioreq_evtchn(struct 
> hvm_ioreq_server *s,
>      }
>  }
>  
> +
>  static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,

Stray addition of a blank line?

> @@ -501,19 +531,19 @@ static void hvm_ioreq_server_free_rangesets(struct 
> hvm_ioreq_server *s,
>  }
>  
>  static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
> -                                            bool is_default)
> +                                            ioservid_t id)
>  {
>      unsigned int i;
>      int rc;
>  
> -    if ( is_default )
> +    if ( IS_DEFAULT(s) )
>          goto done;

Wouldn't comparing the ID be even cheaper in cases like this one?
And perhaps assert that ID and server actually match?

> @@ -741,35 +754,34 @@ int hvm_destroy_ioreq_server(struct domain *d, 
> ioservid_t id)
>  
>      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>  
> -    rc = -ENOENT;
> -    list_for_each_entry ( s,
> -                          &d->arch.hvm_domain.ioreq_server.list,
> -                          list_entry )
> -    {
> -        if ( s == d->arch.hvm_domain.default_ioreq_server )
> -            continue;
> +    s = get_ioreq_server(d, id);
>  
> -        if ( s->id != id )
> -            continue;
> +    rc = -ENOENT;
> +    if ( !s )
> +        goto out;
>  
> -        domain_pause(d);
> +    rc = -EPERM;
> +    if ( IS_DEFAULT(s) )
> +        goto out;

Here I think it is particularly strange to not use the ID in the check;
this could even be done without holding the lock. Same in other
functions below.

> @@ -785,29 +797,27 @@ int hvm_get_ioreq_server_info(struct domain *d, 
> ioservid_t id,
>  
>      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>  
> -    rc = -ENOENT;
> -    list_for_each_entry ( s,
> -                          &d->arch.hvm_domain.ioreq_server.list,
> -                          list_entry )
> -    {
> -        if ( s == d->arch.hvm_domain.default_ioreq_server )
> -            continue;
> +    s = get_ioreq_server(d, id);
>  
> -        if ( s->id != id )
> -            continue;
> +    rc = -ENOENT;
> +    if ( !s )
> +        goto out;
>  
> -        *ioreq_gfn = s->ioreq.gfn;
> +    rc = -EOPNOTSUPP;
> +    if ( IS_DEFAULT(s) )
> +        goto out;

Why EOPNOTSUPP when it was just the same ENOENT as no
server at all before (same further down)?

>  void hvm_destroy_all_ioreq_servers(struct domain *d)
>  {
> -    struct hvm_ioreq_server *s, *next;
> +    struct hvm_ioreq_server *s;
> +    unsigned int id;
>  
>      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>  
>      /* No need to domain_pause() as the domain is being torn down */
>  
> -    list_for_each_entry_safe ( s,
> -                               next,
> -                               &d->arch.hvm_domain.ioreq_server.list,
> -                               list_entry )
> +    FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
> -        bool is_default = (s == d->arch.hvm_domain.default_ioreq_server);
> -
> -        hvm_ioreq_server_disable(s, is_default);
> -
> -        if ( is_default )
> -            d->arch.hvm_domain.default_ioreq_server = NULL;
> +        if ( !s )
> +            continue;
>  
> -        list_del(&s->list_entry);
> +        hvm_ioreq_server_disable(s);
> +        hvm_ioreq_server_deinit(s);
>  
> -        hvm_ioreq_server_deinit(s, is_default);
> +        ASSERT(d->arch.hvm_domain.ioreq_server.count);
> +        --d->arch.hvm_domain.ioreq_server.count;

Seeing this - do you actually need the count as a separate field?
I.e. are there performance critical uses of it, where going through
the array would be too expensive? Most of the uses are just
ASSERT()s anyway.

> @@ -1111,7 +1111,7 @@ int hvm_set_dm_domain(struct domain *d, domid_t domid)
>       * still be set and thus, when the server is created, it will have
>       * the correct domid.
>       */
> -    s = d->arch.hvm_domain.default_ioreq_server;
> +    s = get_ioreq_server(d, DEFAULT_IOSERVID);

Similar to above, is it really useful to go through get_ioreq_server()
here (and in other similar cases)?

Jan

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