Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-20 Thread Hu Tao 
On Thu, Jul 19, 2012 at 08:15:28AM -0600, Eric Blake wrote:
 On 07/18/2012 07:07 PM, Hu Tao wrote:
  ...
  
  +
  +#  define virAtomicIntGet(atomic)   \
  +(__extension__ ({   \
  +verify (sizeof(*(atomic)) == sizeof(int));  \
  +(void) (0 ? *(atomic) ^ *(atomic) : 0); \
  +__sync_synchronize ();  \
  +(int) *(atomic);\
  +}))
 
  
  The `verify' lines cause building warnings:
  
  cc1: warnings being treated as errors
  util/virobject.c: In function 'virClassNew':
  util/virobject.c:74:99: error: nested extern declaration of 
  '_gl_verify_function2' [-Wnested-externs]
 
 Which version of gcc?
 
 I'll have to see if I can come up with a solution in upstream gnulib
 that expands verify() in such a way that works with -Wnested-externs.

$ gcc --version
gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4)
Copyright (C) 2010 Free Software Foundation, Inc.


This is the gcc shipped with Fedora 14.

-- 
Thanks,
Hu Tao

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-20 Thread Eric Blake 
On 07/20/2012 12:36 AM, Hu Tao wrote:
 The `verify' lines cause building warnings:

 cc1: warnings being treated as errors
 util/virobject.c: In function 'virClassNew':
 util/virobject.c:74:99: error: nested extern declaration of 
 '_gl_verify_function2' [-Wnested-externs]

 Which version of gcc?

 I'll have to see if I can come up with a solution in upstream gnulib
 that expands verify() in such a way that works with -Wnested-externs.
 
 $ gcc --version
 gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4)
 Copyright (C) 2010 Free Software Foundation, Inc.
 
 
 This is the gcc shipped with Fedora 14.

You _do_ realize that Fedora 14 is unsupported upstream (right now, the
oldest supported version is Fedora 16).  That said, I still have an F14
VM lying around, so I will try to reproduce this.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-20 Thread Eric Blake 
On 07/20/2012 12:36 AM, Hu Tao wrote:

 +#  define virAtomicIntGet(atomic)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ *(atomic) : 0); \
 +__sync_synchronize ();  \
 +(int) *(atomic);\
 +}))


 The `verify' lines cause building warnings:

 cc1: warnings being treated as errors
 util/virobject.c: In function 'virClassNew':
 util/virobject.c:74:99: error: nested extern declaration of 
 '_gl_verify_function2' [-Wnested-externs]

 Which version of gcc?

It turns out this is a problem with any gcc older than 4.6 (that is,
newer gcc with static_assert support no longer triggers this problematic
nested extern).  I was unable to come up with a way to avoid triggering
-Wnested-externs on older gcc without creating conflicting definitions.
 However, it's easy to work around in libvirt.  If -Wnested-externs is
in effect, then restrict the use of verify(...) to the top level
(outside of functions), and within functions, use this instead:

(void)verify_true(...)

The cast to void is unfortunately necessary to avoid a -Wall of
'statement with no effect'.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-20 Thread Eric Blake 
On 07/20/2012 04:26 PM, Eric Blake wrote:
 +#  define virAtomicIntGet(atomic)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \

 within functions, use this instead:
 
 (void)verify_true(...)

Or this, to give a better error message where possible:

(void) verify_expr (sizeof(*(atomic)) == sizeof(int), wrong size);

 
 The cast to void is unfortunately necessary to avoid a -Wall of
 'statement with no effect'.

Likewise true of verify_expr().

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-19 Thread Eric Blake 
On 07/18/2012 07:07 PM, Hu Tao wrote:
 ...
 
 +
 +#  define virAtomicIntGet(atomic)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ *(atomic) : 0); \
 +__sync_synchronize ();  \
 +(int) *(atomic);\
 +}))

 
 The `verify' lines cause building warnings:
 
 cc1: warnings being treated as errors
 util/virobject.c: In function 'virClassNew':
 util/virobject.c:74:99: error: nested extern declaration of 
 '_gl_verify_function2' [-Wnested-externs]

Which version of gcc?

