Some oxenstored improvements

Some oxenstored improvements

[Zheng Li]

It's mainly about removing the 1024 fds limitation in the current oxenstored
implementation. We also fixed some bugs and made some perf improvements along
the way.

[PATCH 1/8] oxenstored: add a poll-based select mechanism

[Zheng Li]

Currently, oxenstored uses Unix.select underneath, so it doesn't work properly
if given a FD number >= 1024. This is a scalability bottleneck for hosts
running large number of VMs.

To remove this limitation, we implemented a poll-based mechanism but with the
same type signature as the Unix.select currently in use. So these two functions
can be interchangeable at any stage.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/Makefile       |  9 +++--
 tools/ocaml/xenstored/select.ml      | 49 ++++++++++++++++++++++++++
 tools/ocaml/xenstored/select.mli     | 20 +++++++++++
 tools/ocaml/xenstored/select_stubs.c | 68 ++++++++++++++++++++++++++++++++++++
 tools/ocaml/xenstored/xenstored.ml   |  2 +-
 5 files changed, 144 insertions(+), 4 deletions(-)
 create mode 100644 tools/ocaml/xenstored/select.ml
 create mode 100644 tools/ocaml/xenstored/select.mli
 create mode 100644 tools/ocaml/xenstored/select_stubs.c

diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 068e04a..47d5303 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -15,10 +15,12 @@ OCAMLINCLUDE += \
 	-I $(OCAML_TOPLEVEL)/libs/xc \
 	-I $(OCAML_TOPLEVEL)/libs/eventchn
 
-LIBS = syslog.cma syslog.cmxa
+LIBS = syslog.cma syslog.cmxa select.cma select.cmxa
 syslog_OBJS = syslog
 syslog_C_OBJS = syslog_stubs
-OCAML_LIBRARY = syslog
+select_OBJS = select
+select_C_OBJS = select_stubs
+OCAML_LIBRARY = syslog select
 
 LIBS += systemd.cma systemd.cmxa
 systemd_OBJS = systemd
@@ -46,12 +48,13 @@ OBJS = define \
 	process \
 	xenstored
 
-INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi
+INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi select.cmi
 
 XENSTOREDLIBS = \
 	unix.cmxa \
 	-ccopt -L -ccopt . syslog.cmxa \
 	-ccopt -L -ccopt . systemd.cmxa \
+	-ccopt -L -ccopt . select.cmxa \
 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/mmap $(OCAML_TOPLEVEL)/libs/mmap/xenmmap.cmxa \
 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/eventchn $(OCAML_TOPLEVEL)/libs/eventchn/xeneventchn.cmxa \
 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/xc $(OCAML_TOPLEVEL)/libs/xc/xenctrl.cmxa \
diff --git a/tools/ocaml/xenstored/select.ml b/tools/ocaml/xenstored/select.ml
new file mode 100644
index 0000000..2c18c70
--- /dev/null
+++ b/tools/ocaml/xenstored/select.ml
@@ -0,0 +1,49 @@
+(*
+ * Copyright (C) 2014 Zheng Li <dev@zheng.li>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *)
+
+
+type event = {
+	mutable read: bool;
+	mutable write: bool;
+	mutable except: bool;
+}
+
+external select_on_poll: (Unix.file_descr * event) array -> int -> int = "stub_select_on_poll"
+
+let init_event () = {read = false; write = false; except = false}
+
+let select in_fds out_fds exc_fds timeout =
+	let h = Hashtbl.create 57 in
+	let add_event event_set fd =
+		let e =
+			try Hashtbl.find h fd
+			with Not_found ->
+				let e = init_event () in
+				Hashtbl.add h fd e; e in
+		event_set e in
+	List.iter (add_event (fun x -> x.read <- true)) in_fds;
+	List.iter (add_event (fun x -> x.write <- true)) out_fds;
+	List.iter (add_event (fun x -> x.except <- true)) exc_fds;
+	let a = Array.make (Hashtbl.length h) (Unix.stdin, init_event ()) in
+	let i = ref (-1) in
+	Hashtbl.iter (fun fd event -> incr i; Array.set a !i (fd, event)) h;
+	let n = select_on_poll a (int_of_float (timeout *. 1000.)) in
+	let r = [], [], [] in
+	if n = 0 then r else
+		Array.fold_right
+			(fun (fd, event) (r, w, x) ->
+			 (if event.read then fd :: r else r),
+			 (if event.write then fd :: w else w),
+			 (if event.except then fd :: x else x))
+			a r
diff --git a/tools/ocaml/xenstored/select.mli b/tools/ocaml/xenstored/select.mli
new file mode 100644
index 0000000..1253d4e
--- /dev/null
+++ b/tools/ocaml/xenstored/select.mli
@@ -0,0 +1,20 @@
+(*
+ * Copyright (C) 2014 Zheng Li <dev@zheng.li>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *)
+
+
+(** Same interface and semantics as [Unix.select] but with an extra alternative
+    implementation based on poll. *)
+val select:
+	Unix.file_descr list -> Unix.file_descr list -> Unix.file_descr list -> float
+	-> Unix.file_descr list * Unix.file_descr list * Unix.file_descr list
diff --git a/tools/ocaml/xenstored/select_stubs.c b/tools/ocaml/xenstored/select_stubs.c
new file mode 100644
index 0000000..a50f417
--- /dev/null
+++ b/tools/ocaml/xenstored/select_stubs.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2014 Zheng Li <dev@zheng.li>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include <poll.h>
+#include <errno.h>
+#include <sys/resource.h>
+#include <unistd.h>
+#include <caml/mlvalues.h>
+#include <caml/memory.h>
+#include <caml/fail.h>
+#include <caml/alloc.h>
+#include <caml/signals.h>
+#include <caml/unixsupport.h>
+
+CAMLprim value stub_select_on_poll(value fd_events, value timeo) {
+
+	CAMLparam2(fd_events, timeo);
+	CAMLlocal1(events);
+	int i, rc, c_len = Wosize_val(fd_events), c_timeo = Int_val(timeo);
+	struct pollfd c_fds[c_len];	
+
+
+	for (i = 0; i < c_len; i++) {
+
+		events = Field(Field(fd_events, i), 1);
+
+		c_fds[i].fd = Int_val(Field(Field(fd_events, i), 0));
+		c_fds[i].events = c_fds[i].revents = 0;
+		c_fds[i].events |= Bool_val(Field(events, 0)) ? POLLIN : 0;
+		c_fds[i].events |= Bool_val(Field(events, 1)) ? POLLOUT: 0;
+		c_fds[i].events |= Bool_val(Field(events, 2)) ? POLLPRI: 0;
+
+	};
+
+	caml_enter_blocking_section();
+	rc = poll(c_fds, c_len, c_timeo);
+	caml_leave_blocking_section();
+
+	if (rc < 0) uerror("select", Nothing);
+
+	if (rc > 0) {
+
+		for (i = 0; i < c_len; i++) {
+
+			events = Field(Field(fd_events, i), 1);
+
+			if (c_fds[i].revents & POLLNVAL) unix_error(EBADF, "select", Nothing);
+			Field(events, 0) = Val_bool(c_fds[i].events | POLLIN  && c_fds[i].revents & (POLLIN |POLLHUP|POLLERR));
+			Field(events, 1) = Val_bool(c_fds[i].events | POLLOUT && c_fds[i].revents & (POLLOUT|POLLHUP|POLLERR));
+			Field(events, 2) = Val_bool(c_fds[i].revents & POLLPRI);
+			
+		}
+
+	}
+
+	CAMLreturn(Val_int(rc));
+}
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 1c02f2f..bfa488f 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -368,7 +368,7 @@ let _ =
 		let timeout = if List.length mw > 0 then 0. else -1. in
 		let rset, wset, _ =
 		try
