[Libguestfs] [PATCH nbdkit 3/3] server: Remove explicit connection parameter, use TLS instead.

[Richard W.M. Jones]

On Tue, Feb 11, 2020 at 11:46:06AM -0600, Eric Blake wrote:
> On 2/11/20 11:15 AM, Richard W.M. Jones wrote:
> >Since commit 86fdb48c6a5362d66865493d9d2172166f99722e we have stored
> >the connection object in thread-local storage.
> >
> >In this very large, but mostly mechanical change we stop passing the
> >connection pointer around everywhere, and instead use the value stored
> >in thread-local storage.
> >
> >This assumes a 1-1 mapping between the connection and the current
> >thread which is true in *most* places.  Occasionally we still have the
> >explicit connection pointer, especially just before we launch a thread
> >or call threadlocal_set_conn.
> >---
> >  server/internal.h                    | 191 +++++++++----------
> >  server/backend.c                     | 160 +++++++++-------
> >  server/connections.c                 |  68 ++++---
> >  server/crypto.c                      |  14 +-
> >  server/filters.c                     | 270 +++++++++++++--------------
> >  server/locks.c                       |  12 +-
> >  server/plugins.c                     |  64 +++----
> >  server/protocol-handshake-newstyle.c | 172 +++++++++--------
> >  server/protocol-handshake-oldstyle.c |   7 +-
> >  server/protocol-handshake.c          |  40 ++--
> >  server/protocol.c                    | 127 ++++++-------
> >  server/public.c                      |   2 +-
> >  server/test-public.c                 |   2 +-
> >  13 files changed, 574 insertions(+), 555 deletions(-)
> 
> Makes sense to me (and not too hard to rebase my NBD_INFO_INIT_STATE
> stuff on top of it).  Are we sure that there is not going to be a
> speed penalty (from frequent access to the thread-local storage,
> compared to previous access through a parameter stored in a
> register)?

In v2 the new GET_CONN macro still makes a non-inline call to
threadlocal_get_conn ().

I changed threadlocal_get_conn so it's inlined in the header file
internal.h - this is quite an ugly change because you have to pull a
few private structures into that header file.  Anyway it still makes a
non-inline call to pthread_getspecific.

So I went back to v2 and instead enabled LTO, but sadly something
(probably libtool) breaks LTO.

Anyway TLS data is basically a dereference through a register:

https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/pthread_getspecific.c;h=569dae47c609b85f4f57dc839578ffc1993b550e;hb=HEAD#l31

(note that THREAD_SELF is a macro which returns a constant offset in
the %fs segment) so it should eventually be possible to make this
fast.

> A few comments:
> 
> >+++ b/server/filters.c
> >@@ -49,13 +49,13 @@ struct backend_filter {
> >    struct nbdkit_filter filter;
> >  };
> >-/* Literally a backend, a connection pointer, and the filter's handle.
> >+/* Literally a backend and the filter's handle.
> >+ *
> >   * This is the implementation of our handle in .open, and serves as
> >   * a stable ‘void *nxdata’ in the filter API.
> >   */
> >-struct b_conn {
> >+struct b_h {
> >    struct backend *b;
> >-  struct connection *conn;
> >    void *handle;
> >  };
> >@@ -186,22 +186,22 @@ filter_config_complete (struct backend *b)
> >  static int
> >  next_preconnect (void *nxdata, int readonly)
> >  {
> >-  struct b_conn *b_conn = nxdata;
> >-  return b_conn->b->preconnect (b_conn->b, b_conn->conn, readonly);
> >+  struct b_h *b_h = nxdata;
> >+  return b_h->b->preconnect (b_h->b, readonly);
> >  }
> 
> None of the next_*() wrappers use b_h->handle, they all stick to
> b_h->b.  I don't think that matters too much, other than...
> 
> >@@ -267,101 +266,101 @@ filter_close (struct backend *b, struct connection *conn, void *handle)
> >  /* The next_functions structure contains pointers to backend
> >   * functions.  However because these functions are all expecting a
> >- * backend and a connection, we cannot call them directly, but must
> >+ * backend and a handle, we cannot call them directly, but must
> >   * write some next_* functions that unpack the two parameters from a
> >- * single ‘void *nxdata’ struct pointer (‘b_conn’).
> >+ * single ‘void *nxdata’ struct pointer (‘b_h’).
> 
> ...this comment is slightly off.  In fact, if ALL we use is b_h->b,
> we could probably populate next_ops with backend_* functions rather
> than next_* wrapper functions:

This was in fact my original motivation for this patch series, except
I was expecting we'd use struct backend * instead of the function.

As you say in the snipped bit, this should be a separate patch once we
get this one right.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v