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

[Marcello Seri <marcello.seri@citrix.com>]

On 31/05/18 17:29, Juergen Gross 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.

Unfortunately I don't know exactly what fixed the issue. My belief is that the first
patch fixed it. The second patch contains both changes to use the changes from
the first patch and changes to remove further uses of unsafe conversions.
I could try to test them separately, and see if the first plus a small part of the second is
enough, but I will no longer be able to access the test environment starting from
tomorrow as I am changing jobs and the tests are very slow to run.

>>> 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.

No, I did not know about it. I will setup a machine with these changes
and test it. Thanks! Indeed, xs-test shows a failure on the unpatched
version.

>> 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?

This is not just a cleanup, as it contains changes to adapt to the new xb interface.
I think it would be safer to include this patch anyways because it reduces the
calls to unsafe conversion functions, further reducing the surface for potential
issues.

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