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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 10.08.20 22:00, 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.
It seems, yes


> 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;
Thank you, will give it a try.

Can we behave the same way for both "pending I/O" and "failure" or we 
need to distinguish them?
Probably we need some sort of safe timeout/number attempts in order to 
not spin forever?

--
Regards,

Oleksandr Tyshchenko