Richard W.M. Jones
2019-Aug-11 13:03 UTC
[Libguestfs] [PATCH libnbd proposal] api: Add semi-private function for freeing persistent data.
This adds a C-only semi-private function for freeing various types of persistent data passed to libnbd. There are some similarities with nbd_add_close_callback which we removed in commit 7f191b150b52ed50098976309a6af883d245fc56. --- generator/generator | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/generator/generator b/generator/generator index 55c4dfc..0ea6b61 100755 --- a/generator/generator +++ b/generator/generator @@ -3284,6 +3284,7 @@ let generate_lib_libnbd_syms () pr "LIBNBD_%d.%d {\n" major minor; pr " global:\n"; if (major, minor) = (1, 0) then ( + pr " nbd_add_free_callback;\n"; pr " nbd_create;\n"; pr " nbd_close;\n"; pr " nbd_get_errno;\n"; @@ -3581,6 +3582,12 @@ let generate_include_libnbd_h () pr "extern int nbd_get_errno (void);\n"; pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n"; pr "\n"; + pr "typedef void (*nbd_free_callback) (void *ptr);\n"; + pr "extern int nbd_add_free_callback (struct nbd_handle *h,\n"; + pr " nbd_free_callback cb,\n"; + pr " void *ptr);\n"; + pr "#define LIBNBD_HAVE_NBD_ADD_FREE_CALLBACK 1\n"; + pr "\n"; print_closure_typedefs (); List.iter ( fun (name, { args; optargs; ret }) -> @@ -4005,6 +4012,45 @@ C<E<lt>errno.hE<gt>>. List.iter print_api handle_calls; pr "\ +=head1 FREE CALLBACKS + +B<Note:> The API described in this section is only +available from C. It is designed to help when writing +bindings to libnbd in other programming languages. +As such it is B<not> covered by the usual API stability +guarantee offered by other parts of the C API. Use it with care. + +Some pointers passed to libnbd calls are saved in the +S<C<struct nbd_handle>>. These include pointers to buffers +passed to C<nbd_aio_pread>, C<nbd_aio_pwrite>, etc., +and pointers to the C<user_data> for callbacks. If you +want to know when it is safe to free these objects then +you can register a free callback using: + + typedef void (*nbd_free_callback) (void *ptr); + int nbd_add_free_callback (struct nbd_handle *h, + nbd_free_callback cb, + void *ptr); + +C<ptr> is a pointer to the object (ie. the buffer or +callback C<user_data>). C<cb (ptr)> is called when libnbd +no longer holds a reference to that object. + +To illustrate how this works with an example: + + struct my_user_data *my_user_data; + void *buf; + + my_user_data = malloc (sizeof *my_user_data); + buf = malloc (512); + nbd_add_free_callback (nbd, free, my_user_data); + nbd_add_free_callback (nbd, free, buf); + nbd_aio_pwrite_callback (nbd, buf, 512, 0, + write_completed, my_user_data, 0); + +This would free both C<my_user_data> and C<buf> once libnbd +has finished with them. + =head1 SEE ALSO L<libnbd(3)>. -- 2.22.0
Eric Blake
2019-Aug-12 14:08 UTC
Re: [Libguestfs] [PATCH libnbd proposal] api: Add semi-private function for freeing persistent data.
On 8/11/19 8:03 AM, Richard W.M. Jones wrote:> This adds a C-only semi-private function for freeing various types of > persistent data passed to libnbd. > > There are some similarities with nbd_add_close_callback which we > removed in commit 7f191b150b52ed50098976309a6af883d245fc56. > --- > generator/generator | 46 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/generator/generator b/generator/generator > index 55c4dfc..0ea6b61 100755 > --- a/generator/generator > +++ b/generator/generator > @@ -3284,6 +3284,7 @@ let generate_lib_libnbd_syms () > pr "LIBNBD_%d.%d {\n" major minor; > pr " global:\n"; > if (major, minor) = (1, 0) then ( > + pr " nbd_add_free_callback;\n"; > pr " nbd_create;\n"; > pr " nbd_close;\n"; > pr " nbd_get_errno;\n"; > @@ -3581,6 +3582,12 @@ let generate_include_libnbd_h () > pr "extern int nbd_get_errno (void);\n"; > pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n"; > pr "\n"; > + pr "typedef void (*nbd_free_callback) (void *ptr);\n"; > + pr "extern int nbd_add_free_callback (struct nbd_handle *h,\n"; > + pr " nbd_free_callback cb,\n"; > + pr " void *ptr);\n"; > + pr "#define LIBNBD_HAVE_NBD_ADD_FREE_CALLBACK 1\n"; > + pr "\n";Would this change the signature of callbacks? With this in place, you no longer need the VALID|FREE parameter, but can defer the free action to an added free callback. How would you actually track when to call the free callback? I guess each time a pointer lifetime ends (basically, when retiring a completed command) we check each pointer grabbed by that command and compare it against the list of registered pointers for any free callbacks to use? How does that scale, if the user can register an arbitrary number of pointers because they queue up a large number of requests before starting the poll loop? Is the existing idea of a VALID|FREE parameter not sufficient? If we can convince the C bindings for python nbd.aio_pread to increase the refcount of buf, and then install a C completion handler (whether the python user called nbd.aio_pread with no python completer, or whether they called nbd.aio_pread_complete) that does the decref when called with FREE, then that would do the same job, even without the need for nbd_add_free_callback, wouldn't it? And there's no issue of scanning through a list of pointers with a free callback associated with them (storing registered pointers with some sort of hash may eventually get to amortized O(1) lookup, but that seems like a lot of overhead compared to the fact that we already have O(1) access to when to call the completion callback with the FREE flag). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Aug-12 14:25 UTC
Re: [Libguestfs] [PATCH libnbd proposal] api: Add semi-private function for freeing persistent data.
On 8/12/19 9:08 AM, Eric Blake wrote:> Is the existing idea of a VALID|FREE parameter not sufficient? If we > can convince the C bindings for python nbd.aio_pread to increase the > refcount of buf, and then install a C completion handler (whether the > python user called nbd.aio_pread with no python completer, or whether > they called nbd.aio_pread_complete) that does the decref when called > with FREE, then that would do the same job, even without the need for > nbd_add_free_callback, wouldn't it? And there's no issue of scanning > through a list of pointers with a free callback associated with them > (storing registered pointers with some sort of hash may eventually get > to amortized O(1) lookup, but that seems like a lot of overhead compared > to the fact that we already have O(1) access to when to call the > completion callback with the FREE flag).Maybe we could add an OClosure optional argument to optargs, and pass the callback closure as an optional argument (making it legal to pass NULL instead of a closure in C, and making it so that other language bindings can have a closure argument present or absent). Thus, in Python, we'd allow any of: h.aio_pread(buf, offset) h.aio_pread(buf, offset, cb=callback) h.aio_pread(buf, offset, flags=nbd.flag) while the C API would always have to pass enough parameters: nbd_aio_pread(h, buf, count, offset, callback, user_data, flags) but maybe we could add a convenience macro: #define nbd_aio_pread_easy(h, buf, count, offset) \ nbd_aio_pread(h, buf, count, offset, NULL, NULL, 0) but it becomes easier to write the Python bindings, because whether or not the python code passed in a Python callback, we still always use a C completion callback to do proper inc/dec of the buf refcounts. On the other hand, the above ideas are a rather large API change, affecting fio and other existing clients. We'll have to think carefully on how many variants the generator actually produces per language, and whether or not we want Python bindings to use a simple name for all cases, while the C API reserves the simple name for the least parameters and uses a longer name when optional parameters are present. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Aug-12 17:58 UTC
Re: [Libguestfs] [PATCH libnbd proposal] api: Add semi-private function for freeing persistent data.
On Mon, Aug 12, 2019 at 09:08:37AM -0500, Eric Blake wrote:> On 8/11/19 8:03 AM, Richard W.M. Jones wrote: > > This adds a C-only semi-private function for freeing various types of > > persistent data passed to libnbd. > > > > There are some similarities with nbd_add_close_callback which we > > removed in commit 7f191b150b52ed50098976309a6af883d245fc56. > > --- > > generator/generator | 46 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/generator/generator b/generator/generator > > index 55c4dfc..0ea6b61 100755 > > --- a/generator/generator > > +++ b/generator/generator > > @@ -3284,6 +3284,7 @@ let generate_lib_libnbd_syms () > > pr "LIBNBD_%d.%d {\n" major minor; > > pr " global:\n"; > > if (major, minor) = (1, 0) then ( > > + pr " nbd_add_free_callback;\n"; > > pr " nbd_create;\n"; > > pr " nbd_close;\n"; > > pr " nbd_get_errno;\n"; > > @@ -3581,6 +3582,12 @@ let generate_include_libnbd_h () > > pr "extern int nbd_get_errno (void);\n"; > > pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n"; > > pr "\n"; > > + pr "typedef void (*nbd_free_callback) (void *ptr);\n"; > > + pr "extern int nbd_add_free_callback (struct nbd_handle *h,\n"; > > + pr " nbd_free_callback cb,\n"; > > + pr " void *ptr);\n"; > > + pr "#define LIBNBD_HAVE_NBD_ADD_FREE_CALLBACK 1\n"; > > + pr "\n"; > > Would this change the signature of callbacks? With this in place, you > no longer need the VALID|FREE parameter, but can defer the free action > to an added free callback.Correct - the full patch series which I've now posted has a final step where we can remove the valid_flag completely.> How would you actually track when to call the free callback? I guess > each time a pointer lifetime ends (basically, when retiring a completed > command) we check each pointer grabbed by that command and compare it > against the list of registered pointers for any free callbacks to use? > How does that scale, if the user can register an arbitrary number of > pointers because they queue up a large number of requests before > starting the poll loop?My implementation scales as O(log n). However nbd_add_free_callback is O(n) because we maintain the list of pointers in address order. nbd_add_free_callback is only really used in a few places by language bindings.> Is the existing idea of a VALID|FREE parameter not sufficient? If we > can convince the C bindings for python nbd.aio_pread to increase the > refcount of buf, and then install a C completion handler (whether the > python user called nbd.aio_pread with no python completer, or whether > they called nbd.aio_pread_complete) that does the decref when called > with FREE, then that would do the same job, even without the need for > nbd_add_free_callback, wouldn't it? And there's no issue of scanning > through a list of pointers with a free callback associated with them > (storing registered pointers with some sort of hash may eventually get > to amortized O(1) lookup, but that seems like a lot of overhead compared > to the fact that we already have O(1) access to when to call the > completion callback with the FREE flag).It's true but then nbd.aio_pread would have to call the _callback variant in all cases. In any case I've posted the implementation of nbd_add_free_callback for discussion, so we can take a look at it and see if it's better or worse. 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
Reasonably Related Threads
- [PATCH libnbd proposal] api: Add semi-private function for freeing persistent data.
- [PATCH libnbd 1/7] api: Add semi-private function for freeing persistent data.
- Re: [PATCH libnbd 1/7] api: Add semi-private function for freeing persistent data.
- Re: [PATCH libnbd 1/7] api: Add semi-private function for freeing persistent data.
- Re: [PATCH libnbd 1/7] api: Add semi-private function for freeing persistent data.