-			Unix.select (spec_fds @ inset) outset [] timeout
+			Select.select (spec_fds @ inset) outset [] timeout
 		with Unix.Unix_error(Unix.EINTR, _, _) ->
 			[], [], [] in
 		let sfds, cfds =
-- 
2.1.0

Re: [PATCH 1/8] oxenstored: add a poll-based select mechanism

[David Scott]

Hi,

Thanks for sending these.

On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:

> Currently, oxenstored uses Unix.select underneath, so it doesn't work properly
> if given a FD number >= 1024. This is a scalability bottleneck for hosts
> running large number of VMs.
> 
> To remove this limitation, we implemented a poll-based mechanism but with the
> same type signature as the Unix.select currently in use. So these two functions
> can be interchangeable at any stage.
> 
> Signed-off-by: Zheng Li <dev@zheng.li>
> ---
> tools/ocaml/xenstored/Makefile       |  9 +++--
> tools/ocaml/xenstored/select.ml      | 49 ++++++++++++++++++++++++++
> tools/ocaml/xenstored/select.mli     | 20 +++++++++++
> tools/ocaml/xenstored/select_stubs.c | 68 ++++++++++++++++++++++++++++++++++++
> tools/ocaml/xenstored/xenstored.ml   |  2 +-
> 5 files changed, 144 insertions(+), 4 deletions(-)
> create mode 100644 tools/ocaml/xenstored/select.ml
> create mode 100644 tools/ocaml/xenstored/select.mli
> create mode 100644 tools/ocaml/xenstored/select_stubs.c
> 
> diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
> index 068e04a..47d5303 100644
> --- a/tools/ocaml/xenstored/Makefile
> +++ b/tools/ocaml/xenstored/Makefile
> @@ -15,10 +15,12 @@ OCAMLINCLUDE += \
> 	-I $(OCAML_TOPLEVEL)/libs/xc \
> 	-I $(OCAML_TOPLEVEL)/libs/eventchn
> 
> -LIBS = syslog.cma syslog.cmxa
> +LIBS = syslog.cma syslog.cmxa select.cma select.cmxa
> syslog_OBJS = syslog
> syslog_C_OBJS = syslog_stubs
> -OCAML_LIBRARY = syslog
> +select_OBJS = select
> +select_C_OBJS = select_stubs
> +OCAML_LIBRARY = syslog select
> 
> LIBS += systemd.cma systemd.cmxa
> systemd_OBJS = systemd
> @@ -46,12 +48,13 @@ OBJS = define \
> 	process \
> 	xenstored
> 
> -INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi
> +INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi select.cmi
> 
> XENSTOREDLIBS = \
> 	unix.cmxa \
> 	-ccopt -L -ccopt . syslog.cmxa \
> 	-ccopt -L -ccopt . systemd.cmxa \
> +	-ccopt -L -ccopt . select.cmxa \
> 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/mmap $(OCAML_TOPLEVEL)/libs/mmap/xenmmap.cmxa \
> 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/eventchn $(OCAML_TOPLEVEL)/libs/eventchn/xeneventchn.cmxa \
> 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/xc $(OCAML_TOPLEVEL)/libs/xc/xenctrl.cmxa \
> diff --git a/tools/ocaml/xenstored/select.ml b/tools/ocaml/xenstored/select.ml
> new file mode 100644
> index 0000000..2c18c70
> --- /dev/null
> +++ b/tools/ocaml/xenstored/select.ml
> @@ -0,0 +1,49 @@
> +(*
> + * Copyright (C) 2014 Zheng Li <dev@zheng.li>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *)
> +
> +
> +type event = {
> +	mutable read: bool;
> +	mutable write: bool;
> +	mutable except: bool;
> +}

If I understand correctly this is a representation of the POLL{IN,OUT,PRI} bitmap in the C stubs — might be worth a short comment (to help me remember in future)

> +
> +external select_on_poll: (Unix.file_descr * event) array -> int -> int = "stub_select_on_poll"
> +
> +let init_event () = {read = false; write = false; except = false}
> +
> +let select in_fds out_fds exc_fds timeout =
> +	let h = Hashtbl.create 57 in
> +	let add_event event_set fd =
> +		let e =
> +			try Hashtbl.find h fd
> +			with Not_found ->
> +				let e = init_event () in
> +				Hashtbl.add h fd e; e in
> +		event_set e in
> +	List.iter (add_event (fun x -> x.read <- true)) in_fds;
> +	List.iter (add_event (fun x -> x.write <- true)) out_fds;
> +	List.iter (add_event (fun x -> x.except <- true)) exc_fds;
> +	let a = Array.make (Hashtbl.length h) (Unix.stdin, init_event ()) in

Might be worth a comment to say that Unix.stdin is a dummy value, since all entries in the array are overwritten below.

