Re: [Qemu-devel] [PATCH v3.1 00/31] NUMA series, and hostmem improvements

[Eduardo Habkost]

On Fri, May 09, 2014 at 04:29:49PM +0800, Hu Tao wrote:
> On Thu, May 08, 2014 at 04:51:56PM +0200, Paolo Bonzini wrote:
> > Il 06/05/2014 11:27, Hu Tao ha scritto:
> > > This series includes work on QOMifying the memory backends.
> > > the idea is to delegate all properties of the memory backend to
> > > a new QOM class hierarchy, in which the concrete classes
> > > are hostmem-ram and hostmem-file.  The backend is passed to the
> > > machine via "-numa node,memdev=foo" where "foo" is the id of the
> > > backend object.
> > 
> > Hello,
> > 
> > I noticed now that if you have the host-nodes property set Linux
> > requires you to set a policy other than "default" too.  If you don't,
> > the mbind system call fails.
> > 
> > What about squashing something like this?
> > 
> > Paolo
> > 
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index d3f8476..a0a3111 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -299,12 +299,23 @@ host_memory_backend_memory_init(UserCreatable *uc, 
> > Error **errp)
> >  
> >  #ifdef CONFIG_NUMA
> >      unsigned long maxnode = find_last_bit(backend->host_nodes, MAX_NODES);
> > +    unsigned policy = backend->policy;
> > +
> > +    /* Linux does not accept MPOL_DEFAULT with nonzero bitmap, but
> > +     * "-object memory-ram,size=128M,hostnodes=0,policy=bind" is a
> > +     * bit of a mouthful.  So if the host_nodes bitmap is nonzero,
> > +     * pick the BIND policy.

Are we sure MPOL_BIND is a better default than MPOL_INTERLEAVE or
MPOL_PREFERRED?


> > +     */
> > +    if (find_first_bit(backend->host_nodes, MAX_NODES) != MAX_NODES &&
> > +        policy == MPOL_DEFAULT) {
> > +        policy = MPOL_BIND;
> > +    }
> 
> This only changes the policy applied to memory. But info memdev still
> shows the policy as 'default'(the 'default' here is interpreted as
> 'MPOL_DEFAULT', right?).

That's what I would expect. With this, the user can't even check what is
the actual result of the command-line they used.

> 
> So this is related to how we deal with cases where 'policy' is not
> specified in qemu:
> 
> 1. if possible, the policy is set to MPOL_DEFAULT.
> 2. if host-nodes is not zero, the policy is set to MPOL_BIND.(set
>    backend->policy too, other than just apply it)

You mean, only if policy is omitted, right? host-nodes may be set if the
user wants policy=preferred or policy=interleave, too.

> 
> Opinions?

I find it confusing that an explicit policy=default option won't always
mean MPOL_DEFAULT.

So, what about:

 * If policy=default is set, it is always going to be MPOL_DEFAULT.
 * If policy=bind is set, it is always going to be MPOL_BIND.
 * if policy=preferred is set, it is always going to be MPOL_PREFERRED.
 * If policy is omitted, it will be be MPOL_DEFAULT is host-nodes is
   unset, and MPOL_BIND if host_nodes is set.

> 
> >  
> >      /* This is a workaround for a long standing bug in Linux'
> >       * mbind implementation, which cuts off the last specified
> >       * node.
> >       */
> > -    if (mbind(ptr, sz, backend->policy, backend->host_nodes, maxnode + 2, 
> > 0)) {
> > +    if (mbind(ptr, sz, policy, backend->host_nodes, maxnode + 2, 0)) {
> >          error_setg_errno(errp, errno,
> >                           "cannot bind memory to host NUMA nodes");
> 

-- 
Eduardo