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

[Oleksandr <olekstysh@gmail.com>]

Hi Paul.


>>>>>> 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?
Err, yes, I wasn't precise when providing an example.
Single emulator is running in DomD and servicing DomU. So with the 
helper in place the vcpu_ioreq_handle_completion() gets only called for 
DomU vCPUs (as expected).
Without an optimization the vcpu_ioreq_handle_completion() gets called 
for _all_ vCPUs, and I see it as an extra action for Dom0, DomD vCPUs.


>
>> 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.

OK, is the implementation below close to what you propose? If yes, I 
will update a helper and drop nr_servers variable.

bool domain_has_ioreq_server(const struct domain *d)
{
     const struct ioreq_server *s;
     unsigned int id;

     FOR_EACH_IOREQ_SERVER(d, id, s)
         return true;

     return false;
}

-- 
Regards,

Oleksandr Tyshchenko