> +	let i = ref (-1) in
> +	Hashtbl.iter (fun fd event -> incr i; Array.set a !i (fd, event)) h;
> +	let n = select_on_poll a (int_of_float (timeout *. 1000.)) in
> +	let r = [], [], [] in
> +	if n = 0 then r else
> +		Array.fold_right
> +			(fun (fd, event) (r, w, x) ->
> +			 (if event.read then fd :: r else r),
> +			 (if event.write then fd :: w else w),
> +			 (if event.except then fd :: x else x))
> +			a r
> diff --git a/tools/ocaml/xenstored/select.mli b/tools/ocaml/xenstored/select.mli
> new file mode 100644
> index 0000000..1253d4e
> --- /dev/null
> +++ b/tools/ocaml/xenstored/select.mli
> @@ -0,0 +1,20 @@
> +(*
> + * Copyright (C) 2014 Zheng Li <dev@zheng.li>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *)
> +
> +
> +(** Same interface and semantics as [Unix.select] but with an extra alternative
> +    implementation based on poll. *)
> +val select:
> +	Unix.file_descr list -> Unix.file_descr list -> Unix.file_descr list -> float
> +	-> Unix.file_descr list * Unix.file_descr list * Unix.file_descr list
> diff --git a/tools/ocaml/xenstored/select_stubs.c b/tools/ocaml/xenstored/select_stubs.c
> new file mode 100644
> index 0000000..a50f417
> --- /dev/null
> +++ b/tools/ocaml/xenstored/select_stubs.c
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (C) 2014 Zheng Li <dev@zheng.li>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include <poll.h>
> +#include <errno.h>
> +#include <sys/resource.h>
> +#include <unistd.h>
> +#include <caml/mlvalues.h>
> +#include <caml/memory.h>
> +#include <caml/fail.h>
> +#include <caml/alloc.h>
> +#include <caml/signals.h>
> +#include <caml/unixsupport.h>
> +
> +CAMLprim value stub_select_on_poll(value fd_events, value timeo) {
> +
> +	CAMLparam2(fd_events, timeo);
> +	CAMLlocal1(events);
> +	int i, rc, c_len = Wosize_val(fd_events), c_timeo = Int_val(timeo);
> +	struct pollfd c_fds[c_len];	
> +
> +
> +	for (i = 0; i < c_len; i++) {
> +
> +		events = Field(Field(fd_events, i), 1);
> +
> +		c_fds[i].fd = Int_val(Field(Field(fd_events, i), 0));
> +		c_fds[i].events = c_fds[i].revents = 0;
> +		c_fds[i].events |= Bool_val(Field(events, 0)) ? POLLIN : 0;
> +		c_fds[i].events |= Bool_val(Field(events, 1)) ? POLLOUT: 0;
> +		c_fds[i].events |= Bool_val(Field(events, 2)) ? POLLPRI: 0;
> +
> +	};
> +
> +	caml_enter_blocking_section();
> +	rc = poll(c_fds, c_len, c_timeo);
> +	caml_leave_blocking_section();
> +
> +	if (rc < 0) uerror("select", Nothing);

Perhaps the error string should be “poll” so it would be obvious from the Unix_error which implementation failed?

> +
> +	if (rc > 0) {
> +
> +		for (i = 0; i < c_len; i++) {
> +
> +			events = Field(Field(fd_events, i), 1);
> +
> +			if (c_fds[i].revents & POLLNVAL) unix_error(EBADF, "select", Nothing);
> +			Field(events, 0) = Val_bool(c_fds[i].events | POLLIN  && c_fds[i].revents & (POLLIN |POLLHUP|POLLERR));
> +			Field(events, 1) = Val_bool(c_fds[i].events | POLLOUT && c_fds[i].revents & (POLLOUT|POLLHUP|POLLERR));
> +			Field(events, 2) = Val_bool(c_fds[i].revents & POLLPRI);
> +			
> +		}
> +
> +	}
> +
> +	CAMLreturn(Val_int(rc));
> +}
> diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
> index 1c02f2f..bfa488f 100644
> --- a/tools/ocaml/xenstored/xenstored.ml
> +++ b/tools/ocaml/xenstored/xenstored.ml
> @@ -368,7 +368,7 @@ let _ =
> 		let timeout = if List.length mw > 0 then 0. else -1. in
> 		let rset, wset, _ =
> 		try
> -			Unix.select (spec_fds @ inset) outset [] timeout
> +			Select.select (spec_fds @ inset) outset [] timeout
> 		with Unix.Unix_error(Unix.EINTR, _, _) ->
> 			[], [], [] in
> 		let sfds, cfds =
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/8] oxenstored: add a poll-based select mechanism

[Zheng Li]

On 16/09/2014 10:01, David Scott wrote:
>> +type event = {
>> +	mutable read: bool;
>> +	mutable write: bool;
>> +	mutable except: bool;
>> +}
>
> If I understand correctly this is a representation of the POLL{IN,OUT,PRI} bitmap in the C stubs — might be worth a short comment (to help me remember in future)

Yes, it also corresponds to the read/write/except fd_set as in the existing Unix.select.

It's currently an internal type and not exposed by the module interface, but I agree a comment would be useful to developers.  I will add the in v2.

>> +	let a = Array.make (Hashtbl.length h) (Unix.stdin, init_event ()) in
>
> Might be worth a comment to say that Unix.stdin is a dummy value, since all entries in the array are overwritten below.
>

Will do in v2.

>> +	if (rc < 0) uerror("select", Nothing);
>
> Perhaps the error string should be “poll” so it would be obvious from the Unix_error which implementation failed?
>

Initially it was implemented as a faithful in-place substitution of Unix.select (i.e. same type signature, same semantics, except the 1024 fd limitation), as such we even kept the exception message exactly the same.

Now think twice, I agree with you it's better to be different, especially if we consider to move to a type signature/implementation more natural to poll in the future version (for some performance advantages).

Cheers,
Zheng

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/8] oxenstored: add facilities to raise the max open fds uplimit

[Zheng Li]

To go beyond 1024 fds, we also need to raise the process limitation on max
open fds (usually defaults to 1024).

We need to know the system level max open fds so that we won't go above that.
Simply setting the limit to RLIM_INFINITY doesn't work on Linux 3.x (EPERM), a
patch on this went into the 2.x branch but not 3.x for some reason. Also,
getting the system level nr_open is not very straightfoward on 3.x, which is
mentioned in the comment inline.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/select.ml      | 15 +++++++++++++++
 tools/ocaml/xenstored/select_stubs.c | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/tools/ocaml/xenstored/select.ml b/tools/ocaml/xenstored/select.ml
index 2c18c70..c19df41 100644
--- a/tools/ocaml/xenstored/select.ml
+++ b/tools/ocaml/xenstored/select.ml
@@ -20,6 +20,21 @@ type event = {
 }
 
 external select_on_poll: (Unix.file_descr * event) array -> int -> int = "stub_select_on_poll"
+external set_fd_limit: int -> unit = "stub_set_fd_limit"
+
+(* The rlim_max given to setrlimit must not beyond system level nr_open, which
+   was defined as NR_OPEN in fs.h in Linux 2.x and then sysctl_nr_open variable
+   since 3.x. Unfortunately sysctl_nr_open was not exposed in either head files
+   or sysctl param vector. This function workarounds this by trying to read
+   from /proc directly and default to the old NR_OPEN value if that fails.
+   Fortunately, we'll need to call this at most once.
+*)
+let get_sys_fs_nr_open () =
+	try
+		let ch = open_in "/proc/sys/fs/nr_open" in
+		let v = int_of_string (input_line ch) in
+		close_in_noerr ch; v
+	with _ -> 1024 * 1024
 
 let init_event () = {read = false; write = false; except = false}
 
diff --git a/tools/ocaml/xenstored/select_stubs.c b/tools/ocaml/xenstored/select_stubs.c
index a50f417..13efbac 100644
--- a/tools/ocaml/xenstored/select_stubs.c
+++ b/tools/ocaml/xenstored/select_stubs.c
@@ -66,3 +66,15 @@ CAMLprim value stub_select_on_poll(value fd_events, value timeo) {
 
 	CAMLreturn(Val_int(rc));
 }
+
+
+CAMLprim value stub_set_fd_limit(value limit) {
+
+	CAMLparam1(limit);
+	struct rlimit rl;
+
+	rl.rlim_cur = rl.rlim_max = Int_val(limit);
+	if (setrlimit(RLIMIT_NOFILE, &rl) != 0) uerror("setrlimit", Nothing);
+	CAMLreturn(Val_unit);
+
+}
-- 
2.1.0

Re: [PATCH 2/8] oxenstored: add facilities to raise the max open fds uplimit

[Dave Scott]

On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:

