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 14:04
> 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:41, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----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.
> 
> By "now" you mean with this patch in place as is, rather than the
> current tip of the tree? I don't see a regression compared to
> current staging, and I question the need to performance optimize
> a corner case a guest should not have much interest getting into.

I mean against current master, which has the following test at:

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/ioreq.c;hb=HEAD#l1168

    if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
        return NULL;

Without a count then the array would need to be searched, which is clearly 
slower than testing the emptiness a linked list.

> Anyway - I'm not meaning to block the patch because of the
> presence of this field (the more that you're the maintainer of this
> code anyway), it merely seems to me that the patch would be
> ending up smaller but no worse in quality if the field wasn't there.
> 

Agreed it's a corner case though so the regression is not of great concern. 
I'll get rid of the count for now... It can always be put back if it proves to 
be a real issue.

  Paul

> Jan


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