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 13:38
> 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 14:12, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 26 September 2017 12:45
> >> >>> 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
> >> >> >  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.
> 
> That's not answering the question, because you leave out implied
> context: Is it a performance critical path to get here with no
> ioreq server registers (i.e. count being zero)? After all if count is
> non-zero, you get past the early-out conditional there anyway.
> 

Clearly, only emulation that does not hit any IOREQ servers would be impacted. 
Generally you'd hope that the guest would not hit memory ranges that don't 
exist too often but, if it does, then by not maintaining a count there would be 
a small performance regression over the code as it stands now.

  Paul

> Jan


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