> To go beyond 1024 fds, we also need to raise the process limitation on max
> open fds (usually defaults to 1024).
> 
> We need to know the system level max open fds so that we won't go above that.
> Simply setting the limit to RLIM_INFINITY doesn't work on Linux 3.x (EPERM), a
> patch on this went into the 2.x branch but not 3.x for some reason. Also,
> getting the system level nr_open is not very straightfoward on 3.x, which is
> mentioned in the comment inline.
> 
> Signed-off-by: Zheng Li <dev@zheng.li>
> ---
> tools/ocaml/xenstored/select.ml      | 15 +++++++++++++++
> tools/ocaml/xenstored/select_stubs.c | 12 ++++++++++++
> 2 files changed, 27 insertions(+)
> 
> diff --git a/tools/ocaml/xenstored/select.ml b/tools/ocaml/xenstored/select.ml
> index 2c18c70..c19df41 100644
> --- a/tools/ocaml/xenstored/select.ml
> +++ b/tools/ocaml/xenstored/select.ml
> @@ -20,6 +20,21 @@ type event = {
> }
> 
> external select_on_poll: (Unix.file_descr * event) array -> int -> int = "stub_select_on_poll"
> +external set_fd_limit: int -> unit = "stub_set_fd_limit"
> +
> +(* The rlim_max given to setrlimit must not beyond system level nr_open, which
> +   was defined as NR_OPEN in fs.h in Linux 2.x and then sysctl_nr_open variable
> +   since 3.x. Unfortunately sysctl_nr_open was not exposed in either head files

typo “header files”

> +   or sysctl param vector. This function workarounds this by trying to read
> +   from /proc directly and default to the old NR_OPEN value if that fails.
> +   Fortunately, we'll need to call this at most once.
> +*)
> +let get_sys_fs_nr_open () =
> +	try
> +		let ch = open_in "/proc/sys/fs/nr_open" in
> +		let v = int_of_string (input_line ch) in
> +		close_in_noerr ch; v
> +	with _ -> 1024 * 1024
> 
> let init_event () = {read = false; write = false; except = false}
> 
> diff --git a/tools/ocaml/xenstored/select_stubs.c b/tools/ocaml/xenstored/select_stubs.c
> index a50f417..13efbac 100644
> --- a/tools/ocaml/xenstored/select_stubs.c
> +++ b/tools/ocaml/xenstored/select_stubs.c
> @@ -66,3 +66,15 @@ CAMLprim value stub_select_on_poll(value fd_events, value timeo) {
> 
> 	CAMLreturn(Val_int(rc));
> }
> +
> +
> +CAMLprim value stub_set_fd_limit(value limit) {
> +
> +	CAMLparam1(limit);
> +	struct rlimit rl;
> +
> +	rl.rlim_cur = rl.rlim_max = Int_val(limit);
> +	if (setrlimit(RLIMIT_NOFILE, &rl) != 0) uerror("setrlimit", Nothing);
> +	CAMLreturn(Val_unit);
> +
> +}
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2/8] oxenstored: add facilities to raise the max open fds uplimit

[David Vrabel]

On 15/09/14 23:39, Zheng Li wrote:
> To go beyond 1024 fds, we also need to raise the process limitation on max
> open fds (usually defaults to 1024).
> 
> We need to know the system level max open fds so that we won't go above that.
> Simply setting the limit to RLIM_INFINITY doesn't work on Linux 3.x (EPERM), a
> patch on this went into the 2.x branch but not 3.x for some reason. Also,
> getting the system level nr_open is not very straightfoward on 3.x, which is
> mentioned in the comment inline.

>From sysctl(2):

       "Since Linux 2.6.24, uses of [the sysctl] system call result in
        warnings in the kernel log.   Remove  it  from  your
        programs now; use the /proc/sys interface instead."

So using /proc/sys is the correct method and not some awkward workaround.

David

Re: [PATCH 2/8] oxenstored: add facilities to raise the max open fds uplimit

[Zheng Li]

Hi David,

On 16/09/2014 10:38, David Vrabel wrote:
> On 15/09/14 23:39, Zheng Li wrote:
>> We need to know the system level max open fds so that we won't go above that.
>> Simply setting the limit to RLIM_INFINITY doesn't work on Linux 3.x (EPERM), a
>> patch on this went into the 2.x branch but not 3.x for some reason. Also,
>> getting the system level nr_open is not very straightfoward on 3.x, which is
>> mentioned in the comment inline.
>
>  From sysctl(2):
>
>         "Since Linux 2.6.24, uses of [the sysctl] system call result in
>          warnings in the kernel log.   Remove  it  from  your
>          programs now; use the /proc/sys interface instead."
>
> So using /proc/sys is the correct method and not some awkward workaround.
>

Good to know that, in which case I can remove majority words of the inline comment.

Will do in v2 (and reword the changeset summary).

Thanks,
Zheng

[PATCH 3/8] oxenstored: add a --use-select command line flag

[Zheng Li]

This allows to fall back to the original Unix.select if preferred. It could be
useful for debugging purposes too.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/parse_arg.ml |  8 ++++++--
 tools/ocaml/xenstored/select.ml    | 15 ++++++++++++++-
 tools/ocaml/xenstored/select.mli   |  9 ++++++++-
 tools/ocaml/xenstored/xenstored.ml |  2 ++
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/tools/ocaml/xenstored/parse_arg.ml b/tools/ocaml/xenstored/parse_arg.ml
index 5d21601..6e22c16 100644
--- a/tools/ocaml/xenstored/parse_arg.ml
+++ b/tools/ocaml/xenstored/parse_arg.ml
@@ -25,6 +25,7 @@ type config =
 	tracefile: string option; (* old xenstored compatibility *)
 	restart: bool;
 	disable_socket: bool;
+	use_select: bool;
 }
 
 let do_argv =
@@ -35,7 +36,8 @@ let do_argv =
 	and reraise_top_level = ref false
 	and config_file = ref ""
 	and restart = ref false
-	and disable_socket = ref false in
+	and disable_socket = ref false
+	and use_select = ref false in
 
 	let speclist =
 		[ ("--no-domain-init", Arg.Unit (fun () -> domain_init := false),
@@ -52,8 +54,9 @@ let do_argv =
 		  ("-T", Arg.Set_string tracefile, ""); (* for compatibility *)
 		  ("--restart", Arg.Set restart, "Read database on starting");
 		  ("--disable-socket", Arg.Unit (fun () -> disable_socket := true), "Disable socket");
+		  ("--use-select", Arg.Unit (fun () -> use_select := true), "Use select instead of poll"); (* for backward compatibility and testing *)
 		] in
-	let usage_msg = "usage : xenstored [--config-file <filename>] [--no-domain-init] [--help] [--no-fork] [--reraise-top-level] [--restart] [--disable-socket]" in
+	let usage_msg = "usage : xenstored [--config-file <filename>] [--no-domain-init] [--help] [--no-fork] [--reraise-top-level] [--restart] [--disable-socket] [--use-select]" in
 	Arg.parse speclist (fun s -> ()) usage_msg;
 	{
 		domain_init = !domain_init;
@@ -65,4 +68,5 @@ let do_argv =
 		tracefile = if !tracefile <> "" then Some !tracefile else None;
 		restart = !restart;
 		disable_socket = !disable_socket;
+		use_select = !use_select;
 	}
diff --git a/tools/ocaml/xenstored/select.ml b/tools/ocaml/xenstored/select.ml
index c19df41..3e9ee68 100644
--- a/tools/ocaml/xenstored/select.ml
+++ b/tools/ocaml/xenstored/select.ml
@@ -38,7 +38,7 @@ let get_sys_fs_nr_open () =
 
 let init_event () = {read = false; write = false; except = false}
 
-let select in_fds out_fds exc_fds timeout =
+let poll_select in_fds out_fds exc_fds timeout =
 	let h = Hashtbl.create 57 in
 	let add_event event_set fd =
 		let e =
@@ -62,3 +62,16 @@ let select in_fds out_fds exc_fds timeout =
 			 (if event.write then fd :: w else w),
 			 (if event.except then fd :: x else x))
 			a r
