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

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

[Marcello Seri]

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.

Marcello Seri (2):
  ocaml/libs/xb: use bytes in place of strings for mutable buffers
  ocaml/xenstored: reduce use of unsafe conversions

 tools/ocaml/libs/xb/xb.ml        | 12 +++++-------
 tools/ocaml/libs/xb/xb.mli       |  2 +-
 tools/ocaml/libs/xb/xs_ring.ml   | 12 +++++++-----
 tools/ocaml/xenstored/logging.ml | 16 ++++++----------
 tools/ocaml/xenstored/stdext.ml  |  2 +-
 tools/ocaml/xenstored/utils.ml   |  9 ++++-----
 6 files changed, 24 insertions(+), 29 deletions(-)

-- 
2.17.0


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

[PATCH for-4.11 1/2] ocaml/libs/xb: use bytes in place of strings for mutable buffers

[Marcello Seri]

Since ocaml 4.06.0, that made safe-string on by default, the compiler is
allowed to perform optimisations on immutable strings.  They should no
longer be used as mutable buffers, and bytes should be used instead. The
c stubs for Xs_ring have been updated to use bytes, and the interface
rationalised mimicking the new Unix module in the standard library (the
implementation of Unix.write_substring uses unsafe_of_string in the
exact same way, and both the write implementations are using the bytes
as an immutable payload for the write).

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
---
 tools/ocaml/libs/xb/xb.ml      | 12 +++++-------
 tools/ocaml/libs/xb/xb.mli     |  2 +-
 tools/ocaml/libs/xb/xs_ring.ml | 12 +++++++-----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 660224f895..ca738657df 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -76,9 +76,9 @@ let read_fd back con b len =
 	rd
 
 let read_mmap back con b len =
-	let s = String.make len (char_of_int 0) in
+	let s = Bytes.make len '\000' in
 	let rd = Xs_ring.read back.mmap s len in
-	Bytes.blit_string s 0 b 0 rd;
+	Bytes.blit s 0 b 0 rd;
 	back.work_again <- (rd > 0);
 	if rd > 0 then
 		back.eventchn_notify ();
@@ -90,19 +90,17 @@ let read con b len =
 	| Xenmmap backmmap -> read_mmap backmmap con b len
 
 let write_fd back con b len =
-	Unix.write back.fd b 0 len
+	Unix.write_substring back.fd b 0 len
 
 let write_mmap back con s len =
-	let ws = Xs_ring.write back.mmap s len in
+	let ws = Xs_ring.write_substring back.mmap s len in
 	if ws > 0 then
 		back.eventchn_notify ();
 	ws
 
 let write con s len =
 	match con.backend with
-	(* we can use unsafe_of_string here as the bytes are used immutably
-	   in the Unix.write operation. *)
-	| Fd backfd     -> write_fd backfd con (Bytes.unsafe_of_string s) len
+	| Fd backfd     -> write_fd backfd con s len
 	| Xenmmap backmmap -> write_mmap backmmap con s len
 
 (* NB: can throw Reconnect *)
diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
index d566011fc7..3a00da6cdd 100644
--- a/tools/ocaml/libs/xb/xb.mli
+++ b/tools/ocaml/libs/xb/xb.mli
@@ -79,7 +79,7 @@ val queue : t -> Packet.t -> unit
 val read_fd : backend_fd -> 'a -> bytes -> int -> int
 val read_mmap : backend_mmap -> 'a -> bytes -> int -> int
 val read : t -> bytes -> int -> int
-val write_fd : backend_fd -> 'a -> bytes -> int -> int
+val write_fd : backend_fd -> 'a -> string -> int -> int
 val write_mmap : backend_mmap -> 'a -> string -> int -> int
 val write : t -> string -> int -> int
 val output : t -> bool
diff --git a/tools/ocaml/libs/xb/xs_ring.ml b/tools/ocaml/libs/xb/xs_ring.ml
index 48e06f4cbf..db7f86bd27 100644
--- a/tools/ocaml/libs/xb/xs_ring.ml
+++ b/tools/ocaml/libs/xb/xs_ring.ml
@@ -24,12 +24,14 @@ module Server_features = Set.Make(struct
 	let compare = compare
 end)
 
-external read: Xenmmap.mmap_interface -> string -> int -> int = "ml_interface_read"
-external write: Xenmmap.mmap_interface -> string -> int -> int = "ml_interface_write"
+external read: Xenmmap.mmap_interface -> bytes -> int -> int = "ml_interface_read"
+external write: Xenmmap.mmap_interface -> bytes -> int -> int = "ml_interface_write"
 
