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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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