I'll have to see if I can come up with a solution in upstream gnulib
that expands verify() in such a way that works with -Wnested-externs.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-18 Thread Hu Tao 
...

 +
 +#  define virAtomicIntGet(atomic)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ *(atomic) : 0); \
 +__sync_synchronize ();  \
 +(int) *(atomic);\
 +}))
 +#  define virAtomicIntSet(atomic, newval)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ (newval) : 0);  \
 +*(atomic) = (newval);   \
 +__sync_synchronize ();  \
 +}))
 +#  define virAtomicIntInc(atomic)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ *(atomic) : 0); \
 +(void) __sync_fetch_and_add ((atomic), 1);  \
 +}))
 +#  define virAtomicIntDecAndTest(atomic)\
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ *(atomic) : 0); \
 +__sync_fetch_and_sub ((atomic), 1) == 1;\
 +}))
 +#  define virAtomicIntCompareExchange(atomic, oldval, newval)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ (newval) ^ (oldval) : 0);   \
 +(bool) __sync_bool_compare_and_swap ((atomic), (oldval), 
 (newval)); \
 +   }))
 +#  define virAtomicIntAdd(atomic, val)  \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ (val) : 0); \
 +(int) __sync_fetch_and_add ((atomic), (val));   \
 +}))
 +#  define virAtomicIntAnd(atomic, val)  \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ (val) : 0); \
 +(unsigned int) __sync_fetch_and_and ((atomic), (val));  \
 +}))
 +#  define virAtomicIntOr(atomic, val)   \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \
 +(void) (0 ? *(atomic) ^ (val) : 0); \
 +(unsigned int) __sync_fetch_and_or ((atomic), (val));   \
 +}))
 +#  define virAtomicIntXor(atomic, val)  \
 +(__extension__ ({   \
 +verify (sizeof(*(atomic)) == sizeof(int));  \


The `verify' lines cause building warnings:

cc1: warnings being treated as errors
util/virobject.c: In function 'virClassNew':
util/virobject.c:74:99: error: nested extern declaration of 
'_gl_verify_function2' [-Wnested-externs]
util/virobject.c: In function 'virObjectNew':
util/virobject.c:112:84: error: nested extern declaration of 
'_gl_verify_function3' [-Wnested-externs]
util/virobject.c: In function 'virObjectUnref':
util/virobject.c:138:100: error: nested extern declaration of 
'_gl_verify_function4' [-Wnested-externs]
util/virobject.c: In function 'virObjectRef':
util/virobject.c:170:84: error: nested extern declaration of 
'_gl_verify_function5' [-Wnested-externs]


 +(void) (0 ? *(atomic) ^ (val) : 0); \
 +(unsigned int) __sync_fetch_and_xor ((atomic), (val));  \
 +}))
 +

-- 
Thanks,
Hu Tao

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-16 Thread Daniel P. Berrange 
On Fri, Jul 13, 2012 at 03:44:03PM -0600, Eric Blake wrote:
 On 07/11/2012 07:35 AM, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
  
  There are a few issues with the current virAtomic APIs
  
   - They require use of a virAtomicInt struct instead of a plain
 int type
   - Several of the methods do not implement memory barriers
   - The methods do not implement compiler re-ordering barriers
   - There is no Win32 native impl
  
  The GLib library has a nice LGPLv2+ licensed impl of atomic
  ops that works with GCC, Win32, or pthreads.h that addresses
  all these problems. The main downside to their code is that
  the pthreads impl uses a single global mutex, instead of
  a per-variable mutex. Given that it does have a Win32 impl
 
 Don't you mean 'gcc impl' here?

No, I meant Win32 impl - both codebases have a GCC impl - I was
pointing out that we gain a Win32 impl, which is the only non-GCC
scenario we hugely care about.

 
  though, we don't expect anyone to seriously use the pthread.h
  impl, so this downside is not significant.
 
 Agree that the majority of compilation is currently done with gcc, even
 though we try hard to allow other compilers.
 
  +++ b/configure.ac
  @@ -157,7 +157,64 @@ dnl Availability of various common headers (non-fatal 
  if missing).
   AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/un.h \
 sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \
 sys/un.h sys/syscall.h netinet/tcp.h ifaddrs.h libtasn1.h \
  -  net/if.h execinfo.h])
  +  net/if.h execinfo.h pthread.h])
 
 NACK to this change.  We already check for pthread.h via gnulib.

Hmm, I wonder why I didn't see HAVE_PTHREAD_H in config.h before. I'll
investigate.

 
  +
  +dnl We need to decide at configure time if libvirt will use real atomic
  +dnl operations (lock free) or emulated ones with a mutex.
  +
  +dnl Note that the atomic ops are only available with GCC on x86 when
  +dnl using -march=i486 or higher.  If we detect that the atomic ops are
  +dnl not available but would be available given the right flags, we want
  +dnl to abort and advise the user to fix their CFLAGS.  It's better to do
  +dnl that then to silently fall back on emulated atomic ops just because
  +dnl the user had the wrong build environment.
  +
  +atomic_ops=
  +
  +AC_MSG_CHECKING([for atomic ops implementation])
  +
  +AC_TRY_COMPILE([], [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;],[
  +  atomic_ops=gcc
  +],[])
  +
  +if test $atomic_ops =  ; then
  +  SAVE_CFLAGS=${CFLAGS}
  +  CFLAGS=-march=i486
  +  AC_TRY_COMPILE([],
  + [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;],
  + [AC_MSG_ERROR([Libvirt must be build with -march=i486 or 
  later.])],
  + [])
  +  CFLAGS=${SAVE_CFLAGS}
  +
  +  case $host in
  +*-*-mingw* | *-*-cygwin* | *-*-msvc* )
  +  atomic_ops=win32
 
 Okay for mingw and msvc, but looks wrong for cygwin.  Cygwin prides
 itself in having the same interface as Linux, and mucking with win32
 internals violates that.  But that means you are inheriting a glib bug,
 not something you are introducing yourself.  On the other hand, cygwin
 cannot be compiled unless you use gcc, so the extent of the glib bug is
 a dead arm of the case statement, and not something that will ever
 trigger in reality.  I suppose I can live with it, on the grounds that
 it was copy and paste; but I'd really like it if we isolated this into a
 separate file under m4/virt-...m4, and attributed it back to the glib
 sources, so that if glib enhances their tests, then we can more easily
 stay in sync with those enhancements.

Ok, I'll separate this out.

 
  @@ -714,7 +712,7 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req)
   
   virNWFilterSnoopLock();
   
  -if (virAtomicIntDec(req-refctr) == 0) {
  +if (virAtomicIntDecAndTest(req-refctr)) {
 
 The old code checked for 0, the new code checks for non-zero...
 
   /*
* delete the request:
* - if we don't find req on the global list anymore
  @@ -896,7 +894,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
   skip_instantiate:
   VIR_FREE(ipl);
   
  -virAtomicIntDec(virNWFilterSnoopState.nLeases);
  +virAtomicIntDecAndTest(virNWFilterSnoopState.nLeases);
 
 ...but here, the old and new code both test for non-zero.  Did you
 introduce a logic bug?

Yeah I think so.

 
  
  -if (virAtomicIntInc(virNWFilterSnoopState.wLeases) =
  -virAtomicIntRead(virNWFilterSnoopState.nLeases) * 20)
  +if (virAtomicIntAdd(virNWFilterSnoopState.wLeases, 1) =
  +virAtomicIntGet(virNWFilterSnoopState.nLeases) * 20)
 
 Did you mean to use virAtomicIntInc(virNWFilterSnoopState.wLeases)
 instead of virAtomicIntAdd(virNWFilterSnoopState.wLeases, 1)? ...

IntInc is void(), so I had to use  IntAdd. That said this code is
not too pleasant so I'll check into this.

 
  -# if ((__GNUC__ == 4)  (__GNUC_MINOR__ = 1)) || (__GNUC__  4)
  -#  undef

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-16 Thread Eric Blake 
On 07/16/2012 09:45 AM, Daniel P. Berrange wrote:
 On Fri, Jul 13, 2012 at 03:44:03PM -0600, Eric Blake wrote:
 On 07/11/2012 07:35 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com

 There are a few issues with the current virAtomic APIs


 +
 +#  define virAtomicIntGet(atomic)   \
 +virAtomicIntGet((int *)atomic)

 What are these macro casts for?  You are not properly parenthesizing
 things, and didn't the glib definition already check that the right size
 was being passed in?
 
 With the inline GCC macros we can pass either 'int' or 'unsigned int'
 with no warnings. When we use the WIn32/pthread helper functions though,
 we would get compiler warnings from mixed-sign ints. These macro casts
 get around that.

Fair enough, but a comment in the code before the defines would go a
long way to help the next reader.  Looking forward to v2.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/13] Rewrite virAtomic APIs using GLib's atomic ops code

2012-07-13 Thread Eric Blake 
On 07/11/2012 07:35 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 There are a few issues with the current virAtomic APIs
 
  - They require use of a virAtomicInt struct instead of a plain
int type
  - Several of the methods do not implement memory barriers
  - The methods do not implement compiler re-ordering barriers
  - There is no Win32 native impl
 
 The GLib library has a nice LGPLv2+ licensed impl of atomic
 ops that works with GCC, Win32, or pthreads.h that addresses
 all these problems. The main downside to their code is that
 the pthreads impl uses a single global mutex, instead of
 a per-variable mutex. Given that it does have a Win32 impl

Don't you mean 'gcc impl' here?

 though, we don't expect anyone to seriously use the pthread.h
 impl, so this downside is not significant.

Agree that the majority of compilation is currently done with gcc, even
though we try hard to allow other compilers.

 +++ b/configure.ac
 @@ -157,7 +157,64 @@ dnl Availability of various common headers (non-fatal if 
 missing).
  AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/un.h \
sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \
sys/un.h sys/syscall.h netinet/tcp.h ifaddrs.h libtasn1.h \
 -  net/if.h execinfo.h])
 +  net/if.h execinfo.h pthread.h])

NACK to this change.  We already check for pthread.h via gnulib.

 +
 +dnl We need to decide at configure time if libvirt will use real atomic
 +dnl operations (lock free) or emulated ones with a mutex.
 +
 +dnl Note that the atomic ops are only available with GCC on x86 when
 +dnl using -march=i486 or higher.  If we detect that the atomic ops are
 +dnl not available but would be available given the right flags, we want
 +dnl to abort and advise the user to fix their CFLAGS.  It's better to do
 +dnl that then to silently fall back on emulated atomic ops just because
 +dnl the user had the wrong build environment.
 +
 +atomic_ops=
 +
 +AC_MSG_CHECKING([for atomic ops implementation])
 +
 +AC_TRY_COMPILE([], [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;],[
 +  atomic_ops=gcc
 +],[])
 +
 +if test $atomic_ops =  ; then
 +  SAVE_CFLAGS=${CFLAGS}
 +  CFLAGS=-march=i486
 +  AC_TRY_COMPILE([],
 + [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;],
 + [AC_MSG_ERROR([Libvirt must be build with -march=i486 or 
 later.])],
 + [])
 +  CFLAGS=${SAVE_CFLAGS}
 +
 +  case $host in
 +*-*-mingw* | *-*-cygwin* | *-*-msvc* )
 +  atomic_ops=win32

Okay for mingw and msvc, but looks wrong for cygwin.  Cygwin prides
itself in having the same interface as Linux, and mucking with win32
internals violates that.  But that means you are inheriting a glib bug,
not something you are introducing yourself.  On the other hand, cygwin
cannot be compiled unless you use gcc, so the extent of the glib bug is
a dead arm of the case statement, and not something that will ever
trigger in reality.  I suppose I can live with it, on the grounds that
it was copy and paste; but I'd really like it if we isolated this into a
separate file under m4/virt-...m4, and attributed it back to the glib
sources, so that if glib enhances their tests, then we can more easily
stay in sync with those enhancements.

 @@ -714,7 +712,7 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req)
  
  virNWFilterSnoopLock();
  
 -if (virAtomicIntDec(req-refctr) == 0) {
 +if (virAtomicIntDecAndTest(req-refctr)) {

The old code checked for 0, the new code checks for non-zero...

  /*
   * delete the request:
   * - if we don't find req on the global list anymore
 @@ -896,7 +894,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
  skip_instantiate:
  VIR_FREE(ipl);
  
 -virAtomicIntDec(virNWFilterSnoopState.nLeases);
 +virAtomicIntDecAndTest(virNWFilterSnoopState.nLeases);

...but here, the old and new code both test for non-zero.  Did you
introduce a logic bug?

 
 -if (virAtomicIntInc(virNWFilterSnoopState.wLeases) =
 -virAtomicIntRead(virNWFilterSnoopState.nLeases) * 20)
 +if (virAtomicIntAdd(virNWFilterSnoopState.wLeases, 1) =
 +virAtomicIntGet(virNWFilterSnoopState.nLeases) * 20)

Did you mean to use virAtomicIntInc(virNWFilterSnoopState.wLeases)
instead of virAtomicIntAdd(virNWFilterSnoopState.wLeases, 1)? ...

 -# if ((__GNUC__ == 4)  (__GNUC_MINOR__ = 1)) || (__GNUC__  4)
 -#  undef __VIR_ATOMIC_USES_LOCK
 -# endif
 +/**
 + * virAtomicIntInc:
 + * Increments the value of atomic by 1.
 + *
 + * Think of this operation as an atomic version of { *atomic += 1; }
 + *
 + * This call acts as a full compiler and hardware memory barrier.
 + */
 +void virAtomicIntInc(volatile int *atomic)
 +ATTRIBUTE_NONNULL(1);

...Oh, I guess you can't, since this returns void instead of the old
value.  Then again, you have a subtle bug due to change of semantics.
The old virAtomicIntAdd returns the NEW value; the new virAtomicIntAdd
returns the OLD value.  Since you are adding one, you have