Richard W.M. Jones
2019-May-30 15:35 UTC
[Libguestfs] [PATCH libnbd 0/2] Avoid lock and error overhead on some calls.
This works. I'm in the middle of testing whether there is any noticable benefit. Rich.
Richard W.M. Jones
2019-May-30 15:35 UTC
[Libguestfs] [PATCH libnbd 1/2] generator: Implement unlocked API calls.
There was a "dormant" (ie. never implemented) feature of the generator
for calls which don't need to take the handle lock. This implements
it.
By making h->state be an atomic field (which should be nearly free on
most normal architectures) we can modify calls which only read
h->state and do nothing else with the handle so they never acquire the
lock.
---
generator/generator | 46 +++++++++++++++++++++++++++------------------
lib/aio.c | 6 ++++++
lib/internal.h | 3 ++-
3 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/generator/generator b/generator/generator
index e04c9e1..f0de6b0 100755
--- a/generator/generator
+++ b/generator/generator
@@ -807,9 +807,13 @@ and structured_reply_state_machine = [
type call = {
args : arg list; (* parameters (except handle) *)
ret : ret; (* return value *)
- is_locked : bool; (* most functions need to take a lock *)
shortdesc : string; (* short description *)
longdesc : string; (* long description *)
+ (* Most functions must take a lock. The only known exception is
+ * for a function which {b only} reads from the atomic [h->state]
+ * field and does nothing else with the handle.
+ *)
+ is_locked : bool;
}
and arg | ArrayAndLen of arg * string (* array + number of entries *)
@@ -841,11 +845,18 @@ and ret | RInt64 (* 64 bit int, -1 =
error *)
| RString (* return a newly allocated string, caller frees *)
-let default_call = { args = []; ret = RErr; is_locked = true;
- shortdesc = ""; longdesc = "" }
+let default_call = { args = []; ret = RErr;
+ shortdesc = ""; longdesc = "";
+ is_locked = true }
-(* Calls - first parameter [struct nbd_handle *nbd] is implicit. *)
-let handle_calls = [
+(* Calls.
+ *
+ * The first parameter [struct nbd_handle *nbd] is implicit.
+ *
+ * Disable:
+ * Warning 23: all the fields are explicitly listed in this record:
+ *)
+let [@warning "-23"] handle_calls = [
"set_debug", {
default_call with
args = [ Bool "debug" ]; ret = RErr;
@@ -1668,7 +1679,7 @@ connection is writable.";
"aio_is_created", {
default_call with
- args = []; ret = RBool;
+ args = []; ret = RBool; is_locked = false;
shortdesc = "check if the connection has just been created";
longdesc = "\
Return true if this connection has just been created. This
@@ -1679,7 +1690,7 @@ by calling functions such as
C<nbd_aio_connect>.";
"aio_is_connecting", {
default_call with
- args = []; ret = RBool;
+ args = []; ret = RBool; is_locked = false;
shortdesc = "check if the connection is connecting or
handshaking";
longdesc = "\
Return true if this connection is connecting to the server
@@ -1690,7 +1701,7 @@ issue commands (see C<nbd_aio_is_ready>).";
"aio_is_ready", {
default_call with
- args = []; ret = RBool;
+ args = []; ret = RBool; is_locked = false;
shortdesc = "check if the connection is in the ready state";
longdesc = "\
Return true if this connection is connected to the NBD server,
@@ -1701,7 +1712,7 @@ issue commands.";
"aio_is_processing", {
default_call with
- args = []; ret = RBool;
+ args = []; ret = RBool; is_locked = false;
shortdesc = "check if the connection is processing a command";
longdesc = "\
Return true if this connection is connected to the NBD server,
@@ -1715,7 +1726,7 @@ is processing them), but libnbd is not processing
them.";
"aio_is_dead", {
default_call with
- args = []; ret = RBool;
+ args = []; ret = RBool; is_locked = false;
shortdesc = "check if the connection is dead";
longdesc = "\
Return true if the connection has encountered a fatal
@@ -1725,7 +1736,7 @@ There is no way to recover a handle from the dead
state.";
"aio_is_closed", {
default_call with
- args = []; ret = RBool;
+ args = []; ret = RBool; is_locked = false;
shortdesc = "check if the connection is closed";
longdesc = "\
Return true if the connection has closed. There is no way to
@@ -2650,7 +2661,7 @@ let generate_lib_unlocked_h () * grab the thread mutex
(lock) and do logging.
*)
let generate_lib_api_c () - let print_wrapper name args ret + let
print_wrapper (name, {args; ret; is_locked}) (match ret with
| RBool
| RErr
@@ -2674,13 +2685,15 @@ let generate_lib_api_c () | RString -> pr
" char *ret;\n"
);
pr "\n";
- pr " pthread_mutex_lock (&h->lock);\n";
+ if is_locked then
+ pr " pthread_mutex_lock (&h->lock);\n";
pr " nbd_internal_reset_error (\"nbd_%s\");\n" name;
pr " ret = nbd_unlocked_%s (h" name;
let argnames = List.flatten (List.map name_of_arg args) in
List.iter (pr ", %s") argnames;
pr ");\n";
- pr " pthread_mutex_unlock (&h->lock);\n";
+ if is_locked then
+ pr " pthread_mutex_unlock (&h->lock);\n";
pr " return ret;\n";
pr "}\n";
pr "\n";
@@ -2698,10 +2711,7 @@ let generate_lib_api_c () pr "#include
\"libnbd.h\"\n";
pr "#include \"internal.h\"\n";
pr "\n";
- List.iter (
- fun (name, { args; ret }) ->
- print_wrapper name args ret
- ) handle_calls
+ List.iter print_wrapper handle_calls
let print_api (name, { args; ret; shortdesc; longdesc }) pr "=head2 %s
—\n" name;
diff --git a/lib/aio.c b/lib/aio.c
index 5adc3cd..65b1ef3 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -48,6 +48,7 @@ nbd_unlocked_aio_notify_write (struct nbd_handle *h)
return nbd_internal_run (h, notify_write);
}
+/* NB: is_locked = false. */
int
nbd_unlocked_aio_is_created (struct nbd_handle *h)
{
@@ -72,6 +73,7 @@ is_connecting_group (enum state_group group)
}
}
+/* NB: is_locked = false. */
int
nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
{
@@ -80,6 +82,7 @@ nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
return is_connecting_group (group);
}
+/* NB: is_locked = false. */
int
nbd_unlocked_aio_is_ready (struct nbd_handle *h)
{
@@ -100,6 +103,7 @@ is_processing_group (enum state_group group)
}
}
+/* NB: is_locked = false. */
int
nbd_unlocked_aio_is_processing (struct nbd_handle *h)
{
@@ -108,12 +112,14 @@ nbd_unlocked_aio_is_processing (struct nbd_handle *h)
return is_processing_group (group);
}
+/* NB: is_locked = false. */
int
nbd_unlocked_aio_is_dead (struct nbd_handle *h)
{
return h->state == STATE_DEAD;
}
+/* NB: is_locked = false. */
int
nbd_unlocked_aio_is_closed (struct nbd_handle *h)
{
diff --git a/lib/internal.h b/lib/internal.h
index 419795f..73cb3f9 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -20,6 +20,7 @@
#define LIBNBD_INTERNAL_H
#include <stdbool.h>
+#include <stdatomic.h>
#include <string.h>
#include <netdb.h>
#include <sys/types.h>
@@ -79,7 +80,7 @@ struct nbd_handle {
/* Linked list of close callbacks. */
struct close_callback *close_callbacks;
- enum state state; /* State machine. */
+ _Atomic enum state state; /* State machine. */
bool structured_replies; /* If we negotiated NBD_OPT_STRUCTURED_REPLY */
--
2.21.0
Richard W.M. Jones
2019-May-30 15:35 UTC
[Libguestfs] [PATCH libnbd 2/2] generator: Annotate calls which never call set_error.
On entry to all calls we currently set the error context in
thread-local data. This is somewhat non-trivial, and is wasted effort
if set_error is never called. Avoid this if the call guarantees never
to call set_error.
---
generator/generator | 24 +++++++++++++++---------
lib/aio.c | 12 ++++++------
2 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/generator/generator b/generator/generator
index f0de6b0..cdae440 100755
--- a/generator/generator
+++ b/generator/generator
@@ -814,6 +814,11 @@ type call = {
* field and does nothing else with the handle.
*)
is_locked : bool;
+ (* Most functions can call set_error. For functions which are
+ * {b guaranteed} never to do that we can save a bit of time by
+ * setting this to false.
+ *)
+ can_set_error : bool;
}
and arg | ArrayAndLen of arg * string (* array + number of entries *)
@@ -847,7 +852,7 @@ and ret
let default_call = { args = []; ret = RErr;
shortdesc = ""; longdesc = "";
- is_locked = true }
+ is_locked = true; can_set_error = true }
(* Calls.
*
@@ -1679,7 +1684,7 @@ connection is writable.";
"aio_is_created", {
default_call with
- args = []; ret = RBool; is_locked = false;
+ args = []; ret = RBool; is_locked = false; can_set_error = false;
shortdesc = "check if the connection has just been created";
longdesc = "\
Return true if this connection has just been created. This
@@ -1690,7 +1695,7 @@ by calling functions such as
C<nbd_aio_connect>.";
"aio_is_connecting", {
default_call with
- args = []; ret = RBool; is_locked = false;
+ args = []; ret = RBool; is_locked = false; can_set_error = false;
shortdesc = "check if the connection is connecting or
handshaking";
longdesc = "\
Return true if this connection is connecting to the server
@@ -1701,7 +1706,7 @@ issue commands (see C<nbd_aio_is_ready>).";
"aio_is_ready", {
default_call with
- args = []; ret = RBool; is_locked = false;
+ args = []; ret = RBool; is_locked = false; can_set_error = false;
shortdesc = "check if the connection is in the ready state";
longdesc = "\
Return true if this connection is connected to the NBD server,
@@ -1712,7 +1717,7 @@ issue commands.";
"aio_is_processing", {
default_call with
- args = []; ret = RBool; is_locked = false;
+ args = []; ret = RBool; is_locked = false; can_set_error = false;
shortdesc = "check if the connection is processing a command";
longdesc = "\
Return true if this connection is connected to the NBD server,
@@ -1726,7 +1731,7 @@ is processing them), but libnbd is not processing
them.";
"aio_is_dead", {
default_call with
- args = []; ret = RBool; is_locked = false;
+ args = []; ret = RBool; is_locked = false; can_set_error = false;
shortdesc = "check if the connection is dead";
longdesc = "\
Return true if the connection has encountered a fatal
@@ -1736,7 +1741,7 @@ There is no way to recover a handle from the dead
state.";
"aio_is_closed", {
default_call with
- args = []; ret = RBool; is_locked = false;
+ args = []; ret = RBool; is_locked = false; can_set_error = false;
shortdesc = "check if the connection is closed";
longdesc = "\
Return true if the connection has closed. There is no way to
@@ -2661,7 +2666,7 @@ let generate_lib_unlocked_h () * grab the thread mutex
(lock) and do logging.
*)
let generate_lib_api_c () - let print_wrapper (name, {args; ret; is_locked}) +
let print_wrapper (name, {args; ret; is_locked; can_set_error}) (match ret
with
| RBool
| RErr
@@ -2687,7 +2692,8 @@ let generate_lib_api_c () pr "\n";
if is_locked then
pr " pthread_mutex_lock (&h->lock);\n";
- pr " nbd_internal_reset_error (\"nbd_%s\");\n" name;
+ if can_set_error then
+ pr " nbd_internal_reset_error (\"nbd_%s\");\n" name;
pr " ret = nbd_unlocked_%s (h" name;
let argnames = List.flatten (List.map name_of_arg args) in
List.iter (pr ", %s") argnames;
diff --git a/lib/aio.c b/lib/aio.c
index 65b1ef3..9e0db68 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -48,7 +48,7 @@ nbd_unlocked_aio_notify_write (struct nbd_handle *h)
return nbd_internal_run (h, notify_write);
}
-/* NB: is_locked = false. */
+/* NB: is_locked = false, can_set_error = false. */
int
nbd_unlocked_aio_is_created (struct nbd_handle *h)
{
@@ -73,7 +73,7 @@ is_connecting_group (enum state_group group)
}
}
-/* NB: is_locked = false. */
+/* NB: is_locked = false, can_set_error = false. */
int
nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
{
@@ -82,7 +82,7 @@ nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
return is_connecting_group (group);
}
-/* NB: is_locked = false. */
+/* NB: is_locked = false, can_set_error = false. */
int
nbd_unlocked_aio_is_ready (struct nbd_handle *h)
{
@@ -103,7 +103,7 @@ is_processing_group (enum state_group group)
}
}
-/* NB: is_locked = false. */
+/* NB: is_locked = false, can_set_error = false. */
int
nbd_unlocked_aio_is_processing (struct nbd_handle *h)
{
@@ -112,14 +112,14 @@ nbd_unlocked_aio_is_processing (struct nbd_handle *h)
return is_processing_group (group);
}
-/* NB: is_locked = false. */
+/* NB: is_locked = false, can_set_error = false. */
int
nbd_unlocked_aio_is_dead (struct nbd_handle *h)
{
return h->state == STATE_DEAD;
}
-/* NB: is_locked = false. */
+/* NB: is_locked = false, can_set_error = false. */
int
nbd_unlocked_aio_is_closed (struct nbd_handle *h)
{
--
2.21.0
Richard W.M. Jones
2019-May-30 15:52 UTC
Re: [Libguestfs] [PATCH libnbd 0/2] Avoid lock and error overhead on some calls.
On Thu, May 30, 2019 at 04:35:04PM +0100, Richard W.M. Jones wrote:> This works. I'm in the middle of testing whether there is any > noticable benefit.Is it faster? It's not very clear, but here are some timings. This is with examples/threaded-reads-and-writes with the number of cycles multipled by 100. Before: real 0m27.616s real 0m27.417s real 0m28.396s real 0m28.772s real 0m26.622s After: real 0m23.984s real 0m24.357s real 0m28.294s real 0m27.036s real 0m24.862s The test only calls the nbd_aio_is_* functions in 4 places, but they are called quite often. I think this patch series is good, even if it's not going to fix your 10x slowdown :-/ 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
Possibly Parallel Threads
- [PATCH libnbd 0/4] lib: Atomically update h->state.
- [PATCH libnbd v2 0/6] Test connection states.
- [PATCH libnbd] api: Get rid of nbd_connection.
- [PATCH libnbd v3] lib: Atomically update h->state when leaving the locked region.
- [libnbd PATCH] api: Add nbd_supports_tls