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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Mar 17, 2020 at 10:35 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Ack.

>
> >>> @@ -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.

It grows the structure size to 64-bit, yes, but it doesn't grow the
size of union as other members are much larger. I'll remove the
underscore from the pad name but I still prefer it aligned.

Tamas

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