Re: [Xen-devel] [PATCH RESEND 09/12] libxc: numa-sched: enable getting/specifying per-vcpu node-affinity

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mar, 2013-11-12 at 14:13 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 12, 2013 at 07:40:24PM +0100, Dario Faggioli wrote:

> > > > +int xc_vcpu_getnodeaffinity(xc_interface *xch,
> > > > +                            uint32_t domid,
> > > > +                            int vcpu,
> > > > +                            xc_nodemap_t nodemap)
> > > > +{
> > > > +    DECLARE_DOMCTL;
> > > > +    DECLARE_HYPERCALL_BUFFER(uint8_t, local);
> > > > +    int ret = -1;
> > > > +    int nodesize;
> > > > +
> > > > +    nodesize = xc_get_nodemap_size(xch);
> > > > +    if (!nodesize)
> > > > +    {
> > > > +        PERROR("Could not get number of nodes");
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    local = xc_hypercall_buffer_alloc(xch, local, nodesize);
> > > > +    if (local == NULL)
> > > > +    {
> > > > +        PERROR("Could not allocate memory for getvcpunodeaffinity 
> > > > domctl hypercall");
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    domctl.cmd = XEN_DOMCTL_getvcpunodeaffinity;
> > > > +    domctl.domain = (domid_t)domid;
> > > > +    domctl.u.vcpuaffinity.vcpu = vcpu;
> > > > +
> > > > +    set_xen_guest_handle(domctl.u.vcpuaffinity.map.bitmap, local);
> > > > +    domctl.u.vcpuaffinity.map.nr_bits = nodesize * 8;
> > > 
> > > Could the 8 be replaced by a sizeof?
> > >
> > I guess it could... What was it that you had in mind in particular?
> > 
> > Personally, I consider the names 'bitmap' and 'nr_bits' talking enough
> > to feel comfortable with the 8... To the point that I think the
> > following would be even less readable:
> > 
> >  domctl.u.vcpuaffinity.map.nr_bits = nodesize * 
> >         sizeof(domctl.u.vcpuaffinity.cpumap.bitmap);
> 
> Eww. That is worst. Somehow I assumed you could just do 'sizeof(unsigned long 
> long)'
> or such. or some #define for this magic number.
>
I did think more about this and (re)did the math. After that, I don't
think there is much we can do to make it more clear than it already is
with the 8, assuming that we all are cool with the fact that there are 8
bits in a byte, because that's what the 8 means there.

In fact, both the passed xc_nodemap_t (nodemap, in this case) and the
hypercall buffer (local) are allocated to be nodesize (i.e., the result
of xc_get_nodemap_size()) _bytes_. Since what we want to store in
nr_bits is the number of actual bit available in the resulting bitmap,
given how large it is in bytes, there is nothing much else I can think
of to get there than multiplying such number of byte by the number of
bits in a byte.

Therefore, either we find a suitable sizeof() for
"nr_of_bits_in_a_byte", or I "#define BITS_PER_BYTE 8" (:-P), or we
stick with the 8, which again I think it's the best option.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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