Re: [PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 08/08/2022 09:28, Edwin Torok wrote:
>> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> 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