Libguestfs - Feb 2023 - [PATCH nbdkit] curl: Try to share as much as possible between handles in the pool

I'm mainly posting this to the list as a back-up.  It does work, it
does _not_ improve performance in any noticable way.  However I'm
having lots of trouble getting HTTP/2 to work (with or without this
patch) and that's stopping me from testing anything properly.

Rich.

Using the libcurl share interface we can share data between the
separate curl easy handles in the pool.  For more about this see:

https://curl.se/libcurl/c/CURLSHOPT_SHARE.html
https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b
https://everything.curl.dev/libcurl/sharing
---
 plugins/curl/curldefs.h |  3 +-
 plugins/curl/curl.c     |  4 ++-
 plugins/curl/pool.c     | 75 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/plugins/curl/curldefs.h b/plugins/curl/curldefs.h
index c2a3432fc..d614379d0 100644
--- a/plugins/curl/curldefs.h
+++ b/plugins/curl/curldefs.h
@@ -117,9 +117,10 @@ struct curl_handle {
 };
 
 /* pool.c */
+extern void load_pool (void);
+extern void unload_pool (void);
 extern struct curl_handle *get_handle (void);
 extern void put_handle (struct curl_handle *ch);
-extern void free_all_handles (void);
 
 /* scripts.c */
 extern int do_scripts (struct curl_handle *ch);
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
index b5927b5b4..b8624a6f8 100644
--- a/plugins/curl/curl.c
+++ b/plugins/curl/curl.c
@@ -101,6 +101,8 @@ curl_load (void)
     nbdkit_error ("libcurl initialization failed: %d", (int) r);
     exit (EXIT_FAILURE);
   }
+
+  load_pool ();
 }
 
 static void
@@ -112,7 +114,7 @@ curl_unload (void)
   free (password);
   free (proxy_password);
   scripts_unload ();
-  free_all_handles ();
+  unload_pool ();
   curl_global_cleanup ();
 }
 
diff --git a/plugins/curl/pool.c b/plugins/curl/pool.c
index b6f4f8e5f..536123388 100644
--- a/plugins/curl/pool.c
+++ b/plugins/curl/pool.c
@@ -52,6 +52,7 @@
 
 #include <nbdkit-plugin.h>
 
+#include "array-size.h"
 #include "ascii-ctype.h"
 #include "ascii-string.h"
 #include "cleanup.h"
@@ -73,6 +74,18 @@ static int debug_cb (CURL *handle, curl_infotype type,
 static size_t header_cb (void *ptr, size_t size, size_t nmemb, void *opaque);
 static size_t write_cb (char *ptr, size_t size, size_t nmemb, void *opaque);
 static size_t read_cb (void *ptr, size_t size, size_t nmemb, void *opaque);
+static void lock_cb (CURL *handle, curl_lock_data data,
+                     curl_lock_access access, void *userptr);
+static void unlock_cb (CURL *handle, curl_lock_data data,
+                       void *userptr);
+
+/* These locks protect access to the curl share data.  See:
+ * https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b
+ */
+static pthread_rwlock_t lockarray[CURL_LOCK_DATA_LAST];
+
+/* Curl share data. */
+static CURLSH *share;
 
 /* This lock protects access to the curl_handles vector below. */
 static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
@@ -90,18 +103,46 @@ static curl_handle_list curl_handles = empty_vector;
 static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
 static size_t in_use = 0, waiting = 0;
 
-/* Close and free all handles in the pool. */
+/* Initialize pool structures. */
 void
-free_all_handles (void)
+load_pool (void)
 {
   size_t i;
 
+  for (i = 0; i < ARRAY_SIZE (lockarray); ++i)
+    pthread_rwlock_init (&lockarray[i], NULL);
+
+  share = curl_share_init ();
+  if (share == NULL) {
+    nbdkit_error ("curl_share_init: %m");
+    exit (EXIT_FAILURE);
+  }
+  curl_share_setopt (share, CURLSHOPT_LOCKFUNC, lock_cb);
+  curl_share_setopt (share, CURLSHOPT_UNLOCKFUNC, unlock_cb);
+  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
+  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_COOKIE);
+  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
+  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION);
+}
+
+void
+unload_pool (void)
+{
+  size_t i;
+
+  /* Close and free all handles in the pool. */
   nbdkit_debug ("free_all_handles: number of curl handles allocated:
%zu",
                 curl_handles.len);
 
   for (i = 0; i < curl_handles.len; ++i)
     free_handle (curl_handles.ptr[i]);
   curl_handle_list_reset (&curl_handles);
+
+  /* Free share data. */
+  curl_share_cleanup (share);
+
+  for (i = 0; i < ARRAY_SIZE (lockarray); ++i)
+    pthread_rwlock_destroy (&lockarray[i]);
 }
 
 /* Get a handle from the pool.
@@ -221,6 +262,9 @@ allocate_handle (void)
     goto err;
   }
 
+  /* Share settings with other handles. */
+  curl_easy_setopt (ch->c, CURLOPT_SHARE, share);
+
   /* Various options we always set.
    *
    * NB: Both here and below constants must be explicitly long because
@@ -519,3 +563,30 @@ free_handle (struct curl_handle *ch)
     curl_slist_free_all (ch->headers_copy);
   free (ch);
 }
+
+static void
+lock_cb (CURL *handle, curl_lock_data data, curl_lock_access access,
+         void *userptr)
+{
+  assert (data < ARRAY_SIZE (lockarray));
+
+  switch (access) {
+  case CURL_LOCK_ACCESS_SHARED:
+    pthread_rwlock_rdlock (&lockarray[data]);
+    break;
+  case CURL_LOCK_ACCESS_SINGLE:
+    pthread_rwlock_wrlock (&lockarray[data]);
+    break;
+  default:
+    /* CURL_LOCK_ACCESS_NONE is never used in the current libcurl code. */
+    abort ();
+  }
+}
+
+static void
+unlock_cb (CURL *handle, curl_lock_data data, void *userptr)
+{
+  assert (data < ARRAY_SIZE (lockarray));
+
+  pthread_rwlock_unlock (&lockarray[data]);
+}
-- 
2.39.0

On Wed, Feb 22, 2023 at 03:01:45PM +0000, Richard W.M. Jones
wrote:
> I'm mainly posting this to the list as a back-up.  It does work, it
> does _not_ improve performance in any noticable way.  However I'm
> having lots of trouble getting HTTP/2 to work (with or without this
> patch) and that's stopping me from testing anything properly.
Turns out this patch was causing the HTTP/2 problems, because it is
not thread safe.  So nothing to do with curl, and in fact the curl
docs are correct after all.

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