From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH 2/6] x86: add support for COS/CBM manangement Date: Tue, 17 Mar 2015 18:06:58 +0800 Message-ID: <20150317100658.GG5371@pengc-linux.bj.intel.com> References: <1426241605-4114-1-git-send-email-chao.p.peng@linux.intel.com> <1426241605-4114-3-git-send-email-chao.p.peng@linux.intel.com> <55071C98020000780006A70A@mail.emea.novell.com> <20150317091112.GD5371@pengc-linux.bj.intel.com> <5508012C020000780006A9BB@mail.emea.novell.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: <5508012C020000780006A9BB@mail.emea.novell.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: Jan Beulich Cc: wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, will.auld@intel.com, keir@xen.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Tue, Mar 17, 2015 at 09:25:48AM +0000, Jan Beulich wrote: > >>> On 17.03.15 at 10:11, wrote: > > On Mon, Mar 16, 2015 at 05:10:32PM +0000, Jan Beulich wrote: > >> >>> On 13.03.15 at 11:13, wrote: > >> > + else > >> > + { > >> > + unsigned int cpu = cpumask_check(get_socket_cpu(socket)); > >> > >> Isn't this going to trigger an assertion when the socket count got > >> specified on the command line? > > > > Yes, the assertion is needed in tools side anyway. Here the check seems > > unnecessary. > > No - if the get_socket_cpu() may return nr_cpu_ids, you need a > check. Just not in the form of an assertion. Right, error code can be returned. > > >> > + if( !d->arch.psr_cos_ids ) > >> > + return; > >> > >> Considering this check ... > >> > >> > + for ( socket = 0; socket < opt_socket_num; socket++) > >> > + { > >> > + cos = d->arch.psr_cos_ids[socket]; > >> > + if ( cos == 0 ) > >> > + continue; > >> > + > >> > + map = cat_socket_info[socket].cos_cbm_map; > >> > + if ( map ) > >> > + map[cos].ref--; > >> > + } > >> > + > >> > + xfree(d->arch.psr_cos_ids); > >> > >> ... I think you want to clear the pointer here. > > > > This function is called by arch_domain_destroy() or the failure path of > > arch_domain_create(). In both cases the domain structure will be freed > > later so setting NULL is pointless. > > But this is not visible from this function. The alternative to not > clearing the pointer here would be to do the checks of the > pointer in the callers, which then would make clear whether this > is really only on error and domain destruction paths (and that > hence the pointer can't be double freed). Then I'd like to clear the pointer here, so that even in the future the code will not surprise somebody. > > >> > @@ -222,6 +422,17 @@ static void do_cat_cpu_init(void* data) > >> > info->cbm_len = (eax & 0x1f) + 1; > >> > >> This means cbm_len <= 32. Why is cos_cbm_map[].cbm then a > >> uint64_t? > > > > Currently the cbm_len is EAX[4:0], 64 bits cbm here is for future > > possible enhancement. > > Unless the specification explicitly says that the field width in EAX > may be extended in the future, please omit such preparations > for possible future extensions - it is simply going to confuse > readers (as it did for me already) trying to understand why the > type of the field is what it is. I will try to understand this internally, if 32 bit is enough, then it will be used. Thanks, Chao