Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Julien Grall <julien@xen.org>]

On 10/08/2020 19:09, Oleksandr wrote:
> 
> On 05.08.20 12:32, Julien Grall wrote:
> 
> Hi Julien
> 
>>
>>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>>    */
>>>>   void leave_hypervisor_to_guest(void)
>>>>   {
>>>> +#ifdef CONFIG_IOREQ_SERVER
>>>> +    /*
>>>> +     * XXX: Check the return. Shall we call that in
>>>> +     * continue_running and context_switch instead?
>>>> +     * The benefits would be to avoid calling
>>>> +     * handle_hvm_io_completion on every return.
>>>> +     */
>>>
>>> Yeah, that could be a simple and good optimization
>>
>> Well, it is not simple as it is sounds :). handle_hvm_io_completion() 
>> is the function in charge to mark the vCPU as waiting for I/O. So we 
>> would at least need to split the function.
>>
>> I wrote this TODO because I wasn't sure about the complexity of 
>> handle_hvm_io_completion(current). Looking at it again, the main 
>> complexity is the looping over the IOREQ servers.
>>
>> I think it would be better to optimize handle_hvm_io_completion() 
>> rather than trying to hack the context_switch() or continue_running().
> Well, is the idea in proposed dirty test patch below close to what you 
> expect? Patch optimizes handle_hvm_io_completion() to avoid extra 
> actions if vcpu's domain doesn't have ioreq_server, alternatively
> the check could be moved out of handle_hvm_io_completion() to avoid 
> calling that function at all.

This looks ok to me.

> BTW, TODO also suggests checking the 
> return value of handle_hvm_io_completion(), but I am completely sure we 
> can simply
> just return from leave_hypervisor_to_guest() at this point. Could you 
> please share your opinion?

 From my understanding, handle_hvm_io_completion() may return false if 
there is pending I/O or a failure.

In the former case, I think we want to call handle_hvm_io_completion() 
later on. Possibly after we call do_softirq().

I am wondering whether check_for_vcpu_work() could return whether there 
are more work todo on the behalf of the vCPU.

So we could have:

do
{
   check_for_pcpu_work();
} while (check_for_vcpu_work())

The implementation of check_for_vcpu_work() would be:

if ( !handle_hvm_io_completion() )
   return true;

/* Rest of the existing code */

return false;

Cheers,

-- 
Julien Grall