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

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features
Date: Mon, 10 Aug 2020 23:29:26 +0300	[thread overview]
Message-ID: <48a130dc-57fd-0627-d188-ddff4abf6778@gmail.com> (raw)
In-Reply-To: <659a5b45-a37a-5cf2-0bcd-ba1ea3d47cc4@xen.org>

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