Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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 ?

>  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.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel