Re: [Xen-devel] [PATCH for-4.11 0/2] xenstore: reduce use of unsafe conversions

[Christian Lindig <lindig@xxxxxxxxxxxxx>]

As we recently found out in another project with C bindings, problems stemming 
from interaction with the OCaml garbage collector can be very rare and very 
hard to find. So I appreciate to get this right and Marcello (who is sitting 
next to me) has the most experience with this in our group. But if we have 
doubts that this is correct, we maybe should not include this into the next 
release and test the patches for immutable strings (including this series) for 
the release after this. These patches do look good to me.

— Christian



> On 31 May 2018, at 16:29, Juergen Gross <jgross@xxxxxxxx> wrote:
> 
> On 31/05/18 16:49, Andrew Cooper wrote:
>> On 31/05/18 15:14, Juergen Gross wrote:
>>> On 31/05/18 15:05, Marcello Seri wrote:
>>>> When xenstore was updated to support safe-string, some unnecessary
>>>> copies were introduced. A further patch reduced the copies at the price
>>>> of many calls to unsafe conversions between bytes and strings. In the
>>>> port we also did not notice that some C stubs were still incorrectly
>>>> using ocaml strings as mutable payload.
>>>> 
>>>> This set of patches updates the C stubs that use mutable payloads passed
>>>> from ocaml, and reduces the amount of unsafe conversions where possible
>>>> without further increasing the number of copies.
>>>> 
>>>> This seems also to fix some unclear instabilities that appeared after
>>>> the former patch introducing the unsafe conversion with some version of
>>>> the ocaml compiler.
>>> This is rather vague.
>>> 
>>> Can you confirm that oxenstored is now as stable as it was without the
>>> safe-string patches?
>>> 
>>> Could you please mention the commit of the patch you are fixing in the
>>> related commit message? I'd like to know which of the two patches is
>>> the real fix and which is "only" some improvement of code.
>>> 
>>> We are rather close to the release, so I'm hesitating to accept cleanup
>>> patches now.
>> 
>> So far, these changes do appear to have fixed the issues XenRT first
>> noticed.  Unfortunately the failures are very hard to quantify, but seem
>> to amount to "some operations seem to get dropped" (as there is no
>> obvious corruption), but the issues are rare and takes an large quantity
>> of machine hours to encounter.
> 
> Did you try multiple concurrent runs of "xs-test -r <seconds>" to
> reproduce the problem? This has helped me a lot to find problems in
> xenstore.
> 
>> I can't comment for exact changes, but the bug being fixed by patch 1 is
>> not passing a buffer (which the Ocaml runtime thinks is immutable) to
>> the C stubs to be written into.  AFAICT, it is consequence of the very
>> first attempt to move from mutable to immutable strings.
> 
> So patch 2 is more kind of a cleanup? Or is it needed for the fix, too?
> 
> 
> Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel