Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2023 19:14, Tamas K Lengyel wrote:
>> @@ -1974,7 +2046,10 @@ int mem_sharing_fork_reset(struct domain
>>
>>   state:
>>      if ( reset_state )
>> +    {
>>          rc = copy_settings(d, pd);
>> +        /* TBD: What to do here with -ERESTART? */
> 
> Ideally we could avoid hitting code-paths that are restartable during fork
> reset since it gets called from vm_event replies that have no concept of
> handling errors. If we start having errors like this we would just have to
> drop the vm_event reply optimization and issue a standalone fork reset
> hypercall every time which isn't a big deal, it's just slower.

I'm afraid I don't follow: We are in the process of fork-reset here. How
would issuing "a standalone fork reset hypercall every time" make this
any different? The possible need for a continuation here comes from a
failed spin_trylock() in map_guest_area(). That won't change the next
time round.

But perhaps I should say that till now I didn't even pay much attention
to the 2nd use of the function by vm_event_resume(); I was mainly
focused on the one from XENMEM_sharing_op_fork_reset, where no
continuation handling exists. Yet perhaps your comment is mainly
related to that use?

I actually notice that the comment ahead of the function already has a
continuation related TODO, just that there thought is only of larger
memory footprint.

> My
> preference would actually be that after the initial forking is performed a
> local copy of the parent's state is maintained for the fork to reset to so
> there would be no case of hitting an error like this. It would also allow
> us in the future to unpause the parent for example..

Oh, I guess I didn't realize the parent was kept paused. Such state
copying / caching may then indeed be a possibility, but that's nothing
I can see myself deal with, even less so in the context of this series.
I need a solution to the problem at hand within the scope of what is
there right now (or based on what could be provided e.g. by you within
the foreseeable future). Bubbling up the need for continuation from the
XENMEM_sharing_op_fork_reset path is the most I could see me handle
myself ... For vm_event_resume() bubbling state up the domctl path
_may_ also be doable, but mem_sharing_notification() and friends don't
even check the function's return value.

Jan