[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v11 1/3] xen/mem_sharing: VM forking



On 17.03.2020 17:23, Tamas K Lengyel wrote:
> On Tue, Mar 17, 2020 at 10:02 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 28.02.2020 19:40, Tamas K Lengyel wrote:
>>> @@ -588,7 +594,8 @@ struct page_info *p2m_get_page_from_gfn(
>>>              return page;
>>>
>>>          /* Error path: not a suitable GFN at all */
>>> -        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) )
>>> +        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
>>> +             !mem_sharing_is_fork(p2m->domain) )
>>>              return NULL;
>>
>> This looks pretty broad a condition, i.e. all possible types would
>> make it through here for a fork. Wouldn't it make sense to limit
>> to to p2m_is_hole() page types, like you check for in
>> __get_gfn_type_access()?
> 
> No need to put that check here. By allowing to go further when we have
> a forked VM, this code-path will call get_gfn_type_access, which does
> that check. It's better to have that check at one place instead of all
> over unnecessarily.

Well, if worse performance (due to more cases where the lock will
be taken) is not of concern - so be it.

>>> --- a/xen/include/asm-x86/mem_sharing.h
>>> +++ b/xen/include/asm-x86/mem_sharing.h
>>> @@ -39,6 +39,9 @@ struct mem_sharing_domain
>>>
>>>  #define mem_sharing_enabled(d) ((d)->arch.hvm.mem_sharing.enabled)
>>>
>>> +#define mem_sharing_is_fork(d) \
>>> +    (mem_sharing_enabled(d) && !!((d)->parent))
>>
>> Again not need for !! (for a different reason).
> 
> Which is?

Further up the reason was that you pass the value as argument
for a boolean function parameter. Here the reason is that is an
operand of &&.

>> Also, does the build break if you made this an inline function
>> (as we generally prefer)?
> 
> Any particular reason for that (inline vs define)?

Inline functions add type safety for the arguments, which
#define-s don't do.

>>> @@ -532,6 +533,10 @@ struct xen_mem_sharing_op {
>>>                  uint32_t gref;     /* IN: gref to debug         */
>>>              } u;
>>>          } debug;
>>> +        struct mem_sharing_op_fork {      /* OP_FORK */
>>> +            domid_t parent_domain;        /* IN: parent's domain id */
>>> +            uint16_t _pad[3];             /* Must be set to 0 */
>>
>> Especially in the public interface - no new name space
>> violations please. I.e. please drop the leading underscore.
>> I also struggle to see why this is an array of three
>> elements. In fact I don't see why the padding field would be
>> needed at all - one other union member only gets padded to
>> its alignment (which is what I'd expect), while others
>> (presumably older ones) don't have any padding at all. Here
>> there's no implicit structure's alignment padding that wants
>> making explicit.
> 
> I don't know what you are asking. Drop the padding? I prefer each
> union member to be padded to 64-bit, reduces cognitive load trying to
> figure out what the size and alginment of each member struct would be.

Personally I'd suggest to drop the padding, as it actually
grows the size of the structure. But if you feel strongly
about keeping it, then I'll be okay with just the field's
name changed.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.