Libguestfs - Nov 2019 - Re: [PATCH nbdkit] server: Use GCC hints to move debug and error handling code out of hot paths.

For GCC only, define unlikely() macro.  Use it on error paths to move
code out of the hot path.

In the server only, use the debug() macro (don't call nbdkit_debug
directly).  This macro checks the verbose flag and moves the call to
nbdkit_debug out of the hot path.
---
 server/connections.c | 11 ++++++-----
 server/internal.h    | 17 ++++++++++++++++-
 server/plugins.c     |  2 +-
 server/protocol.c    |  4 ++--
 server/sockets.c     |  4 ++--
 5 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/server/connections.c b/server/connections.c
index c55d381..95d8296 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -97,7 +97,7 @@ connection_set_status (struct connection *conn, int value)
 
       assert (conn->status_pipe[1] >= 0);
       if (write (conn->status_pipe[1], &c, 1) != 1 && errno !=
EAGAIN)
-        nbdkit_debug ("failed to notify pipe-to-self: %m");
+        debug ("failed to notify pipe-to-self: %m");
     }
     conn->status = value;
   }
@@ -176,7 +176,7 @@ _handle_single_connection (int sockin, int sockout)
     debug ("handshake complete, processing requests with %d threads",
            nworkers);
     workers = calloc (nworkers, sizeof *workers);
-    if (!workers) {
+    if (unlikely (!workers)) {
       perror ("malloc");
       goto done;
     }
@@ -185,12 +185,13 @@ _handle_single_connection (int sockin, int sockout)
       struct worker_data *worker = malloc (sizeof *worker);
       int err;
 
-      if (!worker) {
+      if (unlikely (!worker)) {
         perror ("malloc");
         connection_set_status (conn, -1);
         goto wait;
       }
-      if (asprintf (&worker->name, "%s.%d", plugin_name,
nworkers) < 0) {
+      if (unlikely (asprintf (&worker->name, "%s.%d",
plugin_name, nworkers)
+                    < 0)) {
         perror ("asprintf");
         connection_set_status (conn, -1);
         free (worker);
@@ -199,7 +200,7 @@ _handle_single_connection (int sockin, int sockout)
       worker->conn = conn;
       err = pthread_create (&workers[nworkers], NULL, connection_worker,
                             worker);
-      if (err) {
+      if (unlikely (err)) {
         errno = err;
         perror ("pthread_create");
         connection_set_status (conn, -1);
diff --git a/server/internal.h b/server/internal.h
index 5e11e1a..7e0c375 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -45,6 +45,17 @@
 #include "cleanup.h"
 #include "nbd-protocol.h"
 
+/* Define unlikely macro, but only for GCC.  These are used to move
+ * debug and error handling code out of hot paths.
+ */
+#if defined(__GNUC__)
+#define unlikely(x) __builtin_expect (!!(x), 0)
+#define if_verbose if (unlikely (verbose))
+#else
+#define unlikely(x) (x)
+#define if_verbose if (verbose)
+#endif
+
 #ifdef __APPLE__
 #define UNIX_PATH_MAX 104
 #else
@@ -262,7 +273,11 @@ extern int crypto_negotiate_tls (struct connection *conn,
   __attribute__((__nonnull__ (1)));
 
 /* debug.c */
-#define debug nbdkit_debug
+#define debug(fs, ...)                                   \
+  do {                                                   \
+    if_verbose                                           \
+      nbdkit_debug ((fs), ##__VA_ARGS__);                \
+  } while (0)
 
 /* log-*.c */
 #if !HAVE_VFPRINTF_PERCENT_M
diff --git a/server/plugins.c b/server/plugins.c
index 87daaf2..65f6817 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -71,7 +71,7 @@ plugin_thread_model (struct backend *b)
 #if !(defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined
HAVE_PIPE2 && \
       defined HAVE_ACCEPT4)
   if (thread_model > NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) {
-    nbdkit_debug ("system lacks atomic CLOEXEC, serializing to avoid fd
leaks");
+    debug ("system lacks atomic CLOEXEC, serializing to avoid fd
leaks");
     thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
   }
 #endif
diff --git a/server/protocol.c b/server/protocol.c
index 89fbdfa..f6ea35c 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -508,8 +508,8 @@ extents_to_block_descriptors (struct nbdkit_extents
*extents,
 
 #if 0
   for (i = 0; i < *nr_blocks; ++i)
-    nbdkit_debug ("block status: sending block %" PRIu32 " type
%" PRIu32,
-                  blocks[i].length, blocks[i].status_flags);
+    debug ("block status: sending block %" PRIu32 " type %"
PRIu32,
+           blocks[i].length, blocks[i].status_flags);
 #endif
 
   /* Convert to big endian for the protocol. */
diff --git a/server/sockets.c b/server/sockets.c
index 1585a09..119cb99 100644
--- a/server/sockets.c
+++ b/server/sockets.c
@@ -365,7 +365,7 @@ accept_connection (int listen_sock)
   const int flag = 1;
 
   thread_data = malloc (sizeof *thread_data);
-  if (!thread_data) {
+  if (unlikely (!thread_data)) {
     perror ("malloc");
     return;
   }
@@ -409,7 +409,7 @@ accept_connection (int listen_sock)
   pthread_attr_setdetachstate (&attrs, PTHREAD_CREATE_DETACHED);
   err = pthread_create (&thread, &attrs, start_thread, thread_data);
   pthread_attr_destroy (&attrs);
-  if (err != 0) {
+  if (unlikely (err != 0)) {
     fprintf (stderr, "%s: pthread_create: %s\n", program_name,
strerror (err));
     close (thread_data->sock);
     free (thread_data);
-- 
2.23.0

On 11/2/19 4:24 PM, Richard W.M. Jones wrote:
> For GCC only, define unlikely() macro.  Use it on error paths to move
> code out of the hot path.
> 
> In the server only, use the debug() macro (don't call nbdkit_debug
> directly).  This macro checks the verbose flag and moves the call to
> nbdkit_debug out of the hot path.
> ---
> +++ b/server/internal.h
> @@ -45,6 +45,17 @@
>   #include "cleanup.h"
>   #include "nbd-protocol.h"
>   
> +/* Define unlikely macro, but only for GCC.  These are used to move
> + * debug and error handling code out of hot paths.
> + */
> +#if defined(__GNUC__)
> +#define unlikely(x) __builtin_expect (!!(x), 0)
> +#define if_verbose if (unlikely (verbose))
Doesn't clang define __GNUC__, at which point all of our supported 
compilers (since we require __attribute__((cleanup)) support) also have 
__builtin_expect?
> +#else
> +#define unlikely(x) (x)
> +#define if_verbose if (verbose)
> +#endif
Or put another way, this #else may be dead code, and the #if may be 
unnecessary compared to just unconditionally defining unlikely() and 
if_verbose.

At any rate, the changes make sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Nov 04, 2019 at 01:30:58PM -0600, Eric Blake
wrote:
> On 11/2/19 4:24 PM, Richard W.M. Jones wrote:
> >For GCC only, define unlikely() macro.  Use it on error paths to move
> >code out of the hot path.
> >
> >In the server only, use the debug() macro (don't call nbdkit_debug
> >directly).  This macro checks the verbose flag and moves the call to
> >nbdkit_debug out of the hot path.
> >---
> 
> >+++ b/server/internal.h
> >@@ -45,6 +45,17 @@
> >  #include "cleanup.h"
> >  #include "nbd-protocol.h"
> >+/* Define unlikely macro, but only for GCC.  These are used to move
> >+ * debug and error handling code out of hot paths.
> >+ */
> >+#if defined(__GNUC__)
> >+#define unlikely(x) __builtin_expect (!!(x), 0)
> >+#define if_verbose if (unlikely (verbose))
> 
> Doesn't clang define __GNUC__, at which point all of our supported
> compilers (since we require __attribute__((cleanup)) support) also
> have __builtin_expect?
> 
> >+#else
> >+#define unlikely(x) (x)
> >+#define if_verbose if (verbose)
> >+#endif
> 
> Or put another way, this #else may be dead code, and the #if may be
> unnecessary compared to just unconditionally defining unlikely() and
> if_verbose.
Yes ... https://bugs.llvm.org/show_bug.cgi?id=30446

However it seems to be fine in this particular case.  I agree the
other branch is dead code but it's probably best to keep it for future
portability.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top