Re: [PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2022 15:19, Tamas K Lengyel wrote:
> On Wed, Mar 30, 2022, 6:31 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 29.03.2022 16:03, Tamas K Lengyel wrote:
>>> --- a/xen/arch/x86/include/asm/mem_sharing.h
>>> +++ b/xen/arch/x86/include/asm/mem_sharing.h
>>> @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct
>> domain *d)
>>>  int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
>>>                            bool unsharing);
>>>
>>> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
>>> +                           bool reset_memory);
>>
>> Please avoid passing multiple booleans, even more so when you already
>> derive them from a single "flags" value. This would likely be easiest
>> if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
>> XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
>> prove they're the same.
> 
> I don't see why that would be an improvement in any way. I also don't want
> to make VM_EVENT flags tied to the XENMEM ones as they are separate
> interfaces. I rather just drop the changes to the XENMEM interface then do
> that.

If the function gained two or three more flags, how would that look to
you? And how would you easily identify the correct order of all the
booleans?

>>> @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct
>> vm_event_domain *ved)
>>>              if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
>>>                  p2m_mem_paging_resume(d, &rsp);
>>>  #endif
>>> +#ifdef CONFIG_MEM_SHARING
>>> +            if ( mem_sharing_is_fork(d) )
>>> +            {
>>> +                bool reset_state = rsp.flags &
>> VM_EVENT_FLAG_RESET_FORK_STATE;
>>> +                bool reset_mem = rsp.flags &
>> VM_EVENT_FLAG_RESET_FORK_MEMORY;
>>> +
>>> +                if ( reset_state || reset_mem )
>>> +                    ASSERT(!mem_sharing_fork_reset(d, reset_state,
>> reset_mem));
>>> +            }
>>> +#endif
>>
>> Should the two flags be rejected in the "else" case, rather than
>> being silently ignored?
> 
> What do you mean by rejected? There is no feasible "rejection" that could
> be done in this path other then skipping it.

IOW no return value being handed back to the requester? The function
does have an error return path, so I don't see why you couldn't return
-EINVAL.

Jan