+
+(* If the use_poll function is not called at all, we default to the original Unix.select behavior *)
+let select_fun = ref Unix.select
+
+let use_poll yes =
+	let sel_fun, max_fd =
+		if yes then poll_select, get_sys_fs_nr_open ()
+		else Unix.select, 1024 in
+	select_fun := sel_fun;
+	set_fd_limit max_fd
+
+let select in_fds out_fds exc_fds timeout =
+	(!select_fun) in_fds out_fds exc_fds timeout
diff --git a/tools/ocaml/xenstored/select.mli b/tools/ocaml/xenstored/select.mli
index 1253d4e..3912779 100644
--- a/tools/ocaml/xenstored/select.mli
+++ b/tools/ocaml/xenstored/select.mli
@@ -14,7 +14,14 @@
 
 
 (** Same interface and semantics as [Unix.select] but with an extra alternative
-    implementation based on poll. *)
+    implementation based on poll. Switching implementations is done by calling
+     the [use_poll] function. *)
 val select:
 	Unix.file_descr list -> Unix.file_descr list -> Unix.file_descr list -> float
 	-> Unix.file_descr list * Unix.file_descr list * Unix.file_descr list
+
+(** [use_poll true] will use poll based select with max fds number limitation
+   eliminated; [use_poll false] will use standard [Unix.select] with max fd
+   number set to 1024; not calling this function at all equals to use the
+   standard [Unix.select] with max fd number setting untouched. *)
+val use_poll: bool -> unit
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index bfa488f..dacea21 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -274,6 +274,8 @@ let _ =
 		);
 	);
 
+	Select.use_poll (not cf.use_select);
+
 	Sys.set_signal Sys.sighup (Sys.Signal_handle sighup_handler);
 	Sys.set_signal Sys.sigterm (Sys.Signal_handle (fun i -> quit := true));
 	Sys.set_signal Sys.sigusr1 (Sys.Signal_handle (fun i -> sigusr1_handler store));
-- 
2.1.0

[PATCH 4/8] oxenstored: catch the error when a connection is already deleted

[Zheng Li]

The function process_fdset_with is called on the read set connections first.
During the process, it might destroy a connection and remove it from the
connections database if some errors occur. However, a reference to the same
connection might still exist in the write set, which is awaiting to be
processed next. In this case, a Not_found error will be raised and the process
is aborted.

This patch changes the logic to ignore connections just missing from the
connection database and continue the rest part of the work.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/xenstored.ml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index dacea21..ea1a08f 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -43,8 +43,11 @@ let process_connection_fds store cons domains rset wset =
 			debug "closing socket connection"
 		in
 	let process_fdset_with fds fct =
-		List.iter (fun fd -> try_fct fct (Connections.find cons fd)) fds
-	in
+		List.iter
+			(fun fd ->
+			 try try_fct fct (Connections.find cons fd)
+			 with Not_found -> ()
+			) fds in
 	process_fdset_with rset Process.do_input;
 	process_fdset_with wset Process.do_output
 
-- 
2.1.0

Re: [PATCH 4/8] oxenstored: catch the error when a connection is already deleted

[Dave Scott]

On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:

> The function process_fdset_with is called on the read set connections first.
> During the process, it might destroy a connection and remove it from the
> connections database if some errors occur. However, a reference to the same
> connection might still exist in the write set, which is awaiting to be
> processed next. In this case, a Not_found error will be raised and the process
> is aborted.

Good catch! This might explain the occasional mysterious xenstored disappearance...

> 
> This patch changes the logic to ignore connections just missing from the
> connection database and continue the rest part of the work.
> 
> Signed-off-by: Zheng Li <dev@zheng.li>
> ---
> tools/ocaml/xenstored/xenstored.ml | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
> index dacea21..ea1a08f 100644
> --- a/tools/ocaml/xenstored/xenstored.ml
> +++ b/tools/ocaml/xenstored/xenstored.ml
> @@ -43,8 +43,11 @@ let process_connection_fds store cons domains rset wset =
> 			debug "closing socket connection"
> 		in
> 	let process_fdset_with fds fct =
> -		List.iter (fun fd -> try_fct fct (Connections.find cons fd)) fds
> -	in
> +		List.iter
> +			(fun fd ->
> +			 try try_fct fct (Connections.find cons fd)
> +			 with Not_found -> ()
> +			) fds in
> 	process_fdset_with rset Process.do_input;
> 	process_fdset_with wset Process.do_output
> 
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 5/8] oxenstored: use hash table to store socket connections

[Zheng Li]

Currently we use list to store socket connections. This is fine for smaller
number of connections. But when we scale up, traveling through a list of
hundreds or thousands of connections just to find a single one of them is very
low efficient.

This patch replaces the list with a (Unix.file_descr -> Connection.t) hash table.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/connections.ml | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index f4550f9..59c5c1a 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -18,17 +18,17 @@
 let debug fmt = Logging.debug "connections" fmt
 
 type t = {
-	mutable anonymous: Connection.t list;
+	anonymous: (Unix.file_descr, Connection.t) Hashtbl.t;
 	domains: (int, Connection.t) Hashtbl.t;
 	mutable watches: (string, Connection.watch list) Trie.t;
 }
 
-let create () = { anonymous = []; domains = Hashtbl.create 8; watches = Trie.create () }
+let create () = { anonymous = Hashtbl.create 37; domains = Hashtbl.create 37; watches = Trie.create () }
 
 let add_anonymous cons fd can_write =
 	let xbcon = Xenbus.Xb.open_fd fd in
 	let con = Connection.create xbcon None in
-	cons.anonymous <- con :: cons.anonymous
+	Hashtbl.add cons.anonymous (Xenbus.Xb.get_fd xbcon) con
 
 let add_domain cons dom =
 	let xbcon = Xenbus.Xb.open_mmap (Domain.get_interface dom) (fun () -> Domain.notify dom) in
@@ -36,14 +36,14 @@ let add_domain cons dom =
 	Hashtbl.add cons.domains (Domain.get_id dom) con
 
 let select cons =
-	let inset = List.map (fun c -> Connection.get_fd c) cons.anonymous
-	and outset = List.fold_left (fun l c -> if Connection.has_output c
-						then Connection.get_fd c :: l
-						else l) [] cons.anonymous in
-	inset, outset
+	Hashtbl.fold
+		(fun _ con (ins, outs) ->
+		 let fd = Connection.get_fd con in
+		 (fd :: ins,  if Connection.has_output con then fd :: outs else outs))
+		cons.anonymous ([], [])
 
