Re: [Xen-devel] [PATCH v4 02/21] xen: make two memory hypercalls vNUMA-aware

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Jan 23, 2015 at 01:16:19PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 12:13, <wei.liu2@xxxxxxxxxx> wrote:
> > Make XENMEM_increase_reservation and XENMEM_populate_physmap
> > vNUMA-aware.
> > 
> > That is, if guest requests Xen to allocate memory for specific vnode,
> > Xen can translate vnode to pnode using vNUMA information of that guest.
> > 
> > XENMEMF_vnode is introduced for the guest to mark the node number is in
> > fact virtual node number and should be translated by Xen.
> > 
> > XENFEAT_memory_op_vnode_supported is introduced to indicate that Xen is
> > able to translate virtual node to physical node.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Acked-by: Jan Beulich <JBeulich@xxxxxxxx>
> 
> I'm afraid there's another change needed for this to hold:
> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -692,6 +692,50 @@ out:
> >      return rc;
> >  }
> >  
> > +static int translate_vnode_to_pnode(struct domain *d,
> > +                                    struct xen_memory_reservation *r,
> > +                                    struct memop_args *a)
> > +{
> > +    int rc = 0;
> > +    unsigned int vnode, pnode;
> > +
> > +    /*
> > +     * Note: we don't strictly require non-supported bits set to zero,
> > +     * so we may have exact_vnode bit set for old guests that don't
> > +     * support vNUMA.
> > +     *
> > +     * To distinguish spurious vnode request v.s. real one, check if
> > +     * d->vnuma exists.
> > +     */
> > +    if ( r->mem_flags & XENMEMF_vnode )
> > +    {
> > +        read_lock(&d->vnuma_rwlock);
> > +        if ( d->vnuma )
> 
> if r->mem_flags has XENMEMF_vnode set but d->vnuma is NULL,
> you need to clear the node from the flags.
> 

As said in the comment, we don't seem to enforce non-supported bits set
to zero (IIRC you told me that). So an old guest that sets XENMEMF_vnode
by accident will get its other flags cleared if I follow your suggestion.

> > +        {
> > +            vnode = XENMEMF_get_node(r->mem_flags);
> > +
> > +            if ( vnode < d->vnuma->nr_vnodes )
> > +            {
> > +                pnode = d->vnuma->vnode_to_pnode[vnode];
> > +
> > +                a->memflags &=
> > +                    ~MEMF_node(XENMEMF_get_node(r->mem_flags));
> 
> I.e. this one needs to be pulled up, and probably include
> MEMF_exact_node.
> 

> > @@ -747,6 +787,16 @@ long do_memory_op(unsigned long cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >              return start_extent;
> >          args.domain = d;
> >  
> > +        args.memflags |= 
> > MEMF_node(XENMEMF_get_node(reservation.mem_flags));
> > +        if ( reservation.mem_flags & XENMEMF_exact_node_request )
> > +            args.memflags |= MEMF_exact_node;
> 
> Since you force MEMF_exact_node when XENMEMF_vnode (and the
> necessary data is available), I also wonder whether the combination
> of both flags in what the caller requests should be forbidden (to
> potentially obtain some specific meaning in the future). Or
> alternatively don't enforce the former?
> 

We can't forbid guests from setting both flags, the reason is the same
as above. So don't enforce the MEMF_exact_node seems when XENMEMF_vnode
seems the way to go.

Wei.

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel