From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dulloor Subject: Re: [PATCH] libxc bitmap utils and vcpu-affinity Date: Tue, 30 Mar 2010 12:05:37 -0400 Message-ID: <940bcfd21003300905y78754e2cu101055e429a303a1@mail.gmail.com> References: <940bcfd21003300742s28db15dbm9bf3316f8625f180@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "xen-devel@lists.xensource.com" , Jan Beulich List-Id: xen-devel@lists.xenproject.org > No changeset comment. No signed-off-by line. Sorry, I forgot. Will do once we finalize on the patch. > It actually bloats the libraries by a net 650 LOC > (747 added, 87 deleted according to diffstat). In the patch, we have used the library only for vcpu get/set affinity. There are clearly other opportunities (right now and in future) to use most of the functions provided by the library, which will offset this. Also, this provides a cleaner/standard way of using the cpumap structure in libxc. > And below I append the very first function I read: it doesn't inspire > confidence that the implementation is over-complicated/long and > unnecessarily handles 16-bit values. I guess you mean 8-bit values. The library works with byte-based bitmap structures, instead of "uint32_t or uint64_t" bitmap structures, so that we don't need to convert when using the bitmap-library with xenctl_cpumap. Please let know if you would rather that we keep the bitmap utilities separate from xenctl_cpumap and provide for conversion functions. That would be a small change. The function that you quote is inspired from linux kernel implementation (as mentioned) and is a simple generic function. And I have tested the library thoroughly. > Why should I show your patch some love? We can work towards that ;) -dulloor On Tue, Mar 30, 2010 at 11:16 AM, Keir Fraser w= rote: > No changeset comment. No signed-off-by line. It actually bloats the > libraries by a net 650 LOC (747 added, 87 deleted according to diffstat). > And below I append the very first function I read: it doesn't inspire > confidence that the implementation is over-complicated/long and > unnecessarily handles 16-bit values. Why should I show your patch some lo= ve? > > =A0-- Keir > > +static inline int __xc_ffs(uint8_t byte) > +{ > + =A0 =A0 =A0 int num =3D 0; > + > + =A0 =A0 =A0 if ((byte & 0xff) =3D=3D 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 num +=3D 8; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 byte >>=3D 8; > + =A0 =A0 =A0 } > + =A0 =A0 =A0 if ((byte & 0xf) =3D=3D 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 num +=3D 4; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 byte >>=3D 4; > + =A0 =A0 =A0 } > + =A0 =A0 =A0 if ((byte & 0x3) =3D=3D 0) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 num +=3D 2; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 byte >>=3D 2; > + =A0 =A0 =A0 } > + =A0 =A0 =A0 if ((byte & 0x1) =3D=3D 0) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 num +=3D 1; > + =A0 =A0 =A0 return num; > +} > > On 30/03/2010 15:42, "Dulloor" wrote: > >> Resubmitting the patch. >> >> -dulloor >> >> ---------- Forwarded message ---------- >> From: Dulloor >> Date: Tue, Mar 23, 2010 at 12:55 PM >> Subject: Re: [Xen-devel][PATCH] libxc bitmap utils and vcpu-affinity >> To: Keir Fraser >> Cc: Jan Beulich , "xen-devel@lists.xensource.com" >> >> >> >> Please use this patch, in which length of bitmap is >> (physinfo.max_cpu_id+1), rather than (physinfo.nr_cpus). >> >> -dulloor >> >> On Tue, Mar 23, 2010 at 12:41 PM, Dulloor wrote: >>> I meant utils for **xenctl_cpumap** >>> >>> On Tue, Mar 23, 2010 at 12:40 PM, Dulloor wrote: >>>> Fine, I agree with you both. Attached is a patch adding utils for >>>> xenctl_bitmap (to libxc) and using the same in vcpu_(get|set)affinity. >>>> For the guest-numa interface, I will see if I can use xenctl_cpumap. >>>> >>>> -dulloor >>>> >>>> On Tue, Mar 23, 2010 at 7:05 AM, Keir Fraser >>>> wrote: >>>>> On 23/03/2010 10:10, "Jan Beulich" wrote: >>>>> >>>>>>>>> Dulloor 22.03.10 18:44 >>> >>>>>>> Motivation for using xenctl_cpumask in Xen interfaces : >>>>>>> - xenctl_cpumap is just 4 bytes smaller than static xenctl_cpumask = for >>>>>>> 128 cpus (128 would be good for quite some time). However, the new >>>>>> >>>>>> I don't buy this (we're already building for 256 CPUs, looking forwa= rd >>>>>> to further bump this in the not too distant future), and I'm general= ly >>>>>> opposed to introducing hard coded limits in a public interface. >>>>> >>>>> We should use xenctl_cpumask everywhere for specifying physical CPU >>>>> bitmaps, >>>>> even into guest NUMA interfaces if appropriate. I don't really care i= f it >>>>> is >>>>> a bit harder to use than a static bitmap. >>>>> >>>>> =A0-- Keir >>>>> >>>>> >>>>> >>>> >>> > > >