-let find cons fd =
-	List.find (fun c -> Connection.get_fd c = fd) cons.anonymous
+let find cons =
+	Hashtbl.find cons.anonymous
 
 let find_domain cons id =
 	Hashtbl.find cons.domains id
@@ -55,7 +55,7 @@ let del_watches_of_con con watches =
 
 let del_anonymous cons con =
 	try
-		cons.anonymous <- Utils.list_remove con cons.anonymous;
+		Hashtbl.remove cons.anonymous (Connection.get_fd con);
 		cons.watches <- Trie.map (del_watches_of_con con) cons.watches;
 		Connection.close con
 	with exn ->
@@ -74,7 +74,7 @@ let iter_domains cons fct =
 	Hashtbl.iter (fun k c -> fct c) cons.domains
 
 let iter_anonymous cons fct =
-	List.iter (fun c -> fct c) (List.rev cons.anonymous)
+	Hashtbl.iter (fun _ c -> fct c) cons.anonymous
 
 let iter cons fct =
 	iter_domains cons fct; iter_anonymous cons fct
@@ -163,10 +163,10 @@ let stats cons =
 		nb_ops_dom := !nb_ops_dom + con_ops;
 		nb_watchs_dom := !nb_watchs_dom + con_watchs;
 	);
-	(List.length cons.anonymous, !nb_ops_anon, !nb_watchs_anon,
-	 Hashtbl.length cons.domains, !nb_ops_dom, !nb_watchs_dom)
+	(Hashtbl.length cons.anonymous, !nb_ops_anon, !nb_watchs_anon,
+	 Hashtbl.fold (fun k _ a -> if k > 0 then a+1 else a) cons.domains 0, !nb_ops_dom, !nb_watchs_dom)
 
 let debug cons =
-	let anonymous = List.map Connection.debug cons.anonymous in
+	let anonymous = Hashtbl.fold (fun _ con accu -> Connection.debug con :: accu) cons.anonymous [] in
 	let domains = Hashtbl.fold (fun _ con accu -> Connection.debug con :: accu) cons.domains [] in
 	String.concat "" (domains @ anonymous)
-- 
2.1.0

[PATCH 6/8] oxenstored: enable domain connection indexing based on eventchn port

[Zheng Li]

Currently in xenstore connection database,  we use a hash table of
(domid -> connection) to store domain connections. This allows fast indexing
based on domids.

This patch adds another dimention of fast indexing which is based on the
eventchn port number. This is useful when doing selective connections
processing based on incoming events.

For memory efficiency, the implementation reuse the the existing domain
connections hash table, but use the negative of a port number as the index (to
distinguish from domid which always >= 0).

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/connections.ml | 21 +++++++++++++++++----
 tools/ocaml/xenstored/domain.ml      |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index 59c5c1a..a0f1b89 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -19,6 +19,7 @@ let debug fmt = Logging.debug "connections" fmt
 
 type t = {
 	anonymous: (Unix.file_descr, Connection.t) Hashtbl.t;
+	(* the key of the domains table can be either a domid or a port number (taking negative) *)
 	domains: (int, Connection.t) Hashtbl.t;
 	mutable watches: (string, Connection.watch list) Trie.t;
 }
@@ -33,7 +34,10 @@ let add_anonymous cons fd can_write =
 let add_domain cons dom =
 	let xbcon = Xenbus.Xb.open_mmap (Domain.get_interface dom) (fun () -> Domain.notify dom) in
 	let con = Connection.create xbcon (Some dom) in
-	Hashtbl.add cons.domains (Domain.get_id dom) con
+	Hashtbl.add cons.domains (Domain.get_id dom) con;
+	match Domain.get_port dom with
+	| Some p -> Hashtbl.add cons.domains (-1 * Xeneventchn.to_int p) con;
+	| None -> ()
 
 let select cons =
 	Hashtbl.fold
@@ -45,8 +49,11 @@ let select cons =
 let find cons =
 	Hashtbl.find cons.anonymous
 
-let find_domain cons id =
-	Hashtbl.find cons.domains id
+let find_domain cons =
+	Hashtbl.find cons.domains
+
+let find_domain_by_port cons port =
+	Hashtbl.find cons.domains (-1 * Xeneventchn.to_int port)
 
 let del_watches_of_con con watches =
 	match List.filter (fun w -> Connection.get_con w != con) watches with
@@ -65,13 +72,19 @@ let del_domain cons id =
 	try
 		let con = find_domain cons id in
 		Hashtbl.remove cons.domains id;
+		(match Connection.get_domain con with
+		 | Some d ->
+		   (match Domain.get_port d with
+		    | Some p -> Hashtbl.remove cons.domains (-1 * Xeneventchn.to_int p)
+		    | None -> ())
+		 | None -> ());
 		cons.watches <- Trie.map (del_watches_of_con con) cons.watches;
 		Connection.close con
 	with exn ->
 		debug "del domain %u: %s" id (Printexc.to_string exn)
 
 let iter_domains cons fct =
-	Hashtbl.iter (fun k c -> fct c) cons.domains
+	Hashtbl.iter (fun k c -> if k >= 0 then fct c) cons.domains
 
 let iter_anonymous cons fct =
 	Hashtbl.iter (fun _ c -> fct c) cons.anonymous
diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
index 444069d..06d5749 100644
--- a/tools/ocaml/xenstored/domain.ml
+++ b/tools/ocaml/xenstored/domain.ml
@@ -35,6 +35,7 @@ let get_id domain = domain.id
 let get_interface d = d.interface
 let get_mfn d = d.mfn
 let get_remote_port d = d.remote_port
+let get_port d = d.port
 
 let is_bad_domain domain = domain.bad_client
 let mark_as_bad domain = domain.bad_client <- true
-- 
2.1.0

Re: [PATCH 6/8] oxenstored: enable domain connection indexing based on eventchn port

[Dave Scott]

Hi,

On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:

> Currently in xenstore connection database,  we use a hash table of
> (domid -> connection) to store domain connections. This allows fast indexing
> based on domids.
> 
> This patch adds another dimention of fast indexing which is based on the
> eventchn port number. This is useful when doing selective connections
> processing based on incoming events.
> 
> For memory efficiency, the implementation reuse the the existing domain
> connections hash table, but use the negative of a port number as the index (to
> distinguish from domid which always >= 0).

I think on a 32-bit architecture where the OCaml int is 31 bits wide, the evtchn port fits inside an OCaml int. From xenctrl.h:

  /* A port identifier is guaranteed to fit in 31 bits. */
  typedef int evtchn_port_or_error_t;

If we use the sign bit as a further tag then we’re 1 bit short. I’d rather use 2 separate hash tables to be safe.

Cheers,
Dave

> 
> Signed-off-by: Zheng Li <dev@zheng.li>
> ---
> tools/ocaml/xenstored/connections.ml | 21 +++++++++++++++++----
> tools/ocaml/xenstored/domain.ml      |  1 +
> 2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
> index 59c5c1a..a0f1b89 100644
> --- a/tools/ocaml/xenstored/connections.ml
> +++ b/tools/ocaml/xenstored/connections.ml
> @@ -19,6 +19,7 @@ let debug fmt = Logging.debug "connections" fmt
> 
> type t = {
> 	anonymous: (Unix.file_descr, Connection.t) Hashtbl.t;
> +	(* the key of the domains table can be either a domid or a port number (taking negative) *)
> 	domains: (int, Connection.t) Hashtbl.t;
> 	mutable watches: (string, Connection.watch list) Trie.t;
> }
> @@ -33,7 +34,10 @@ let add_anonymous cons fd can_write =
> let add_domain cons dom =
> 	let xbcon = Xenbus.Xb.open_mmap (Domain.get_interface dom) (fun () -> Domain.notify dom) in
> 	let con = Connection.create xbcon (Some dom) in
> -	Hashtbl.add cons.domains (Domain.get_id dom) con
> +	Hashtbl.add cons.domains (Domain.get_id dom) con;
> +	match Domain.get_port dom with
> +	| Some p -> Hashtbl.add cons.domains (-1 * Xeneventchn.to_int p) con;
> +	| None -> ()
> 
> let select cons =
> 	Hashtbl.fold
> @@ -45,8 +49,11 @@ let select cons =
> let find cons =
> 	Hashtbl.find cons.anonymous
> 
> -let find_domain cons id =
> -	Hashtbl.find cons.domains id
> +let find_domain cons =
> +	Hashtbl.find cons.domains
> +
> +let find_domain_by_port cons port =
> +	Hashtbl.find cons.domains (-1 * Xeneventchn.to_int port)
> 
> let del_watches_of_con con watches =
> 	match List.filter (fun w -> Connection.get_con w != con) watches with
> @@ -65,13 +72,19 @@ let del_domain cons id =
> 	try
> 		let con = find_domain cons id in
> 		Hashtbl.remove cons.domains id;
> +		(match Connection.get_domain con with
> +		 | Some d ->
> +		   (match Domain.get_port d with
> +		    | Some p -> Hashtbl.remove cons.domains (-1 * Xeneventchn.to_int p)
> +		    | None -> ())
> +		 | None -> ());
> 		cons.watches <- Trie.map (del_watches_of_con con) cons.watches;
> 		Connection.close con
> 	with exn ->
> 		debug "del domain %u: %s" id (Printexc.to_string exn)
> 
> let iter_domains cons fct =
> -	Hashtbl.iter (fun k c -> fct c) cons.domains
> +	Hashtbl.iter (fun k c -> if k >= 0 then fct c) cons.domains
> 
> let iter_anonymous cons fct =
> 	Hashtbl.iter (fun _ c -> fct c) cons.anonymous
> diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
> index 444069d..06d5749 100644
> --- a/tools/ocaml/xenstored/domain.ml
> +++ b/tools/ocaml/xenstored/domain.ml
> @@ -35,6 +35,7 @@ let get_id domain = domain.id
> let get_interface d = d.interface
> let get_mfn d = d.mfn
> let get_remote_port d = d.remote_port
> +let get_port d = d.port
> 
> let is_bad_domain domain = domain.bad_client
> let mark_as_bad domain = domain.bad_client <- true
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 6/8] oxenstored: enable domain connection indexing based on eventchn port

[Zheng Li]

On 16/09/2014 10:16, Dave Scott wrote:
> On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:
>> For memory efficiency, the implementation reuse the the existing domain
>> connections hash table, but use the negative of a port number as the index (to
>> distinguish from domid which always >= 0).
>
> I think on a 32-bit architecture where the OCaml int is 31 bits wide, the evtchn port fits inside an OCaml int. From xenctrl.h:
>
>    /* A port identifier is guaranteed to fit in 31 bits. */
>    typedef int evtchn_port_or_error_t;
>
> If we use the sign bit as a further tag then we’re 1 bit short. I’d rather use 2 separate hash tables to be safe.

I was a bit hesitant on using the tick too. I'll add an extra table in v2 as per your suggestion.

Cheers,
Zheng

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 7/8] oxenstored: only process domain connections that notify us by events

[Zheng Li]

Currently, upon receiving an event, oxenstored will scan all the xs rings (of
all domains), disregarding who sent that event. This is not very efficient, and
also it can shadow some misbehaviors of xenstore clients.

With this patch, oxenstore will only scan and process the domain connections
that have correctly notified it by events.

The function process_domains is refactored to be process_domain in order to
allow processing single domain connection. Also a mode_switch flag is added to
enable selectively processing just the input or output or both.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/xenstored.ml | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index ea1a08f..41a188d 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -51,13 +51,17 @@ let process_connection_fds store cons domains rset wset =
 	process_fdset_with rset Process.do_input;
 	process_fdset_with wset Process.do_output
 
-let process_domains store cons domains =
-	let do_io_domain domain =
-		if not (Domain.is_bad_domain domain) then
-			let con = Connections.find_domain cons (Domain.get_id domain) in
-				Process.do_input store cons domains con;
-				Process.do_output store cons domains con in
-	Domains.iter domains do_io_domain
+let process_domain ?mode_switch store cons domains c =
+	match Connection.get_domain c with
+	| None -> ()
+	| Some dom ->
+		 if not (Domain.is_bad_domain dom) then
+			 let read, write = match mode_switch with
+				 | None -> true, true
+				 | Some true -> true, false
+				 | Some false -> false, true in
+			 (if read then Process.do_input store cons domains c;
+			  if write then Process.do_output store cons domains c)
 
 let sigusr1_handler store =
 	try
@@ -305,6 +309,7 @@ let _ =
 			Connections.add_anonymous cons cfd can_write
 		and handle_eventchn fd =
 			let port = Event.pending eventchn in
+			debug "pending port %d" (Xeneventchn.to_int port);
 			finally (fun () ->
 				if Some port = eventchn.Event.virq_port then (
 					let (notify, deaddom) = Domains.cleanup xc domains in
@@ -312,7 +317,10 @@ let _ =
 					if deaddom <> [] || notify then
 						Connections.fire_spec_watches cons "@releaseDomain"
 				)
-			) (fun () -> Event.unmask eventchn port);
+				else
+					let c = Connections.find_domain_by_port cons port in
+					process_domain store cons domains c
+				) (fun () -> Event.unmask eventchn port)
 		and do_if_set fd set fct =
 			if List.mem fd set then
 				fct fd in
@@ -382,7 +390,7 @@ let _ =
 			process_special_fds sfds;
 		if List.length cfds > 0 || List.length wset > 0 then
 			process_connection_fds store cons domains cfds wset;
-		process_domains store cons domains
+		List.iter (process_domain store cons domains) mw
 		in
 
 	while not !quit
-- 
2.1.0

Re: [PATCH 7/8] oxenstored: only process domain connections that notify us by events

[Dave Scott]

On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:

> Currently, upon receiving an event, oxenstored will scan all the xs rings (of
> all domains), disregarding who sent that event. This is not very efficient, and
> also it can shadow some misbehaviors of xenstore clients.
> 
> With this patch, oxenstore will only scan and process the domain connections
> that have correctly notified it by events.
> 
> The function process_domains is refactored to be process_domain in order to
> allow processing single domain connection. Also a mode_switch flag is added to
> enable selectively processing just the input or output or both.
> 
> Signed-off-by: Zheng Li <dev@zheng.li>
> ---
> tools/ocaml/xenstored/xenstored.ml | 26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
> index ea1a08f..41a188d 100644
> --- a/tools/ocaml/xenstored/xenstored.ml
> +++ b/tools/ocaml/xenstored/xenstored.ml
> @@ -51,13 +51,17 @@ let process_connection_fds store cons domains rset wset =
> 	process_fdset_with rset Process.do_input;
> 	process_fdset_with wset Process.do_output
> 
> -let process_domains store cons domains =
> -	let do_io_domain domain =
> -		if not (Domain.is_bad_domain domain) then
> -			let con = Connections.find_domain cons (Domain.get_id domain) in
> -				Process.do_input store cons domains con;
> -				Process.do_output store cons domains con in
> -	Domains.iter domains do_io_domain
> +let process_domain ?mode_switch store cons domains c =

What’s mode_switch for?

> +	match Connection.get_domain c with
> +	| None -> ()
> +	| Some dom ->
> +		 if not (Domain.is_bad_domain dom) then
> +			 let read, write = match mode_switch with
> +				 | None -> true, true
> +				 | Some true -> true, false
> +				 | Some false -> false, true in
> +			 (if read then Process.do_input store cons domains c;
> +			  if write then Process.do_output store cons domains c)
> 
> let sigusr1_handler store =
> 	try
> @@ -305,6 +309,7 @@ let _ =
> 			Connections.add_anonymous cons cfd can_write
> 		and handle_eventchn fd =
> 			let port = Event.pending eventchn in
> +			debug "pending port %d" (Xeneventchn.to_int port);

Accidentally left some debugging in? It looks like it could be quite spammy.

> 			finally (fun () ->
> 				if Some port = eventchn.Event.virq_port then (
> 					let (notify, deaddom) = Domains.cleanup xc domains in
> @@ -312,7 +317,10 @@ let _ =
> 					if deaddom <> [] || notify then
> 						Connections.fire_spec_watches cons "@releaseDomain"
> 				)
> -			) (fun () -> Event.unmask eventchn port);
> +				else
> +					let c = Connections.find_domain_by_port cons port in
> +					process_domain store cons domains c
> +				) (fun () -> Event.unmask eventchn port)
> 		and do_if_set fd set fct =
> 			if List.mem fd set then
> 				fct fd in
> @@ -382,7 +390,7 @@ let _ =
> 			process_special_fds sfds;
> 		if List.length cfds > 0 || List.length wset > 0 then
> 			process_connection_fds store cons domains cfds wset;
> -		process_domains store cons domains
> +		List.iter (process_domain store cons domains) mw
> 		in
> 
> 	while not !quit
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 7/8] oxenstored: only process domain connections that notify us by events

[Zheng Li]

On 16/09/2014 10:19, Dave Scott wrote:
>
> On 15 Sep 2014, at 23:39, Zheng Li <dev@zheng.li> wrote:
>> -let process_domains store cons domains =
>> -	let do_io_domain domain =
>> -		if not (Domain.is_bad_domain domain) then
>> -			let con = Connections.find_domain cons (Domain.get_id domain) in
>> -				Process.do_input store cons domains con;
>> -				Process.do_output store cons domains con in
>> -	Domains.iter domains do_io_domain
>> +let process_domain ?mode_switch store cons domains c =
>
> What’s mode_switch for?

Hmm, I should put some comment doc there :-(  Will do in v2.

It's an optional boolean parameter. If true,  only process the input; if false, only process the output; if not preference, process both (like before).

With this added, we can selectively process only the input or output (as we can deduce the type of left work as shown in patch 8/8). Previously, for each domain connection we tried to process on both directions disregarding what kind of work was left.

>> @@ -305,6 +309,7 @@ let _ =
>> 			Connections.add_anonymous cons cfd can_write
>> 		and handle_eventchn fd =
>> 			let port = Event.pending eventchn in
>> +			debug "pending port %d" (Xeneventchn.to_int port);
>
> Accidentally left some debugging in? It looks like it could be quite spammy.

Well, this is actually intentional. It won't be spammy because debug level logging is disabled by default (as you can see there are already quite a few of them spread in the code). The information here is rather useful when doing debugging at this level.

Cheers,
Zheng

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 8/8] oxenstored: fine tunning the recognition of domain connections with queued input/output

[Zheng Li]

... also, only try to input on a connection with queued input, ditto for output.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/connection.ml  |  1 +
 tools/ocaml/xenstored/connections.ml | 14 +++++++++-----
 tools/ocaml/xenstored/xenstored.ml   |  7 ++++---
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 47695f8..1769e23 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -223,6 +223,7 @@ let pop_in con = Xenbus.Xb.get_in_packet con.xb
 let has_more_input con = Xenbus.Xb.has_more_input con.xb
 
 let has_output con = Xenbus.Xb.has_output con.xb
+let has_old_output con = Xenbus.Xb.has_old_output con.xb
 let has_new_output con = Xenbus.Xb.has_new_output con.xb
 let peek_output con = Xenbus.Xb.peek_output con.xb
 let do_output con = Xenbus.Xb.output con.xb
diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index a0f1b89..1baa6c3 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -93,11 +93,15 @@ let iter cons fct =
 	iter_domains cons fct; iter_anonymous cons fct
 
 let has_more_work cons =
-	Hashtbl.fold (fun id con acc ->
-		if Connection.has_more_input con then
-			con :: acc
-		else
-			acc) cons.domains []
+	Hashtbl.fold
+		(fun id con (racc, wacc) ->
+		 (if Connection.has_more_input con
+		  then con :: racc else racc),
+		 (if (* not currently blocked by filled up ring *)
+                    not (Connection.has_old_output con)
+		    && Connection.has_new_output con
+		  then con :: wacc else wacc))
+		cons.domains ([],[])
 
 let key_of_str path =
 	if path.[0] = '@'
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 41a188d..b95232c 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -376,9 +376,9 @@ let _ =
 			periodic_ops ();
 		);
 
-		let mw = Connections.has_more_work cons in
+		let rmw, wmw = Connections.has_more_work cons in
+		let timeout = if List.length rmw > 0 || List.length wmw > 0 then 0. else -1. in
 		let inset, outset = Connections.select cons in
-		let timeout = if List.length mw > 0 then 0. else -1. in
 		let rset, wset, _ =
 		try
 			Select.select (spec_fds @ inset) outset [] timeout
@@ -390,7 +390,8 @@ let _ =
 			process_special_fds sfds;
 		if List.length cfds > 0 || List.length wset > 0 then
 			process_connection_fds store cons domains cfds wset;
-		List.iter (process_domain store cons domains) mw
+		List.iter (process_domain ~mode_switch:true store cons domains) rmw;
+		List.iter (process_domain ~mode_switch:false store cons domains) wmw
 		in
 
 	while not !quit
-- 
2.1.0