| Message ID | 6e5fd9edfea379b69682fa538141298fc1bc3110.1659116941.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/ocaml code and build cleanups | expand |
On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:
Add a finalizer on the event channel value, so that it calls
`xenevtchn_close` when the value would be GCed.
In practice oxenstored seems to be the only user of this,
and it creates a single global event channel only,
but freeing this could still be useful when run with OCAMLRUNPARAM=c
The code was previously casting a C pointer to an OCaml value,
which should be avoided: OCaml 5.0 won't support it.
(all "naked" C pointers must be wrapped inside an OCaml value,
either an Abstract tag, or Nativeint, see the manual
Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
On 29/07/2022 18:53, Edwin Török wrote: > Add a finalizer on the event channel value, so that it calls > `xenevtchn_close` when the value would be GCed. > > In practice oxenstored seems to be the only user of this, > and it creates a single global event channel only, > but freeing this could still be useful when run with OCAMLRUNPARAM=c > > The code was previously casting a C pointer to an OCaml value, > which should be avoided: OCaml 5.0 won't support it. > (all "naked" C pointers must be wrapped inside an OCaml value, > either an Abstract tag, or Nativeint, see the manual > https://ocaml.org/manual/intfc.html#ss:c-outside-head) > > Signed-off-by: Edwin Török <edvin.torok@citrix.com> So while this looks like a good improvement, don't we need to do it for all libraries, not just evtchn? It doesn't look as if Ocaml 5.0 is very far away. > --- > tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c > index f889a7a2e4..c0d57e2954 100644 > --- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c > +++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c > @@ -33,7 +33,30 @@ > #include <caml/fail.h> > #include <caml/signals.h> > > -#define _H(__h) ((xenevtchn_handle *)(__h)) > +/* We want to close the event channel when it is no longer in use, > + which can only be done safely with a finalizer. > + Event channels are typically long lived, so we don't need tighter control over resource deallocation. > + Use a custom block > +*/ > + > +/* Access the xenevtchn_t* part of the OCaml custom block */ > +#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h))) > + > +static void stub_evtchn_finalize(value v) > +{ > + /* docs say to not use any CAMLparam* macros here */ > + xenevtchn_close(_H(v)); > +} > + > +static struct custom_operations xenevtchn_ops = { > + "xenevtchn", > + stub_evtchn_finalize, > + custom_compare_default, /* raises Failure, cannot compare */ > + custom_hash_default, /* ignored */ > + custom_serialize_default, /* raises Failure, can't serialize */ > + custom_deserialize_default, /* raises Failure, can't deserialize */ > + custom_compare_ext_default /* raises Failure */ This wants to gain a trailing comma. > +}; > > CAMLprim value stub_eventchn_init(void) > { > @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void) > if (xce == NULL) > caml_failwith("open failed"); > > - result = (value)xce; > + /* contains file descriptors, trigger full GC at least every 128 allocations */ > + result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128); The memory allocated for xce is tiny (48 bytes) and invariant for the lifetime of the evtchn object, which we've already established tends to operate as a singleton anyway. Don't we want to use the recommended 0 and 1 here? ~Andrew
> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 29/07/2022 18:53, Edwin Török wrote: >> Add a finalizer on the event channel value, so that it calls >> `xenevtchn_close` when the value would be GCed. >> >> In practice oxenstored seems to be the only user of this, >> and it creates a single global event channel only, >> but freeing this could still be useful when run with OCAMLRUNPARAM=c >> >> The code was previously casting a C pointer to an OCaml value, >> which should be avoided: OCaml 5.0 won't support it. >> (all "naked" C pointers must be wrapped inside an OCaml value, >> either an Abstract tag, or Nativeint, see the manual >> https://ocaml.org/manual/intfc.html#ss:c-outside-head) >> >> Signed-off-by: Edwin Török <edvin.torok@citrix.com> > > So while this looks like a good improvement, don't we need to do it for > all libraries, not just evtchn? > > It doesn't look as if Ocaml 5.0 is very far away. There are 2 ways to make it safe: use a block with Abstract_tag, or a block with Custom_tag: https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head (technically it only ever was safe to use naked pointers for word-aligned pointers previously, although malloc usually ensures some minimal alignment). There is a mode where the runtime can warn on stderr whenever naked pointers are used (there is an opam switch for it, or one can specify a flag during the compiler build time), but it only does so when that codepath is hit, not at build/link time. A quick look at the libs: * libs/mmap: uses Abstract_tag * eventchn: fixed here to use Custom_tag * libs/xc: needs fixing, stub_xc_interface_open * libs/xl: uses Custom_tag (although it has other known issues (it should never use caml_callbackN directly, but use caml_callbackN_exn, see https://v2.ocaml.org/manual/intfc.html#ss:c-callbacks) * libs/xentoollog: uses Custom_tag So we need a patch for libs/xc. > >> --- >> tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c >> index f889a7a2e4..c0d57e2954 100644 >> --- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c >> +++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c >> @@ -33,7 +33,30 @@ >> #include <caml/fail.h> >> #include <caml/signals.h> >> >> -#define _H(__h) ((xenevtchn_handle *)(__h)) >> +/* We want to close the event channel when it is no longer in use, >> + which can only be done safely with a finalizer. >> + Event channels are typically long lived, so we don't need tighter control over resource deallocation. >> + Use a custom block >> +*/ >> + >> +/* Access the xenevtchn_t* part of the OCaml custom block */ >> +#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h))) >> + >> +static void stub_evtchn_finalize(value v) >> +{ >> + /* docs say to not use any CAMLparam* macros here */ >> + xenevtchn_close(_H(v)); >> +} >> + >> +static struct custom_operations xenevtchn_ops = { >> + "xenevtchn", >> + stub_evtchn_finalize, >> + custom_compare_default, /* raises Failure, cannot compare */ >> + custom_hash_default, /* ignored */ >> + custom_serialize_default, /* raises Failure, can't serialize */ >> + custom_deserialize_default, /* raises Failure, can't deserialize */ >> + custom_compare_ext_default /* raises Failure */ > > This wants to gain a trailing comma. > >> +}; >> >> CAMLprim value stub_eventchn_init(void) >> { >> @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void) >> if (xce == NULL) >> caml_failwith("open failed"); >> >> - result = (value)xce; >> + /* contains file descriptors, trigger full GC at least every 128 allocations */ >> + result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128); > > The memory allocated for xce is tiny (48 bytes) and invariant for the > lifetime of the evtchn object, which we've already established tends to > operate as a singleton anyway. > > Don't we want to use the recommended 0 and 1 here? It is not just about the memory itself, but also about the file descriptors: those are a limited resource, and if we use the 0 and 1 it means that this will be garbage collected very infrequently since the allocation itself is very small, and you could potentially run out of file descriptors if you keep opening new event channels. Notice there is no API for the user to close the event channel, so it has to rely on the GC, which is not ideal. The mirage version of the eventchn lib does provide a close function: https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.mli, although its implementation just leaks it (to avoid use-after-free): https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.ml#L42 Are event channel always expected to be singletons, is there a valid use case where you'd want more than 1 event channel/process? Best regards, --Edwin
On 08/08/2022 09:28, Edwin Torok wrote: >> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >> >> On 29/07/2022 18:53, Edwin Török wrote: >>> +}; >>> >>> CAMLprim value stub_eventchn_init(void) >>> { >>> @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void) >>> if (xce == NULL) >>> caml_failwith("open failed"); >>> >>> - result = (value)xce; >>> + /* contains file descriptors, trigger full GC at least every 128 allocations */ >>> + result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128); >> The memory allocated for xce is tiny (48 bytes) and invariant for the >> lifetime of the evtchn object, which we've already established tends to >> operate as a singleton anyway. >> >> Don't we want to use the recommended 0 and 1 here? > It is not just about the memory itself, but also about the file descriptors: those are a limited resource, > and if we use the 0 and 1 it means that this will be garbage collected very infrequently since the allocation itself is very small, > and you could potentially run out of file descriptors if you keep opening new event channels. > Notice there is no API for the user to close the event channel, so it has to rely on the GC, which is not ideal. > > The mirage version of the eventchn lib does provide a close function: https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.mli, > although its implementation just leaks it (to avoid use-after-free): https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.ml#L42 > > Are event channel always expected to be singletons, is there a valid use case where you'd want more than 1 event channel/process? Careful on terminology. We're discussing an open /dev/xen/evtchn file handle, upon which an arbitrary number of event channels are muliplexed. There's no good reason to have more than one file handle open, and there's no good reason to close it except during shutdown. So 0 and 1 are probably the right values in this case. ~Andrew
diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c index f889a7a2e4..c0d57e2954 100644 --- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c +++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c @@ -33,7 +33,30 @@ #include <caml/fail.h> #include <caml/signals.h> -#define _H(__h) ((xenevtchn_handle *)(__h)) +/* We want to close the event channel when it is no longer in use, + which can only be done safely with a finalizer. + Event channels are typically long lived, so we don't need tighter control over resource deallocation. + Use a custom block +*/ + +/* Access the xenevtchn_t* part of the OCaml custom block */ +#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h))) + +static void stub_evtchn_finalize(value v) +{ + /* docs say to not use any CAMLparam* macros here */ + xenevtchn_close(_H(v)); +} + +static struct custom_operations xenevtchn_ops = { + "xenevtchn", + stub_evtchn_finalize, + custom_compare_default, /* raises Failure, cannot compare */ + custom_hash_default, /* ignored */ + custom_serialize_default, /* raises Failure, can't serialize */ + custom_deserialize_default, /* raises Failure, can't deserialize */ + custom_compare_ext_default /* raises Failure */ +}; CAMLprim value stub_eventchn_init(void) { @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void) if (xce == NULL) caml_failwith("open failed"); - result = (value)xce; + /* contains file descriptors, trigger full GC at least every 128 allocations */ + result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128); + _H(result) = xce; CAMLreturn(result); }
Add a finalizer on the event channel value, so that it calls `xenevtchn_close` when the value would be GCed. In practice oxenstored seems to be the only user of this, and it creates a single global event channel only, but freeing this could still be useful when run with OCAMLRUNPARAM=c The code was previously casting a C pointer to an OCaml value, which should be avoided: OCaml 5.0 won't support it. (all "naked" C pointers must be wrapped inside an OCaml value, either an Abstract tag, or Nativeint, see the manual https://ocaml.org/manual/intfc.html#ss:c-outside-head) Signed-off-by: Edwin Török <edvin.torok@citrix.com> --- tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)