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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 26 September 2017 12:45
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH v7 08/12] x86/hvm/ioreq: maintain an array of ioreq
> servers rather than a list
> 
> >>> On 26.09.17 at 12:55, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> Sent: 25 September 2017 16:17
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> >>> On 18.09.17 at 17:31, <paul.durrant@xxxxxxxxxx> wrote:
> >> > @@ -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)?
> >>
> >
> > This was because of comments from Roger. In some cases I think a return
> of
> > EOPNOTSUPP is more appropriate. Passing the default id is a distinct failure
> > case.
> 
> And I think the change is fine as long as the commit message makes
> clear it's an intentional change.
> 
> >> >  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.
> >
> > The specific case is in hvm_select_ioreq_server(). If there was no count
> > then the array would have to be searched for the initial test.
> 
> And is this something that happens frequently, i.e. the
> performance of which matters?

Yes, this is on the critical emulation path. I.e. it is a per-io call.

  Paul

> 
> Jan


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