From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dulloor Subject: Re: [XEN][vNUMA][PATCH 3/9] public interface Date: Mon, 5 Jul 2010 22:57:55 -0700 Message-ID: References: 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" List-Id: xen-devel@lists.xenproject.org On Mon, Jul 5, 2010 at 3:23 AM, Keir Fraser wro= te: >> +#ifndef __XEN_PUBLIC_DOM_NUMA_X86_H__ >> +#define __XEN_PUBLIC_DOM_NUMA_X86_H__ >> + >> +/* struct xc_cpumask : static structure */ >> +#define XEN_CPUMASK_BITS_PER_BYTE 8 >> +#define XEN_CPUMASK_BITS_TO_BYTES(bits) \ >> + =A0 =A0(((bits)+XEN_CPUMASK_BITS_PER_BYTE-1)/XEN_CPUMASK_BITS_PER_BYTE= ) >> >> +#define XEN_MAX_VCPUS 128 >> +#define XEN_CPUMASK_DECLARE_BITMAP(name,bits) \ >> + =A0 =A0uint8_t name[XEN_CPUMASK_BITS_TO_BYTES(bits)] >> +struct xen_cpumask{ XEN_CPUMASK_DECLARE_BITMAP(bits, XEN_MAX_VCPUS); }; >> +#define XEN_CPUMASK_BITMAP(maskp) ((maskp)->bits) > > What are xc_cpumask (a libxc concept) related definitions doing in a > hypervisor public header? These aren't even used in this header file. Bel= ow > I suggest a vcpu_to_vnode[] array, which probably gets rid of the need fo= r > this bitmask stuff anyway. Stale comment with xc_cpumask .. sorry ! I did think of the vcpu_to_vnode array, but then we use the bitmask in hvm_= info anyway (with vcpu_online). I thought I could atleast fold them into a single structure. I could change that if you insist. > >> +#define XEN_MAX_VNODES 4 > > A small number to be statically defined. Better to make your structure > extensible I think, perhaps including pointers out to vnode-indexed array= s? This structure is passed in hvm_info page. Should I use offset/len for thes= e dynamic-sized, vnode-indexed arrays ? > >> +/* vnodes are 1GB-aligned */ >> +#define XEN_MIN_VNODE_SHIFT (30) >> + >> +struct xen_vnode_info { >> + =A0 =A0uint8_t vnode_id; >> + =A0 =A0uint8_t mnode_id; > > How do vnodes and mnodes differ? Why should a guest care about or need to > know about both, whatever they are? vnode_id is the node-id in the guest and mnode_id refers to the real node it maps to. Actually I don't need vnode_id. Will take that out. > >> + =A0 =A0uint32_t nr_pages; > > Not an address range? Is that implicitly worked out somehow? Should be > commented, but even better just a range explicitly given? The node address ranges are assumed contiguous and increasing. I will change that to ranges. > >> + =A0 =A0struct xen_cpumask vcpu_mask; /* vnode_to_vcpumask */ >> +}; > > Why not have a single integer array vcpu_to_vnode[] in the main > xen_domain_numa_info structure? No specific reason, except that all the vnode-related info is folded into a single structure. I will change that if you insist. > >> +#define XEN_DOM_NUMA_INTERFACE_VERSION =A00x01 >> + >> +#define XEN_DOM_NUMA_CONFINE =A0 =A00x01 >> +#define XEN_DOM_NUMA_SPLIT =A0 =A0 =A00x02 >> +#define XEN_DOM_NUMA_STRIPE =A0 =A0 0x03 >> +#define XEN_DOM_NUMA_DONTCARE =A0 0x04 > > What should the guest do with these? You're rather light on comments in t= his > critical interface-defining header file. I will add comments. The intent is to share this information with the hyper= visor and PV guests (for ballooning). > >> +struct xen_domain_numa_info { >> + =A0 =A0uint8_t version; >> + =A0 =A0uint8_t type; >> + >> + =A0 =A0uint8_t nr_vcpus; >> + =A0 =A0uint8_t nr_vnodes; >> + >> + =A0 =A0/* XXX: hvm_info_table uses 32-bit for high_mem_pgend, >> + =A0 =A0 * so we should be fine 32-bits too*/ >> + =A0 =A0uint32_t nr_pages; > > If this is going to be visible outside HVMloader (e.g., in PV guests) the= n > just make it a uint64_aligned_t and be done with it. Will do that. > >> + =A0 =A0/* Only (nr_vnodes) entries are filled */ >> + =A0 =A0struct xen_vnode_info vnode_info[XEN_MAX_VNODES]; >> + =A0 =A0/* Only (nr_vnodes*nr_vnodes) entries are filled */ >> + =A0 =A0uint8_t vnode_distance[XEN_MAX_VNODES*XEN_MAX_VNODES]; > > As suggested above, make these pointers out to dynamic-sized arrays. No n= eed > for XEN_MAX_VNODES at all. In general, I realise I should add more comments. > > =A0-- Keir > >> +}; >> + >> +#endif > > On 05/07/2010 09:52, "Dulloor" wrote: > >> oops .. sorry, here it is. >> >> -dulloor >> >> On Mon, Jul 5, 2010 at 12:39 AM, Keir Fraser >> wrote: >>> This patch is incomplete. >>> >>> >>> On 03/07/2010 00:54, "Dulloor" wrote: >>> >>>> Implement the structure that will be shared with hvmloader (with HVMs) >>>> and directly with the VMs (with PV). >>>> >>>> -dulloor >>>> >>>> Signed-off-by : Dulloor >>> >>> >>> > > >