From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH v3 4/8] x86: add support for COS/CBM manangement Date: Tue, 31 Mar 2015 16:40:57 +0800 Message-ID: <20150331084057.GA5816@pengc-linux.bj.intel.com> References: <1427373505-9303-1-git-send-email-chao.p.peng@linux.intel.com> <1427373505-9303-5-git-send-email-chao.p.peng@linux.intel.com> <5515BA8A.707@citrix.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <5515BA8A.707@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, will.auld@intel.com, JBeulich@suse.com, wei.liu2@citrix.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Fri, Mar 27, 2015 at 08:16:10PM +0000, Andrew Cooper wrote: > On 26/03/15 12:38, Chao Peng wrote: > > @@ -238,6 +450,8 @@ static void cat_cpu_init(unsigned int cpu) > > ASSERT(socket < nr_sockets); > > > > info = cat_socket_info + socket; > > + if ( info->socket_cpu_mask == NULL ) > > + info->socket_cpu_mask = per_cpu(cpu_core_mask, cpu); > > Surely this wants to be skipped if info is already initialised? The mask will be set to NULL in psr_cpu_fini() when the last CPU of the socket is hot un-plugged. If any CPU of the socket is plugged in again then the mask needs to be initialized but not skip. > > > > > /* Avoid initializing more than one times for the same socket. */ > > if ( test_and_set_bool(info->initialized) ) > > @@ -254,6 +468,14 @@ static void cat_cpu_init(unsigned int cpu) > > info->cbm_len = (eax & 0x1f) + 1; > > info->cos_max = (edx & 0xffff); > > > > + info->cos_cbm_map = xzalloc_array(struct psr_cat_cbm, > > + info->cos_max + 1UL); > > + if ( !info->cos_cbm_map ) > > + return; > > This indicates that cat_cpu_init() needs to be able to signal ENOMEM. The current behavior for ENOMEM case is: CAT is not enabled on the socket. Both cat_cpu_init() and the caller will not actually make use of this error code. > > > + > > + /* cos=0 is reserved as default cbm(all ones). */ > > + info->cos_cbm_map[0].cbm = (1ull << info->cbm_len) - 1; > > + > > info->enabled = 1; > > printk(XENLOG_DEBUG "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n", > > socket, info->cos_max, info->cbm_len); > > @@ -274,6 +496,24 @@ static void psr_cpu_init(unsigned int cpu) > > psr_assoc_init(); > > } > > > > +static void psr_cpu_fini(unsigned int cpu) > > +{ > > + unsigned int socket, next; > > + cpumask_t *cpu_mask; > > + > > + if ( cat_socket_info ) > > + { > > + socket = cpu_to_socket(cpu); > > + cpu_mask = cat_socket_info[socket].socket_cpu_mask; > > + > > + if ( (next = cpumask_cycle(cpu, cpu_mask)) == cpu ) > > + cat_socket_info[socket].socket_cpu_mask = NULL; > > + else > > + cat_socket_info[socket].socket_cpu_mask = > > + per_cpu(cpu_core_mask, next); > > + } > > +} > > + > Overall, this patch has a lot of moving parts in it. I have not spotted > any major problems, but I also don't feel confident that I understand > all of what is going on. > > It would certainly be easier to review if you split the patch into at > least 3; the core infrastructure, the domctl and the sysctl bits. Even > then, there appear to be several different bits of core changes going > on, with some per-socket infrastructure and per-domain infrastructure. OK, I will split it. > > The code itself appears to attempt to deal with sockets having a > different quantity of COS entries, but how does it resolve having a > different number of entries in the COS bitmaps? This would appear to > mean that a domain given a certain COS would exhibit different behaviour > depending on which socket it happened to be scheduled on. Hypervisor maintains per-socket COS bitmask for each domain as well. So a domain actually has COS bitmask for each socket but not only one COS bitmask. xen_domctl_psr_cat_op interface allows toolstack to set/get each COS bitmask on socket basis. Chao