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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Sep 05, 2017 at 12:37:12PM +0100, 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.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

LGTM, just a couple of nitpicks, I think they can be fixed upon commit
if desired.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> 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         | 491 
> ++++++++++++++++++---------------------
>  xen/include/asm-x86/hvm/domain.h |  11 +-
>  2 files changed, 235 insertions(+), 267 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index f2e0b3f74a..287572bd1f 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -33,6 +33,22 @@
>  
>  #include <public/hvm/ioreq.h>
>  
> +#define SET_IOREQ_SERVER(d, id, s) \
> +    (d)->arch.hvm_domain.ioreq_server.server[id] = (s)

Are the parentheses around s required?

> +
> +#define GET_IOREQ_SERVER(d, id) \
> +    (((id) < MAX_NR_IOREQ_SERVERS) ? \
> +     (d)->arch.hvm_domain.ioreq_server.server[id] : \
> +     NULL)
> +
> +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
> +    for ( (id) = 0, (s) = GET_IOREQ_SERVER((d), (id)); \
> +          (id) < MAX_NR_IOREQ_SERVERS; \
> +          (id)++, (s) = GET_IOREQ_SERVER((d), (id)) )

Same here about the parentheses around s, d and id in the
GET_IOREQ_SERVER calls. In fact you could compact the afterthought as:

s = GET_IOREQ_SERVER(d, ++(id))

But that's just a nit.

>  int hvm_create_ioreq_server(struct domain *d, domid_t domid,
> @@ -685,52 +674,67 @@ int hvm_create_ioreq_server(struct domain *d, domid_t 
> domid,
>                              ioservid_t *id)
>  {
>      struct hvm_ioreq_server *s;
> +    unsigned int i;
>      int rc;
>  
>      if ( bufioreq_handling > HVM_IOREQSRV_BUFIOREQ_ATOMIC )
>          return -EINVAL;
>  
> -    rc = -ENOMEM;
>      s = xzalloc(struct hvm_ioreq_server);
>      if ( !s )
> -        goto fail1;
> +        return -ENOMEM;
>  
>      domain_pause(d);
>      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>  
> -    rc = -EEXIST;
> -    if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
> -        goto fail2;
> -
> -    rc = hvm_ioreq_server_init(s, d, domid, is_default, bufioreq_handling,
> -                               next_ioservid(d));
> -    if ( rc )
> -        goto fail3;
> -
> -    list_add(&s->list_entry,
> -             &d->arch.hvm_domain.ioreq_server.list);
> -
>      if ( is_default )
>      {
> -        d->arch.hvm_domain.default_ioreq_server = s;
> -        hvm_ioreq_server_enable(s, true);
> +        i = DEFAULT_IOSERVID;
> +
> +        rc = -EEXIST;
> +        if ( GET_IOREQ_SERVER(d, i) )
> +            goto fail;
> +    }
> +    else
> +    {
> +        for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
> +        {
> +            if ( i != DEFAULT_IOSERVID &&
> +                 !GET_IOREQ_SERVER(d, i) )

No need for the newline, the condition fits in a single line.

And since you have the macros, this could also be written in terms of
FOR_EACH_IOREQ_SERVER. Not really sure it's worth it anyway, you would
have to introduce another struct hvm_ioreq_server pointer here.

> +                break;
> +        }
> +
> +        rc = -ENOSPC;
> +        if ( i >= MAX_NR_IOREQ_SERVERS )
> +            goto fail;
>      }
>  
> +    SET_IOREQ_SERVER(d, i, s);
> +
> +    rc = hvm_ioreq_server_init(s, d, domid, bufioreq_handling, i);
> +    if ( rc )
> +        goto fail;
> +
> +    if ( IS_DEFAULT(s) )
> +        hvm_ioreq_server_enable(s);
> +
>      if ( id )
> -        *id = s->id;
> +        *id = i;
> +
> +    d->arch.hvm_domain.ioreq_server.count++;
>  
>      spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>      domain_unpause(d);
>  
>      return 0;
>  
> - fail3:
> - fail2:
> + fail:
> +    SET_IOREQ_SERVER(d, i, NULL);
> +
>      spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>      domain_unpause(d);
>  
>      xfree(s);
> - fail1:
>      return rc;
>  }
>  
> @@ -741,35 +745,30 @@ 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;
> -
> -        domain_pause(d);
> +    rc = -ENOENT;
> +    if ( !s || IS_DEFAULT(s) )
> +        goto out;

In the !s case ENOENT is fine, in the default case however EPERM might
be better? Or EOPNOTSUPP? Anyway, just a nit.

>  
> -        p2m_set_ioreq_server(d, 0, s);
> +    domain_pause(d);
>  
> -        hvm_ioreq_server_disable(s, false);
> +    p2m_set_ioreq_server(d, 0, s);
>  
> -        list_del(&s->list_entry);
> +    hvm_ioreq_server_disable(s);
> +    hvm_ioreq_server_deinit(s);
>  
> -        hvm_ioreq_server_deinit(s, false);
> +    domain_unpause(d);
>  
> -        domain_unpause(d);
> +    ASSERT(d->arch.hvm_domain.ioreq_server.count);
> +    --d->arch.hvm_domain.ioreq_server.count;
>  
> -        xfree(s);
> +    SET_IOREQ_SERVER(d, id, NULL);
> +    xfree(s);
>  
> -        rc = 0;
> -        break;
> -    }
> +    rc = 0;
>  
> + out:
>      spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>  
>      return rc;
> @@ -785,29 +784,23 @@ 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;
> -
> -        if ( s->id != id )
> -            continue;
> +    s = GET_IOREQ_SERVER(d, id);
>  
> -        *ioreq_gfn = s->ioreq.gfn;
> +    rc = -ENOENT;
> +    if ( !s || IS_DEFAULT(s) )

See above the comment about the ENOENT/EOPNOTSUPP thing.

Thanks, Roger.

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