[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH V1 11/16] xen/ioreq: Introduce hvm_domain_has_ioreq_server()



> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 16 September 2020 09:05
> To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>; Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Oleksandr Tyshchenko 
> <oleksandr_tyshchenko@xxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Volodymyr 
> Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Wei 
> Liu <wl@xxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: Re: [PATCH V1 11/16] xen/ioreq: Introduce 
> hvm_domain_has_ioreq_server()
> 
> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > This patch introduces a helper the main purpose of which is to check
> > if a domain is using IOREQ server(s).
> >
> > On Arm the benefit is to avoid calling handle_hvm_io_completion()
> > (which implies iterating over all possible IOREQ servers anyway)
> > on every return in leave_hypervisor_to_guest() if there is no active
> > servers for the particular domain.
> >

Is this really worth it? The limit on the number of ioreq serves is small... 
just 8. I doubt you'd be able measure the difference.

> > This involves adding an extra per-domain variable to store the count
> > of servers in use.
> 
> Since only Arm needs the variable (and the helper), perhaps both should
> be Arm-specific (which looks to be possible without overly much hassle)?
> 
> > --- a/xen/common/ioreq.c
> > +++ b/xen/common/ioreq.c
> > @@ -38,9 +38,15 @@ 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.ioreq_server.server[id]);
> > +    ASSERT((!s && d->arch.hvm.ioreq_server.server[id]) ||
> > +           (s && !d->arch.hvm.ioreq_server.server[id]));
> 
> For one, this can be had with less redundancy (and imo even improved
> clarity, but I guess this latter aspect my depend on personal
> preferences):
> 
>     ASSERT(d->arch.hvm.ioreq_server.server[id] ? !s : !!s);
> 
> But then I wonder whether the original intention wasn't rather such
> that replacing NULL by NULL is permissible. Paul?
> 

Yikes, that's a long time ago.. but I can't see why the check for !s would be 
there unless it was indeed intended to allow replacing NULL with NULL.

  Paul




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.