From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 2/6] x86: add support for COS/CBM manangement Date: Tue, 17 Mar 2015 09:25:48 +0000 Message-ID: <5508012C020000780006A9BB@mail.emea.novell.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150317091112.GD5371@pengc-linux.bj.intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chao Peng 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 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. >> > + 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). >> > @@ -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. Jan