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

2023-07-27 Thread Richard W.M. Jones 


Actually once I re-read Daniel Stenberg's reply from last time I see
that using the share interface is actually pessimal with HTTP/2, /3,
see:

https://curl.se/mail/lib-2023-02/0077.html
https://curl.se/mail/lib-2023-02/0082.html

I think I'll have to look at the multi interface instead.

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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-07-27 Thread Richard W.M. Jones 
On Thu, Jul 27, 2023 at 01:59:54PM +0200, Laszlo Ersek wrote:
> On 7/27/23 13:46, Richard W.M. Jones wrote:
> > 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/pool.c | 65 +
> >  1 file changed, 65 insertions(+)
> 
> Can you document the benefits of this change in the commit message?
> (Faster downloads / lower CPU demand / lower memory usage -- just guessing.)

I would do if I knew what they were :-/ There are a bunch of
complexities which I don't understand.  I'm going to ask a question on
the curl mailing list about this in a min.

Rich.

> Acked-by: Laszlo Ersek 
> 
> Thanks
> Laszlo
> 
> > 
> > diff --git a/plugins/curl/pool.c b/plugins/curl/pool.c
> > index 10f9011d7..e9f387c38 100644
> > --- a/plugins/curl/pool.c
> > +++ b/plugins/curl/pool.c
> > @@ -52,6 +52,7 @@
> >  
> >  #include 
> >  
> > +#include "array-size.h"
> >  #include "ascii-ctype.h"
> >  #include "ascii-string.h"
> >  #include "cleanup.h"
> > @@ -72,6 +73,18 @@ static int get_content_length_accept_range (struct 
> > curl_handle *ch);
> >  static bool try_fallback_GET_method (struct curl_handle *ch);
> >  static size_t header_cb (void *ptr, size_t size, size_t nmemb, void 
> > *opaque);
> >  static size_t error_cb (char *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 sharelocks[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;
> > @@ -93,6 +106,22 @@ static size_t in_use = 0, waiting = 0;
> >  void
> >  load_pool (void)
> >  {
> > +  size_t i;
> > +
> > +  for (i = 0; i < ARRAY_SIZE (sharelocks); ++i)
> > +pthread_rwlock_init ([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);
> >  }
> >  
> >  /* Close and free all handles in the pool. */
> > @@ -108,6 +137,12 @@ unload_pool (void)
> >for (i = 0; i < curl_handles.len; ++i)
> >  free_handle (curl_handles.ptr[i]);
> >curl_handle_list_reset (_handles);
> > +
> > +  /* Free share data. */
> > +  curl_share_cleanup (share);
> > +
> > +  for (i = 0; i < ARRAY_SIZE (sharelocks); ++i)
> > +pthread_rwlock_destroy ([i]);
> >  }
> >  
> >  /* Get a handle from the pool.
> > @@ -230,6 +265,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
> > @@ -621,3 +659,30 @@ error_cb (char *ptr, size_t size, size_t nmemb, void 
> > *opaque)
> >return 0; /* in older curl, any size < requested will also be an error */
> >  #endif
> >  }
> > +
> > +static void
> > +lock_cb (CURL *handle, curl_lock_data data, curl_lock_access access,
> > + void *userptr)
> > +{
> > +  assert (data < ARRAY_SIZE (sharelocks));
> > +
> > +  switch (access) {
> > +  case CURL_LOCK_ACCESS_SHARED:
> > +pthread_rwlock_rdlock ([data]);
> > +break;
> > +  case CURL_LOCK_ACCESS_SINGLE:
> > +pthread_rwlock_wrlock ([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 (sharelocks));
> > +
> > +  pthread_rwlock_unlock ([data]);
> > +}

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.

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

2023-07-27 Thread Richard W.M. Jones 
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/pool.c | 65 +
 1 file changed, 65 insertions(+)

diff --git a/plugins/curl/pool.c b/plugins/curl/pool.c
index 10f9011d7..e9f387c38 100644
--- a/plugins/curl/pool.c
+++ b/plugins/curl/pool.c
@@ -52,6 +52,7 @@
 
 #include 
 
+#include "array-size.h"
 #include "ascii-ctype.h"
 #include "ascii-string.h"
 #include "cleanup.h"
@@ -72,6 +73,18 @@ static int get_content_length_accept_range (struct 
curl_handle *ch);
 static bool try_fallback_GET_method (struct curl_handle *ch);
 static size_t header_cb (void *ptr, size_t size, size_t nmemb, void *opaque);
 static size_t error_cb (char *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 sharelocks[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;
@@ -93,6 +106,22 @@ static size_t in_use = 0, waiting = 0;
 void
 load_pool (void)
 {
+  size_t i;
+
+  for (i = 0; i < ARRAY_SIZE (sharelocks); ++i)
+pthread_rwlock_init ([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);
 }
 
 /* Close and free all handles in the pool. */
@@ -108,6 +137,12 @@ unload_pool (void)
   for (i = 0; i < curl_handles.len; ++i)
 free_handle (curl_handles.ptr[i]);
   curl_handle_list_reset (_handles);
+
+  /* Free share data. */
+  curl_share_cleanup (share);
+
+  for (i = 0; i < ARRAY_SIZE (sharelocks); ++i)
+pthread_rwlock_destroy ([i]);
 }
 
 /* Get a handle from the pool.
@@ -230,6 +265,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
@@ -621,3 +659,30 @@ error_cb (char *ptr, size_t size, size_t nmemb, void 
*opaque)
   return 0; /* in older curl, any size < requested will also be an error */
 #endif
 }
+
+static void
+lock_cb (CURL *handle, curl_lock_data data, curl_lock_access access,
+ void *userptr)
+{
+  assert (data < ARRAY_SIZE (sharelocks));
+
+  switch (access) {
+  case CURL_LOCK_ACCESS_SHARED:
+pthread_rwlock_rdlock ([data]);
+break;
+  case CURL_LOCK_ACCESS_SINGLE:
+pthread_rwlock_wrlock ([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 (sharelocks));
+
+  pthread_rwlock_unlock ([data]);
+}
-- 
2.41.0

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-02-26 Thread Richard W.M. Jones 
On Wed, Feb 22, 2023 at 03:01:45PM +, 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-02-22 Thread Richard W.M. Jones 
On Wed, Feb 22, 2023 at 05:25:03PM +0100, Laszlo Ersek wrote:
> On 2/22/23 17:00, Richard W.M. Jones wrote:
> > On Wed, Feb 22, 2023 at 04:52:35PM +0100, Laszlo Ersek wrote:
> > ...
> >> Seems sane to me in general, except for the fact that the documentation
> >> at  writes:
> >>
> >> """
> >> CURL_LOCK_DATA_CONNECT
> >>
> >> Put the connection cache in the share object and make all easy handles
> >> using this share object share the connection cache.
> >>
> >> Note that due to a known bug, it is not safe to share connections this
> >> way between multiple concurrent threads. [...]
> >> """
> >>
> >> Ugh, what? If it's not safe to share the connection cache between
> >> threads, then CURL_LOCK_DATA_CONNECT is effectively unusable, and no
> >> connection pooling can be done. How does that *not* make this whole curl
> >> facility useless?
> >>
> >> The facility in general looks super weird; what sense does it make *not*
> >> to share some particular CURL_LOCK_DATA_xxx?
> > 
> > I can only conclude this cannot be true.  Daniel Stenberg wrote this
> > code which definitely uses threads:
> > 
> > https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b
> > 
> > Also I did a lot of testing and didn't hit any obvious threading bugs.
> 
> OK, so the documentation is busted. News at 11.
> 
> Can you report a bug for cURL, or should I report it sometime later?

I don't think I'm certain enough to file a bug ..

Rich.

> > Nevertheless I'm not planning to integrate this patch any time soon.
> 
> I don't oppose merging the patch once we get a clear verdict that the
> documentation is wrong.
> 
> Thanks!
> Laszlo

-- 
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-02-22 Thread Laszlo Ersek 
On 2/22/23 17:00, Richard W.M. Jones wrote:
> On Wed, Feb 22, 2023 at 04:52:35PM +0100, Laszlo Ersek wrote:
> ...
>> Seems sane to me in general, except for the fact that the documentation
>> at  writes:
>>
>> """
>> CURL_LOCK_DATA_CONNECT
>>
>> Put the connection cache in the share object and make all easy handles
>> using this share object share the connection cache.
>>
>> Note that due to a known bug, it is not safe to share connections this
>> way between multiple concurrent threads. [...]
>> """
>>
>> Ugh, what? If it's not safe to share the connection cache between
>> threads, then CURL_LOCK_DATA_CONNECT is effectively unusable, and no
>> connection pooling can be done. How does that *not* make this whole curl
>> facility useless?
>>
>> The facility in general looks super weird; what sense does it make *not*
>> to share some particular CURL_LOCK_DATA_xxx?
> 
> I can only conclude this cannot be true.  Daniel Stenberg wrote this
> code which definitely uses threads:
> 
> https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b
> 
> Also I did a lot of testing and didn't hit any obvious threading bugs.

OK, so the documentation is busted. News at 11.

Can you report a bug for cURL, or should I report it sometime later?

> Nevertheless I'm not planning to integrate this patch any time soon.

I don't oppose merging the patch once we get a clear verdict that the
documentation is wrong.

Thanks!
Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-02-22 Thread Richard W.M. Jones 
On Wed, Feb 22, 2023 at 04:52:35PM +0100, Laszlo Ersek wrote:
...
> Seems sane to me in general, except for the fact that the documentation
> at  writes:
> 
> """
> CURL_LOCK_DATA_CONNECT
> 
> Put the connection cache in the share object and make all easy handles
> using this share object share the connection cache.
> 
> Note that due to a known bug, it is not safe to share connections this
> way between multiple concurrent threads. [...]
> """
> 
> Ugh, what? If it's not safe to share the connection cache between
> threads, then CURL_LOCK_DATA_CONNECT is effectively unusable, and no
> connection pooling can be done. How does that *not* make this whole curl
> facility useless?
>
> The facility in general looks super weird; what sense does it make *not*
> to share some particular CURL_LOCK_DATA_xxx?

I can only conclude this cannot be true.  Daniel Stenberg wrote this
code which definitely uses threads:

https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b

Also I did a lot of testing and didn't hit any obvious threading bugs.

Nevertheless I'm not planning to integrate this patch any time soon.

> Laszlo

Thanks, Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

2023-02-22 Thread Laszlo Ersek 
On 2/22/23 16:01, Richard W.M. Jones wrote:
> 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 
>  
> +#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 ([i], NULL);

error checking missing

> +
> +  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);

error checking missing

> +}
> +
> +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 (_handles);
> +
> +  /* Free share data. */
> +  curl_share_cleanup (share);
> +
> +  for (i = 0; i < ARRAY_SIZE (lockarray); ++i)
> +pthread_rwlock_destroy ([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)
> +{
> +

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

2023-02-22 Thread Richard W.M. Jones 
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 
 
+#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 ([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 (_handles);
+
+  /* Free share data. */
+  curl_share_cleanup (share);
+
+  for (i = 0; i < ARRAY_SIZE (lockarray); ++i)
+pthread_rwlock_destroy ([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 ([data]);
+break;
+  case CURL_LOCK_ACCESS_SINGLE:
+pthread_rwlock_wrlock ([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

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

2023-02-22 Thread Richard W.M. Jones 
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.


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs