On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
By using the free callback mechanism we don't need to manually free the buffer.
This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56. --- generator/generator | 43 +++++++++++++++++++++----------- ocaml/buffer.c | 22 ---------------- ocaml/examples/asynch_copy.ml | 4 +-- ocaml/libnbd-ocaml.pod | 22 ---------------- ocaml/nbd-c.h | 8 ++++++ ocaml/tests/test_590_aio_copy.ml | 4 +-- 6 files changed, 39 insertions(+), 64 deletions(-)
diff --git a/generator/generator b/generator/generator index d6b9352..92ce170 100755 --- a/generator/generator +++ b/generator/generator @@ -4961,10 +4961,6 @@ module Buffer : sig (** Allocate an uninitialized buffer. The parameter is the size in bytes. *)
- val free : t -> unit - (** The buffer must be manually freed when it is no longer used. - An AIO completion callback is usually a good place. *) -
Getting rid of this public function is nice. Hopefully, how we get rid of it is internal enough that we can switch between either a list of free functions at the C level, or else a smarter forwarding of aio_pread to the C nbd_aio_pread_completion, without affecting what else leaks through to the OCaml view.
@@ -5244,10 +5239,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " caml_enter_blocking_section ();\n"; pr " }\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_FREE) {\n"; - pr " caml_remove_generational_global_root ((value *)user_data);\n"; - pr " free (user_data);\n"; - pr " }\n"; + pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; + pr " free_root (NULL, user_data);\n"; pr "\n";
Here, I'm unfamiliar enough with the OCaml GC rules to know if you're doing it right, but trust that you have been testing with valgrind or similar with a forced GC run to catch obvious mistakes.
@@ -5313,17 +5306,39 @@ let print_ocaml_binding (name, { args; optargs; ret }) = | BytesIn (n, count) -> pr " const void *%s = Bytes_val (%sv);\n" n n; pr " size_t %s = caml_string_length (%sv);\n" count n + | BytesOut (n, count) -> + pr " void *%s = Bytes_val (%sv);\n" n n; + pr " size_t %s = caml_string_length (%sv);\n" count n | BytesPersistIn (n, count) -> + pr " /* The function may save a reference to the Buffer, so we\n"; + pr " * must treat it as a possible GC root.\n"; + pr " */\n"; + pr " value *%s_user_data;\n" n; + pr " %s_user_data = malloc (sizeof (value));\n" n; + pr " if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" n; + pr " *%s_user_data = %sv;\n" n n; + pr " caml_register_generational_global_root (%s_user_data);\n" n; pr " struct nbd_buffer *%s_buf = NBD_buffer_val (%sv);\n" n n; pr " const void *%s = %s_buf->data;\n" n n; - pr " size_t %s = %s_buf->len;\n" count n - | BytesOut (n, count) -> - pr " void *%s = Bytes_val (%sv);\n" n n; - pr " size_t %s = caml_string_length (%sv);\n" count n + pr " size_t %s = %s_buf->len;\n" count n; + pr " if (nbd_add_free_callback (h, (void *)%s,\n" n;
Is the cast to void* necessary? I guess so - it looks like you are casting away const.
+ pr " free_root, %s_user_data) == -1)\n" n;
Hmm. In patch 1, I questioned if we need a separate user_data argument when registering a free callback. But here, you are keying off of the last use of one pointer (the buffer), but with a different pointer (the user_data wrapper) to be passed to the free function...
+++ b/ocaml/nbd-c.h @@ -107,4 +107,12 @@ struct callback_data { extern char **nbd_internal_ocaml_string_list (value); extern value nbd_internal_ocaml_alloc_int32_array (uint32_t *, size_t);
+/* Free a generational global root and its container. */ +static inline void +free_root (void *ptr /* unused */, void *rootp) +{ + caml_remove_generational_global_root ((value *)rootp); + free (rootp); +}
...and indeed, the free function is ignoring the first pointer and instead acting on the second. If we change the C signature to take only a single pointer, then you have to be a bit more creative on h ow to pass the rootp as the pointer to be adjusted when the buffer lifetime ends. So maybe this is the answer to my question in patch 1 as to why you needed two arguments. Or maybe it is all the more reason to try harder to get the generator to call nbd_aio_pread_callback even when the OCaml language did not pass a callback. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org