On Mon, 10 Aug 2020 at 21:29, Oleksandr <olekstysh@gmail.com> wrote:
>
>
> 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?
We don't need to distinguish them. In both cases, we will want to
process softirqs. In all the failure cases, the domain will have
crashed. Therefore the vCPU will be unscheduled.
>
> Probably we need some sort of safe timeout/number attempts in order to
> not spin forever?
Well, anything based on timeout/number of attempts is flaky. How do
you know whether the I/O is just taking a "long time" to complete?
But a vCPU shouldn't continue until an I/O has completed. This is
nothing very different than what a processor would do.
In Xen case, if an I/O never completes then it most likely means that
something went horribly wrong with the Device Emulator. So it is most
likely not safe to continue. In HW, when there is a device failure,
the OS may receive an SError (this is implementation defined) and
could act accordingly if it is able to recognize the issue.
It *might* be possible to send a virtual SError but there are a couple
of issues with it:
* How do you detect a failure?
* SErrors are implementations defined. You would need to teach
your OS (or the firmware) how to deal with them.
I would expect quite a bit of effort in order to design and implement
it. For now, it is probably best to just let the vCPU spin forever.
This wouldn't be an issue for Xen as do_softirq() would be called at
every loop.
Cheers,
>
>
> 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?
We don't need to distinguish them. In both cases, we will want to
process softirqs. In all the failure cases, the domain will have
crashed. Therefore the vCPU will be unscheduled.
>
> Probably we need some sort of safe timeout/number attempts in order to
> not spin forever?
Well, anything based on timeout/number of attempts is flaky. How do
you know whether the I/O is just taking a "long time" to complete?
But a vCPU shouldn't continue until an I/O has completed. This is
nothing very different than what a processor would do.
In Xen case, if an I/O never completes then it most likely means that
something went horribly wrong with the Device Emulator. So it is most
likely not safe to continue. In HW, when there is a device failure,
the OS may receive an SError (this is implementation defined) and
could act accordingly if it is able to recognize the issue.
It *might* be possible to send a virtual SError but there are a couple
of issues with it:
* How do you detect a failure?
* SErrors are implementations defined. You would need to teach
your OS (or the firmware) how to deal with them.
I would expect quite a bit of effort in order to design and implement
it. For now, it is probably best to just let the vCPU spin forever.
This wouldn't be an issue for Xen as do_softirq() would be called at
every loop.
Cheers,