-external _internal_set_server_features: Xenmmap.mmap_interface -> int -> unit = "ml_interface_set_server_features" "noalloc"
-external _internal_get_server_features: Xenmmap.mmap_interface -> int = "ml_interface_get_server_features" "noalloc"
+external _internal_set_server_features: Xenmmap.mmap_interface -> int -> unit = "ml_interface_set_server_features" [@@noalloc]
+external _internal_get_server_features: Xenmmap.mmap_interface -> int = "ml_interface_get_server_features" [@@noalloc]
 
+let write_substring mmap buff len =
+	write mmap (Bytes.unsafe_of_string buff) len
 
 let get_server_features mmap =
 	(* NB only one feature currently defined above *)
@@ -43,4 +45,4 @@ let set_server_features mmap set =
 	let x = if set = Server_features.empty then 0 else 1 in
 	_internal_set_server_features mmap x
 
-external close: Xenmmap.mmap_interface -> unit = "ml_interface_close" "noalloc"
+external close: Xenmmap.mmap_interface -> unit = "ml_interface_close" [@@noalloc]
-- 
2.17.0


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

[PATCH for-4.11 2/2] ocaml/xenstored: reduce use of unsafe conversions

[Marcello Seri]

The rationalisation of the Xs_ring interface in the xb library
allows to further reduce the unsafe calls withouth introducing
copies. This patch also contains some further code cleanups.

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
---
 tools/ocaml/xenstored/logging.ml | 16 ++++++----------
 tools/ocaml/xenstored/stdext.ml  |  2 +-
 tools/ocaml/xenstored/utils.ml   |  9 ++++-----
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 45a2c222e6..ea6033195d 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -252,13 +252,11 @@ let string_of_access_type = function
 	*)
 
 let sanitize_data data =
-	let data = Bytes.copy data in
-	for i = 0 to Bytes.length data - 1
-	do
-		if Bytes.get data i = '\000' then
-			Bytes.set data i ' '
-	done;
-	String.escaped (Bytes.unsafe_to_string data)
+	let data = String.init
+		(String.length data)
+		(fun i -> let c = data.[i] in if c = '\000' then ' ' else c)
+	in
+	String.escaped data
 
 let activate_access_log = ref true
 let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
@@ -291,9 +289,7 @@ let access_logging ~con ~tid ?(data="") ~level access_type =
 				let date = string_of_date() in
 				let tid = string_of_tid ~con tid in
 				let access_type = string_of_access_type access_type in
-				(* we can use unsafe_of_string here as the sanitize_data function
-				   immediately makes a copy of the data and operates on that. *)
-				let data = sanitize_data (Bytes.unsafe_of_string data) in
+				let data = sanitize_data data in
 				let prefix = prefix !access_log_destination date in
 				let msg = Printf.sprintf "%s %s %s %s" prefix tid access_type data in
 				logger.write ~level msg)
diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
index 869fec36f2..305a330aa5 100644
--- a/tools/ocaml/xenstored/stdext.ml
+++ b/tools/ocaml/xenstored/stdext.ml
@@ -122,7 +122,7 @@ let pidfile_write filename =
 		let pid = Unix.getpid () in
 		let buf = string_of_int pid ^ "\n" in
 		let len = String.length buf in
-		if Unix.write fd (Bytes.unsafe_of_string buf) 0 len <> len
+		if Unix.write_substring fd buf 0 len <> len
 		then failwith "pidfile_write failed";
 	)
 	(fun () -> Unix.close fd)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index 73affb7ea4..b252db799b 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -46,12 +46,11 @@ let get_hierarchy path =
 let hexify s =
 	let hexseq_of_char c = sprintf "%02x" (Char.code c) in
 	let hs = Bytes.create (String.length s * 2) in
-	for i = 0 to String.length s - 1
-	do
-		let seq = hexseq_of_char s.[i] in
+	String.iteri (fun i c ->
+		let seq = hexseq_of_char c in
 		Bytes.set hs (i * 2) seq.[0];
 		Bytes.set hs (i * 2 + 1) seq.[1];
-	done;
+	) s;
 	Bytes.unsafe_to_string hs
 
 let unhexify hs =
@@ -84,7 +83,7 @@ let create_unix_socket name =
 
 let read_file_single_integer filename =
 	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
-	let buf = Bytes.make 20 (char_of_int 0) in
+	let buf = Bytes.make 20 '\000' in
 	let sz = Unix.read fd buf 0 20 in
 	Unix.close fd;
 	int_of_string (Bytes.sub_string buf 0 sz)
-- 
2.17.0


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

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

[Juergen Gross]

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.


Juergen

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

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

[Andrew Cooper]

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.

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.

~Andrew

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

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

[Juergen Gross]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Christian Lindig]

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@suse.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Marcello Seri]

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

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

[Juergen Gross]

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.
> 
> Marcello Seri (2):
>   ocaml/libs/xb: use bytes in place of strings for mutable buffers
>   ocaml/xenstored: reduce use of unsafe conversions

For the series:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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