From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask Date: Wed, 7 Jan 2015 17:09:28 +0000 Message-ID: <54AD6848.90807@citrix.com> References: <1420629125-17725-1-git-send-email-chao.p.peng@linux.intel.com> <1420629125-17725-3-git-send-email-chao.p.peng@linux.intel.com> <54AD1825.1060800@citrix.com> <21677.24788.869267.896079@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21677.24788.869267.896079@mariner.uk.xensource.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: Ian Jackson Cc: wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com, Chao Peng , keir@xen.org List-Id: xen-devel@lists.xenproject.org On 07/01/15 16:37, Ian Jackson wrote: > Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask"): >> On 07/01/15 11:12, Chao Peng wrote: >>> +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask) >>> +{ >>> + static int val = 0; >> This should be uint32_t rather than int. >> >> I am somewhat concerned about multithreaded use of libxc, but this is >> not the first issue in libxc, and probably shouldn't be held against >> this patch. > On the contrary, this is quite bad. libxc should be properly > reentrant and I think it generally is. If you are aware of other > global variables of this kind please do ... > > ... I have just seen that the existing version of xc_psr.c has this > problem already. > > IMO this is a serious bug. Why was it made static before ? The idea is to caching a constant value from Xen. >>From a quick grep (and not very thorough): Other culprits are xc_get_max_nodes(), xc_get_max_cpus(), 4 instances in xc_psr.c and most things in xc_offline_page.c which appears to have static structures for domain context. The "pluggable loader" infrastructure in xc_dom.c also appears to be thread-unsafe. xc_dom_decompress_unsafe.c also uses static data, but "unsafe" in the name might be a sufficient guard? There are quite a few files which have static data structures which appear to be able to get away with being static const, and should probably move in that direction. > >> As the result of the hypercall is going to be the same, the >> worse that a race could achieve is a wasted hypercall. > This kind of analysis is unfounded in the presence of modern compilers > with aggressive optimisations. At the very least, if you're going to > do some caching like this, it needs a lock around it. No aggressively optimising compiler is going to perform partial writes on a naturally aligned integer, so I stand by my comment when applied to the common case. A dumb compiler on the other hand might, and C/POSIX make no guarantees in this regard, so the issue is indeed more serious than my initial analysis. ~Andrew