Re: [PATCH v3 1/2] x86: restore pv_rtc_handler() invocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.07.2020 18:27, Jan Beulich wrote:
> On 20.07.2020 17:28, Andrew Cooper wrote:
>> On 16/07/2020 11:06, Jan Beulich wrote:
>>> ACCESS_ONCE() guarantees single access, but doesn't guarantee that
>>> the compiler wouldn't split this single access into multiple insns.
>>
>> ACCESS_ONCE() does guarantee single accesses for any natural integer size.
>>
>> There is a section about this specifically in Linux's
>> memory-barriers.txt, and this isn't the first time I've pointed it out...
> 
> There indeed is text stating this, but I can't find any word on
> why they believe this is the case. My understanding of volatile
> is that it guarantees no more (and also no less) accesses to
> any single storage location than indicated by the source. But
> it doesn't prevent "tearing" of accesses. And really, how could
> it, considering that volatile can also be applied to types that
> aren't basic ones, and hence in particular to ones that can't
> possibly be accessed by a single insn?

To avoid a possible reference to *_ONCE() only accepting scalar
types - even the more explicit logic in the Linux constructs
permits "long long". Yet (I'm inclined to say of course) the
compiler makes no effort at all to carry out such a 64-bit
access as a single (atomic) insn on a 32-bit arch (i.e. cmpxchg8b
on ix86, if available). If there really was such a guarantee, it
surely would need to, or diagnose that it can't.

Furthermore I've looked at the current implementation of their
macros:

/*
 * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
 * atomicity or dependency ordering guarantees. Note that this may result
 * in tears!
 */
#define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))

#define __READ_ONCE_SCALAR(x)                                           \
({                                                                      \
        __unqual_scalar_typeof(x) __x = __READ_ONCE(x);                 \
        smp_read_barrier_depends();                                     \
        (typeof(x))__x;                                                 \
})

#define READ_ONCE(x)                                                    \
({                                                                      \
        compiletime_assert_rwonce_type(x);                              \
        __READ_ONCE_SCALAR(x);                                          \
})

The difference between __READ_ONCE() and READ_ONCE() effectively
is merely the smp_read_barrier_depends() afaics. Hence to me the
"tears" in the comment can only refer to "tear drops", not to
"torn accesses". The comment ahead of
compiletime_assert_rwonce_type() is also "interesting":

/*
 * Yes, this permits 64-bit accesses on 32-bit architectures. These will
 * actually be atomic in some cases (namely Armv7 + LPAE), but for others we
 * rely on the access being split into 2x32-bit accesses for a 32-bit quantity
 * (e.g. a virtual address) and a strong prevailing wind.
 */

(I'm struggling to see what extra effects this construct has over
the type enforcement by __unqual_scalar_typeof().)

Jan