Some oxenstored improvements (v2)

Some oxenstored improvements (v2)

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

This is the 2nd version with all previous review comments addressed.

[PATCH v2 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      | 54 ++++++++++++++++++++++++++++
 tools/ocaml/xenstored/select.mli     | 20 +++++++++++
 tools/ocaml/xenstored/select_stubs.c | 68 ++++++++++++++++++++++++++++++++++++
 tools/ocaml/xenstored/xenstored.ml   |  2 +-
 5 files changed, 149 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..3f4b671
--- /dev/null
+++ b/tools/ocaml/xenstored/select.ml
@@ -0,0 +1,54 @@
+(*
+ * 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.
+ *)
+
+
+(* The [read], [write], [except] are fields mapped to the POLLIN/OUT/PRI
+   subscription flags used by poll, which have a correspondence to the
+   readfds, writefds, exceptfds concept as in select. *)
+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;
+	(* Unix.stdin and init_event are dummy input as stubs, which will
+           always be overwritten later on.  *)
+	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..33beeb9
--- /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("poll", 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

[PATCH v2 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.

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

diff --git a/tools/ocaml/xenstored/select.ml b/tools/ocaml/xenstored/select.ml
index 3f4b671..ab8c847 100644
--- a/tools/ocaml/xenstored/select.ml
+++ b/tools/ocaml/xenstored/select.ml
@@ -23,6 +23,16 @@ 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 go above the system level nr_open,
+   which we can read from /proc/sys. *)
+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 33beeb9..4a8edb5 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

[PATCH v2 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 ab8c847..0455e16 100644
--- a/tools/ocaml/xenstored/select.ml
+++ b/tools/ocaml/xenstored/select.ml
@@ -36,7 +36,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 v2 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

[PATCH v2 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 | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index f4550f9..3e6a48b 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.anonymous, !nb_ops_anon, !nb_watchs_anon,
 	 Hashtbl.length cons.domains, !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 v2 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 dom ids.

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

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

diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index 3e6a48b..1c8d911 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -20,10 +20,16 @@ let debug fmt = Logging.debug "connections" fmt
 type t = {
 	anonymous: (Unix.file_descr, Connection.t) Hashtbl.t;
 	domains: (int, Connection.t) Hashtbl.t;
+	ports: (Xeneventchn.t, Connection.t) Hashtbl.t;
 	mutable watches: (string, Connection.watch list) Trie.t;
 }
 
-let create () = { anonymous = Hashtbl.create 37; domains = Hashtbl.create 37; watches = Trie.create () }
+let create () = {
+	anonymous = Hashtbl.create 37;
+	domains = Hashtbl.create 37;
+	ports = Hashtbl.create 37;
+	watches = Trie.create ()
+}
 
 let add_anonymous cons fd can_write =
 	let xbcon = Xenbus.Xb.open_fd fd in
@@ -33,7 +39,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.ports p con;
+	| None -> ()
 
 let select cons =
 	Hashtbl.fold
@@ -45,8 +54,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.ports port
 
 let del_watches_of_con con watches =
 	match List.filter (fun w -> Connection.get_con w != con) watches with
@@ -65,6 +77,12 @@ 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.ports p
+		    | None -> ())
+		 | None -> ());
 		cons.watches <- Trie.map (del_watches_of_con con) cons.watches;
 		Connection.close con
 	with exn ->
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

[PATCH v2 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 | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index ea1a08f..c7dc6fa 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -51,13 +51,20 @@ 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
+(* [mode_switch] is an optional boolean flag, which, if not present then both
+   input and output will be processed, otherwise only the input (when equals
+   true) or the output (when equals false) will be processed. *)
+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 +312,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 +320,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 +393,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

[PATCH v2 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 1c8d911..1cf916a 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -98,11 +98,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 c7dc6fa..70dff6f 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -379,9 +379,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
@@ -393,7 +393,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

Re: Some oxenstored improvements (v2)

[Ian Jackson]

Zheng Li writes ("Some oxenstored improvements (v2)"):
> 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.
> 
> This is the 2nd version with all previous review comments addressed.

Thanks.  For the avoidance of doubt, this needs an ack from Dave
Scott.

Ian.

Re: Some oxenstored improvements (v2)

[Zheng Li]

Thanks. We now have 2-3 extra patches on this in the pipeline. They are currently in testing, but I'm going to send out for review very soon (likely in this week). It might worth to wait a little more to ACK and pull them in altogether.

Cheers,
Zheng

On 23/09/2014 15:32, Ian Jackson wrote:
> Zheng Li writes ("Some oxenstored improvements (v2)"):
>> 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.
>>
>> This is the 2nd version with all previous review comments addressed.
>
> Thanks.  For the avoidance of doubt, this needs an ack from Dave
> Scott.
>
> Ian.
>

Re: Some oxenstored improvements (v2)

[Dave Scott]

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

> Thanks. We now have 2-3 extra patches on this in the pipeline. They are currently in testing, but I'm going to send out for review very soon (likely in this week). It might worth to wait a little more to ACK and pull them in altogether.

I was just about to Ack the previous set :-) What do the extra patches do?

Cheers,
Dave

> 
> Cheers,
> Zheng
> 
> On 23/09/2014 15:32, Ian Jackson wrote:
>> Zheng Li writes ("Some oxenstored improvements (v2)"):
>>> 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.
>>> 
>>> This is the 2nd version with all previous review comments addressed.
>> 
>> Thanks.  For the avoidance of doubt, this needs an ack from Dave
>> Scott.
>> 
>> Ian.
>> 

Re: Some oxenstored improvements (v2)

[Ian Jackson]

Dave Scott writes ("Re: Some oxenstored improvements (v2)"):
> On 23 Sep 2014, at 16:20, Zheng Li <dev@zheng.li> wrote:
> > Thanks. We now have 2-3 extra patches on this in the pipeline. They are currently in testing, but I'm going to send out for review very soon (likely in this week). It might worth to wait a little more to ACK and pull them in altogether.

Is that just extra patches, or are you going to send out modified
versions of the already-posted ones, too ?

> I was just about to Ack the previous set :-) What do the extra patches do?

If the previous set are fine I think we should probably apply them
right away, unless someone thinks there is a reason for waiting.

Thanks,
Ian.

Re: Some oxenstored improvements (v2)

[Zheng Li]

> Dave Scott writes ("Re: Some oxenstored improvements (v2)"):
>> I was just about to Ack the previous set :-) What do the extra patches do?

One is to add a periodical checker as a safe net for the behavioural optimization change in previous set (from exhaustively scanning all the rings to only checking targeted ones). The consideration here is that there might be some ill-behaved clients currently existing in the wild, so it's better to make gradual move.

The other one is a performance improvement related to syslog. Currently, if configured to use syslog, every single line of access log could potentially call sys.stat 3 times, which is a considerable cost given the intensiveness of our access logging.

On 23/09/2014 16:41, Ian Jackson wrote:

> If the previous set are fine I think we should probably apply them
> right away, unless someone thinks there is a reason for waiting.

As the first patch is close related to the some feature in the previous set (as a safety improvement), it's better to just wait a little bit.

Thanks,
Zheng