Re: [Qemu-devel] [PULL 00/27] Migration pull

[Alexey Perevalov]

On 01/22/2018 01:03 PM, Peter Maydell wrote:
> On 20 January 2018 at 23:36, Juan Quintela <address@hidden> wrote:
> > Peter Maydell <address@hidden> wrote:
> > > On 19 January 2018 at 16:43, Alexey Perevalov <address@hidden> wrote:
> > > > As I remember, I tested build in QEMU's docker build system,
> > > > but now I checked it on i386 Ubuntu, and yes linker says about unresolved
> > > > atomic symbols. Next week, I'll have a time to investigate it deeper.
> > > This sounds like exactly the problem I pointed out in a previous
> > > round of this patchset :-(
> > >
> > > https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02103.html
> > >
> > > Ignoring comments and sending patches anyway makes me grumpy,
> > > especially when the result is exactly "fails obscurely on
> > > some architectures only"...
> > It compiles for me.  F25 i686 gcc.  I did change it to use intptr_t
> > instead of uint64_t.  So, I don't know what is going on here.
> Did you change it to not use the 'nocheck' versions of the macros?
> The code in master uses 'nocheck' which has exactly the effect
> of masking this bug on i686...
>
> > So, I can agree that we have to fix anything that don't work, but I
> > can't agree that I didn't care about comments, at least I tried to fix
> > the problems you pointed me to.
> I said the could should probably not use the nocheck macros. The
> code in master is still using those macros, wrongly, which is why
> this problem shows only on ppc32 and not all 32-bit hosts.
clang doesn't have atomic_*_8 function set on 32 platform,
but gcc has.

I also checked on ubuntu server 12.04, looks like no "ATOMIC_RELAXED" in
both gcc and clang toolchain, so

following code
atomic_xchg__nocheck(&a64, b64);
is expanding to
(({ asm volatile("" ::: "memory"); (void)0; }), 
__sync_lock_test_and_set(&a64, b64));
and we don't have problem neither in gcc nor in clang. Maybe it's not so 
effective
from performance point of view.

I like glib approach.
#if defined(__ATOMIC_SEQ_CST) && !defined(__clang__)
there __ATOMIC_SEQ_CST instead of __ATOMIC_RELAXED in QEMU, but it 
doesn't matter.
But clang has atomic_*_[1, 2, 4] functions, so it's not reasonable to 
avoid using clang
for all cases.

I want to keep 64bit atomic operations in migration.
Maybe add into atomic.h additional check for clang and 64bit pointer and 
in this case use
(({ asm volatile("" ::: "memory"); (void)0; }), 
__sync_lock_test_and_set(&a64, b64));
?


> thanks
> -- PMM

--
Best regards,
Alexey Perevalov