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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Mar 17, 2020 at 10:02 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 28.02.2020 19:40, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -509,6 +509,12 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, 
> > unsigned long gfn_l,
> >
> >      mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> >
> > +    /* Check if we need to fork the page */
> > +    if ( (q & P2M_ALLOC) && p2m_is_hole(*t) &&
> > +         !mem_sharing_fork_page(p2m->domain, gfn, !!(q & P2M_UNSHARE)) )
>
> No need for !! here.

I don't think it really matters but sure.

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

>
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1269,6 +1269,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, 
> > unsigned offset)
> >
> >      v->vcpu_info = new_info;
> >      v->vcpu_info_mfn = page_to_mfn(page);
> > +#ifdef CONFIG_MEM_SHARING
> > +    v->vcpu_info_offset = offset;
> > +#endif
>
> Seeing something like this makes me wonder whether forking shouldn't
> have its own Kconfig control.

For now I think it's fine to have it under mem_sharing.

>
> > --- 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?

>
> Also, does the build break if you made this an inline function
> (as we generally prefer)?

Any particular reason for that (inline vs define)?

>
> > @@ -141,6 +148,16 @@ static inline int mem_sharing_notify_enomem(struct 
> > domain *d, unsigned long gfn,
> >      return -EOPNOTSUPP;
> >  }
> >
> > +static inline int mem_sharing_fork(struct domain *d, struct domain *cd, 
> > bool vcpu)
> > +{
> > +    return -EOPNOTSUPP;
> > +}

This actually is no longer needed at all.

> > +
> > +static inline int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool 
> > lock)
> > +{
> > +    return -EOPNOTSUPP;
> > +}
>
> Can these be reached? If not, please add ASSERT_UNREACHABLE().

This can be reached.

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

>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -248,6 +248,9 @@ struct vcpu
> >
> >      /* Guest-specified relocation of vcpu_info. */
> >      mfn_t            vcpu_info_mfn;
> > +#ifdef CONFIG_MEM_SHARING
> > +    uint32_t         vcpu_info_offset;
>
> There's no need for a fixed width type here afaics - unsigned
> int and probably even unsigned short would seem to both be
> fine.

OK.

Thanks,
Tamas

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