RE: [PATCH V3 17/23] xen/ioreq: Introduce domain_has_ioreq_server()

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr <olekstysh@xxxxxxxxx>
> Sent: 08 December 2020 20:17
> To: paul@xxxxxxx
> Cc: 'Jan Beulich' <jbeulich@xxxxxxxx>; 'Oleksandr Tyshchenko' 
> <oleksandr_tyshchenko@xxxxxxxx>;
> 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; 'Julien Grall' 
> <julien@xxxxxxx>; 'Volodymyr Babchuk'
> <Volodymyr_Babchuk@xxxxxxxx>; 'Andrew Cooper' <andrew.cooper3@xxxxxxxxxx>; 
> 'George Dunlap'
> <george.dunlap@xxxxxxxxxx>; 'Ian Jackson' <iwj@xxxxxxxxxxxxxx>; 'Wei Liu' 
> <wl@xxxxxxx>; 'Julien Grall'
> <julien.grall@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH V3 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
> 
> 
> On 08.12.20 21:43, Paul Durrant wrote:
> 
> Hi Paul
> 
> >> -----Original Message-----
> >> From: Oleksandr <olekstysh@xxxxxxxxx>
> >> Sent: 08 December 2020 16:57
> >> To: Paul Durrant <paul@xxxxxxx>
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Oleksandr Tyshchenko 
> >> <oleksandr_tyshchenko@xxxxxxxx>; Stefano
> >> Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; 
> >> Volodymyr Babchuk
> >> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; 
> >> George Dunlap
> >> <george.dunlap@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; Wei Liu 
> >> <wl@xxxxxxx>; Julien Grall
> >> <julien.grall@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH V3 17/23] xen/ioreq: Introduce 
> >> domain_has_ioreq_server()
> >>
> >>
> >> Hi Paul.
> >>
> >>
> >> On 08.12.20 17:33, Oleksandr wrote:
> >>> On 08.12.20 17:11, Jan Beulich wrote:
> >>>
> >>> Hi Jan
> >>>
> >>>> On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
> >>>>> --- a/xen/include/xen/ioreq.h
> >>>>> +++ b/xen/include/xen/ioreq.h
> >>>>> @@ -55,6 +55,20 @@ struct ioreq_server {
> >>>>>        uint8_t                bufioreq_handling;
> >>>>>    };
> >>>>>    +/*
> >>>>> + * This should only be used when d == current->domain and it's not
> >>>>> paused,
> >>>> Is the "not paused" part really relevant here? Besides it being rare
> >>>> that the current domain would be paused (if so, it's in the process
> >>>> of having all its vCPU-s scheduled out), does this matter at all?do
> >>>> any extra actionsdo any extra actions
> >>> No, it isn't relevant, I will drop it.
> >>>
> >>>
> >>>> Apart from this the patch looks okay to me, but I'm not sure it
> >>>> addresses Paul's concerns. Iirc he had suggested to switch back to
> >>>> a list if doing a swipe over the entire array is too expensive in
> >>>> this specific case.
> >>> We would like to avoid to do any extra actions in
> >>> leave_hypervisor_to_guest() if possible.
> >>> But not only there, the logic whether we check/set
> >>> mapcache_invalidation variable could be avoided if a domain doesn't
> >>> use IOREQ server...
> >>
> >> Are you OK with this patch (common part of it)?
> > How much of a performance benefit is this? The array is small to simply 
> > counting the non-NULL
> entries should be pretty quick.
> I didn't perform performance measurements on how much this call consumes.
> In our system we run three domains. The emulator is in DomD only, so I
> would like to avoid to call vcpu_ioreq_handle_completion() for every
> Dom0/DomU's vCPUs
> if there is no real need to do it.

This is not relevant to the domain that the emulator is running in; it's 
concerning the domains which the emulator is servicing. How many of those are 
there?

> On Arm vcpu_ioreq_handle_completion()
> is called with IRQ enabled, so the call is accompanied with
> corresponding irq_enable/irq_disable.
> These unneeded actions could be avoided by using this simple one-line
> helper...
> 

The helper may be one line but there is more to the patch than that. I still 
think you could just walk the array in the helper rather than keeping a running 
occupancy count.

  Paul