Some oxenstored improvements (v3)

Some oxenstored improvements (v3)

[Zheng Li]

This is 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 v3. The first 6 patches are the same as in v2. For patch 7/8/9, we

  * Add a safe net mechanism for ill-behaved legacy clients
  * Make some performance improvement to reduce syslog workload
  * Small refactoring on patch 7/8 to accomodate the new changes

Cheers,
Zheng

[PATCH v3 1/9] 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.1

[PATCH v3 2/9] 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.1

[PATCH v3 3/9] 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.1

[PATCH v3 4/9] 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.1

[PATCH v3 5/9] 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.1

[PATCH v3 6/9] 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.1

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

[Zheng Li]

Currently, upon receiving an event, oxenstored will always scan/process all
the domain connections (xs rings), disregarding which domain sent that event.
This is rather costy and inefficient. It also shadows and indulges client
for not correctly communicating with us on message/space availability.

With this patch, oxenstore will only scan/process the domain connections
that have correctly notified us by events or have IO actions leftover from
previous communication.

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

diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 47695f8..807fc00 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -223,10 +223,14 @@ 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
 
+let has_more_work con =
+	has_more_input con || not (has_old_output con) && has_new_output con
+
 let incr_ops con = con.stat_nb_ops <- con.stat_nb_ops + 1
 
 let mark_symbols con =
diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index 1c8d911..f9bc225 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -98,11 +98,10 @@ 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 acc ->
+		 if Connection.has_more_work con then con :: acc else acc)
+		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 ea1a08f..b13393c 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -57,7 +57,10 @@ let process_domains store cons domains =
 			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
+	List.iter
+		(fun c ->
+		 match Connection.get_domain c with
+		 | Some d -> do_io_domain d | _ -> ())
 
 let sigusr1_handler store =
 	try
@@ -305,6 +308,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 +316,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_domains 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 +389,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
+		process_domains store cons domains mw
 		in
 
 	while not !quit
-- 
2.1.1

[PATCH v3 8/9] oxenstored: add a safe net mechanism for existing ill-behaved clients

[Zheng Li]

In previous commit, we moved from exhaustively scanning all domain connections
to only processing those have correctly notified us by events. The benefits are
not only efficiency but also correctness, because it could potentially block an
ill-behaved client and have it waiting on its own mistake. If someone makes a
mistake on this when developing a piece of code, he/she would immediately
notice the problem (as the process being blocked), so that he/she could fix it
rightaway before anything else. Note that the chances of making such mistakes
are rare in reality, because most client code would use the libxenstore library
(which has all the notification logic built in correctly) instead of having to
implement raw accessing from scratch.

On the other hand, we did notice that there were some legacy code that didn't do
the notification correctly. As some code might be still running in wild, it
would be bad if they break by this change (e.g. after an upgrade). This patch
introduces a safe net mechanism to ensure ill-behaved clients continue to work,
but still retain most of the performance benefits here.

  * We add a checker to still scan all the rings periodically, so that we can
    still pick up these messages at an acceptable frequency.

  * Internally, we introduce an io_credit concept for domain connections. It
    represents the rounds of ring scan we are going to perform on a domain
    connection. For well-behaved connections, this value is changing between 0
    and 1; but for connections detected as ill-behaved, we'll bump its credit
    to a high value so that we'll unconditionally scan its ring for the next
    $n$ rounds. This way, the client won't hiccupped by the interval between
    checker's running (especially during periods when it continously interacts
    with oxenstored); and oxenstored doesn't have to keep scanning these
    rings indefinitely (with the credit running out), as they are usually quite
    most of the time.

  * We log an message when a domain connection is suspected as ill-behaved.
    Enable [info] level logging if you want/need to see it in action. Note that
    this information won't be accurate, as false positives are possible due to
    time window (e.g. we detect a client has written to the ring and we get no
    notificiation from it for the time being, but still the notification could
    potentially arrive at some time later). It's no harm to give a domain
    connection extra credit though.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/domain.ml       | 11 ++++-
 tools/ocaml/xenstored/oxenstored.conf |  3 ++
 tools/ocaml/xenstored/xenstored.ml    | 76 ++++++++++++++++++++++++++---------
 3 files changed, 69 insertions(+), 21 deletions(-)

diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
index 06d5749..ab34314 100644
--- a/tools/ocaml/xenstored/domain.ml
+++ b/tools/ocaml/xenstored/domain.ml
@@ -28,6 +28,9 @@ type t =
 	eventchn: Event.t;
 	mutable port: Xeneventchn.t option;
 	mutable bad_client: bool;
+	mutable io_credit: int; (* the rounds of ring process left to do, default is 0,
+	                           usually set to 1 when there is work detected, could
+	                           also set to n to give "lazy" clients extra credit *)
 }
 
 let get_path dom = "/local/domain/" ^ (sprintf "%u" dom.id)
@@ -40,6 +43,11 @@ let get_port d = d.port
 let is_bad_domain domain = domain.bad_client
 let mark_as_bad domain = domain.bad_client <- true
 
+let get_io_credit domain = domain.io_credit
+let set_io_credit ?(n=1) domain = domain.io_credit <- max 0 n
+let incr_io_credit domain = domain.io_credit <- domain.io_credit + 1
+let decr_io_credit domain = domain.io_credit <- max 0 (domain.io_credit - 1)
+
 let string_of_port = function
 | None -> "None"
 | Some x -> string_of_int (Xeneventchn.to_int x)
@@ -74,7 +82,8 @@ let make id mfn remote_port interface eventchn = {
 	interface = interface;
 	eventchn = eventchn;
 	port = None;
-	bad_client = false
+	bad_client = false;
+	io_credit = 0;
 }
 
 let is_dom0 d = d.id = 0
diff --git a/tools/ocaml/xenstored/oxenstored.conf b/tools/ocaml/xenstored/oxenstored.conf
index 13ee770..dd20eda 100644
--- a/tools/ocaml/xenstored/oxenstored.conf
+++ b/tools/ocaml/xenstored/oxenstored.conf
@@ -33,3 +33,6 @@ persistent = false
 # acesss-log-nb-chars = 180
 # access-log-special-ops = false
 
+# Perodically scanning all the rings as a safenet for lazy clients.
+# Define the interval in seconds, set to negative to disable.
+# ring-scan-interval = 20
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index b13393c..bfe689b 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -54,13 +54,14 @@ let process_connection_fds store cons domains rset wset =
 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
+			let io_credit = Domain.get_io_credit domain in
+			if io_credit > 0 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
-	List.iter
-		(fun c ->
-		 match Connection.get_domain c with
-		 | Some d -> do_io_domain d | _ -> ())
+				Process.do_output store cons domains con;
+				Domain.decr_io_credit domain;
+			) in
+	Domains.iter domains do_io_domain
 
 let sigusr1_handler store =
 	try
@@ -82,6 +83,8 @@ let config_filename cf =
 
 let default_pidfile = "/var/run/xenstored.pid"
 
+let ring_scan_interval = ref 20
+
 let parse_config filename =
 	let pidfile = ref default_pidfile in
 	let options = [
@@ -108,6 +111,7 @@ let parse_config filename =
 		("access-log-transactions-ops", Config.Set_bool Logging.access_log_transaction_ops);
 		("access-log-special-ops", Config.Set_bool Logging.access_log_special_ops);
 		("allow-debug", Config.Set_bool Process.allow_debug);
+		("ring-scan-interval", Config.Set_int ring_scan_interval);
 		("pid-file", Config.Set_string pidfile); ] in
 	begin try Config.read filename options (fun _ _ -> raise Not_found)
 	with
@@ -318,7 +322,8 @@ let _ =
 				)
 				else
 					let c = Connections.find_domain_by_port cons port in
-					process_domains store cons domains [c]
+					match Connection.get_domain c with
+					| Some dom -> Domain.incr_io_credit dom | None -> ()
 				) (fun () -> Event.unmask eventchn port)
 		and do_if_set fd set fct =
 			if List.mem fd set then
@@ -327,11 +332,30 @@ let _ =
 		maybe (fun fd -> do_if_set fd rset (accept_connection true)) rw_sock;
 		maybe (fun fd -> do_if_set fd rset (accept_connection false)) ro_sock;
 		do_if_set (Event.fd eventchn) rset (handle_eventchn)
-		in
+	in
+
+	let ring_scan_checker dom =
+		(* no need to scan domains already marked as for processing *)
+		if not (Domain.get_io_credit dom > 0) then
+			let con = Connections.find_domain cons (Domain.get_id dom) in
+			if not (Connection.has_more_work con) then (
+				Process.do_output store cons domains con;
+				Process.do_input store cons domains con;
+				if Connection.has_more_work con then
+					(* Previously thought as no work, but detect some after scan (as
+					   processing a new message involves multiple steps.) It's very
+					   likely to be a "lazy" client, bump its credit. It could be false
+					   positive though (due to time window), but it's no harm to give a
+					   domain extra credit. *)
+					let n = 32 + 2 * (Domains.number domains) in
+					info "found lazy domain %d, credit %d" (Domain.get_id dom) n;
+					Domain.set_io_credit ~n dom
+			) in
 
 	let last_stat_time = ref 0. in
-	let periodic_ops_counter = ref 0 in
-	let periodic_ops () =
+	let last_scan_time = ref 0. in
+
+	let periodic_ops now =
 		(* we garbage collect the string->int dictionary after a sizeable amount of operations,
 		 * there's no need to be really fast even if we got loose
 		 * objects since names are often reuse.
@@ -344,10 +368,13 @@ let _ =
 			Symbol.garbage ()
 		end;
 
+		(* scan all the xs rings as a safenet for ill-behaved clients *)
+		if !ring_scan_interval >= 0 && now > (!last_scan_time +. float !ring_scan_interval) then
+			(last_scan_time := now; Domains.iter domains ring_scan_checker);
+
 		(* make sure we don't print general stats faster than 2 min *)
-		let ntime = Unix.gettimeofday () in
-		if ntime > (!last_stat_time +. 120.) then (
-			last_stat_time := ntime;
+		if now > (!last_stat_time +. 120.) then (
+			last_stat_time := now;
 
 			let gc = Gc.stat () in
 			let (lanon, lanon_ops, lanon_watchs,
@@ -368,16 +395,20 @@ let _ =
 		)
 		in
 
+		let period_ops_interval = 15. in
+		let period_start = ref 0. in
+
 	let main_loop () =
-		incr periodic_ops_counter;
-		if !periodic_ops_counter > 20 then (
-			periodic_ops_counter := 0;
-			periodic_ops ();
-		);
 
 		let mw = Connections.has_more_work cons in
+		List.iter
+			(fun c ->
+			 match Connection.get_domain c with
+			 | None -> () | Some d -> Domain.incr_io_credit d)
+			mw;
+		let timeout =
+			if List.length mw > 0 then 0. else period_ops_interval 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
@@ -389,7 +420,12 @@ 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 mw
+		if timeout <> 0. then (
+			let now = Unix.gettimeofday () in
+			if now > !period_start +. period_ops_interval then
+				(period_start := now; periodic_ops now)
+		);
+		process_domains store cons domains
 		in
 
 	while not !quit
-- 
2.1.1

[PATCH v3 9/9] oxenstored: reduce syslog call overhead

[Zheng Li]

We noticed that, if configured to use syslog as the logging backend, every
single line of access logging (via the syslog C binding) will call stat on
/etc/localtime for 3 times. The rational behind this is probably to detect any
timezone changes over time.

This is a considerable cost we'd like to avoid, given the intensiveness of our
access logging --- we log almost every xenstore status change (for good
reason). Also a running Xen host is rarely a mobile environment, so the little
benefit can hardly justify the cost.

Setting up the TZ environment varialbe can avoid stat calls.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/logging.ml | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 3d4a329..665b922 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -130,8 +130,17 @@ let string_of_date () =
 		tm.Unix.tm_hour tm.Unix.tm_min tm.Unix.tm_sec
 		(int_of_float (1000.0 *. msec))
 
+(* We can defer to syslog for log management *)
 let make_syslog_logger facility =
-	(* We defer to syslog for log management *)
+	(* When TZ is unset in the environment, each syslog call will stat the
+	   /etc/localtime file at least three times during the process. We'd like to
+	   avoid this cost given that we are not a mobile environment and we log
+	   almost every xenstore entry update/watch. *)
+	let () =
+		let tz_is_set =
+			try String.length (Unix.getenv "TZ") > 0
+			with Not_found -> false in
+		if not tz_is_set then Unix.putenv "TZ" "/etc/localtime" in
 	let nothing () = () in
 	let write ?level s =
 		let level = match level with
-- 
2.1.1

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Zheng Li]

We noticed that, if configured to use syslog as the logging backend, every
single line of access logging (via the syslog C binding) will call stat on
/etc/localtime for 3 times. The rational behind this is probably to detect any
timezone changes over time.

This is a considerable cost we'd like to avoid, given the intensiveness of our
access logging --- we log almost every xenstore status change (for good
reason). Also a running Xen host is rarely a mobile environment, so the little
benefit can hardly justify the cost.

Setting up the TZ environment varialbe to avoid stat calls.

Signed-off-by: Zheng Li <dev@zheng.li>
---
 tools/ocaml/xenstored/logging.ml | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index 3d4a329..76ccc55 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -130,8 +130,17 @@ let string_of_date () =
 		tm.Unix.tm_hour tm.Unix.tm_min tm.Unix.tm_sec
 		(int_of_float (1000.0 *. msec))
 
+(* We can defer to syslog for log management *)
 let make_syslog_logger facility =
-	(* We defer to syslog for log management *)
+	(* When TZ is unset in the environment, each syslog call will stat the
+	   /etc/localtime file three times during the process. We'd like to
+	   avoid this cost given that we are not a mobile environment and we log
+	   almost every xenstore entry update/watch. *)
+	let () =
+		let tz_is_set =
+			try String.length (Unix.getenv "TZ") > 0
+			with Not_found -> false in
+		if not tz_is_set then Unix.putenv "TZ" ":/etc/localtime" in
 	let nothing () = () in
 	let write ?level s =
 		let level = match level with
-- 
2.1.1

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Ian Jackson]

Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
> Setting up the TZ environment varialbe to avoid stat calls.
...
> +		if not tz_is_set then Unix.putenv "TZ" ":/etc/localtime" in

I wasn't aware of this syntax.  Can you point me to its specification ?

Thanks,
Ian.

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Zheng Li]

Hi Ian,

On 26/09/2014 16:16, Ian Jackson wrote:
> Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
>> Setting up the TZ environment varialbe to avoid stat calls.
> ...
>> +		if not tz_is_set then Unix.putenv "TZ" ":/etc/localtime" in
>
> I wasn't aware of this syntax.  Can you point me to its specification ?

Please see the 3rd format (:characters format) in the linked document below

http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html

Also a similar issue reported on stackoverflow and discussion

http://stackoverflow.com/questions/4554271/how-to-avoid-excessive-stat-etc-localtime-calls-in-strftime-on-linux

I've manually tested the change. With TZ set, strace no longer shows the intensive stat calls, and the timestamps seem to be correct wrt the local time setting.

Cheers,
Zheng

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Ian Jackson]

Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
> Please see the 3rd format (:characters format) in the linked document below
> http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html

That's great for glibc-based guests.  But what about the BSDs ?

I looked at
  http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08
and it just says that the : syntax is `implementation defined'.

Thanks,
Ian.

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Zheng Li]

On 26/09/2014 16:57, Ian Jackson wrote:
> Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
>> Please see the 3rd format (:characters format) in the linked document below
>> http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
>
> That's great for glibc-based guests.  But what about the BSDs ?

This code does xenstored access logging in Dom0. Looking at the supported Dom0 distros page on the wiki, its seems that only NetBSD is the concern here?

> I looked at
>    http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08
> and it just says that the : syntax is `implementation defined'.

I guess most BSDs adopt the same semantics. I searched for NetBSD libc manual and found this (starting from "if the value does not begin with a colon ...")

http://netbsd.gw.com/cgi-bin/man-cgi?tzset+3

Cheers,
Zheng

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Ian Jackson]

Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
> On 26/09/2014 16:57, Ian Jackson wrote:
> > Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
> >> Please see the 3rd format (:characters format) in the linked document below
> >> http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
> >
> > That's great for glibc-based guests.  But what about the BSDs ?
> 
> This code does xenstored access logging in Dom0. Looking at the supported Dom0 distros page on the wiki, its seems that only NetBSD is the concern here?

Indeed, although we might expect to port to FreeBSD too.

> > I looked at
> >    http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08
> > and it just says that the : syntax is `implementation defined'.
> 
> I guess most BSDs adopt the same semantics. I searched for NetBSD libc manual and found this (starting from "if the value does not begin with a colon ...")
> 
> http://netbsd.gw.com/cgi-bin/man-cgi?tzset+3

Ah, good.  I doubt NetBSD has diverged much here, so that's OK.

Thanks,
Ian.

Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)

[Zheng Li]

On 26/09/2014 17:29, Zheng Li wrote:
> On 26/09/2014 16:57, Ian Jackson wrote:
>> Zheng Li writes ("Re: [PATCH v3 9/9] oxenstored: reduce syslog call overhead (fix typo)"):
>>> Please see the 3rd format (:characters format) in the linked document below
>>> http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
>>
>> That's great for glibc-based guests.  But what about the BSDs ?
>
> This code does xenstored access logging in Dom0. Looking at the supported Dom0 distros page on the wiki, its seems that only NetBSD is the concern here?
>
>> I looked at
>>    http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08
>> and it just says that the : syntax is `implementation defined'.
>
> I guess most BSDs adopt the same semantics. I searched for NetBSD libc manual and found this (starting from "if the value does not begin with a colon ...")

Sorry, should be "if the value begins with a colon ...". From what I read, even if it doesn't start with a colon, it will still be treated as a time conversion file as the first resort.

Also, the syslog implementation on BSD might be different (e.g. might not have the "stat" issue at all). So as far as setting the TZ variable won't cause any problem for the xenstored process, we should be fine with it.

Cheers,
Zheng

Re: Some oxenstored improvements (v3)

[Konrad Rzeszutek Wilk]

On Thu, Sep 25, 2014 at 06:34:53PM +0100, Zheng Li wrote:
> This is 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 v3. The first 6 patches are the same as in v2. For patch 7/8/9, we
> 
>   * Add a safe net mechanism for ill-behaved legacy clients
>   * Make some performance improvement to reduce syslog workload
>   * Small refactoring on patch 7/8 to accomodate the new changes

That all looks quite nice but I have no experience with OCaml.

I fear that this patchset will have wait until an OCaml expert can
review the code :-(
> 
> Cheers,
> Zheng
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Some oxenstored improvements (v3)

[Dave Scott]

Hi,

On 29 Sep 2014, at 15:37, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Thu, Sep 25, 2014 at 06:34:53PM +0100, Zheng Li wrote:
>> This is 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 v3. The first 6 patches are the same as in v2. For patch 7/8/9, we
>> 
>>  * Add a safe net mechanism for ill-behaved legacy clients
>>  * Make some performance improvement to reduce syslog workload
>>  * Small refactoring on patch 7/8 to accomodate the new changes
> 
> That all looks quite nice but I have no experience with OCaml.
> 
> I fear that this patchset will have wait until an OCaml expert can
> review the code :-(

I’ve just read through the v3 patches and am happy with them. So I’m happy to give them an

  Acked-by: David Scott <dave.scott@citrix.com>

I think this patch:

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

is a useful bug fix — this might explain why oxenstored has occasionally ‘disappeared’ in mysterious circumstances. Zheng: could this patch be taken by itself? If so I think the release should definitely have it.

The rest of the patches will help scalability a lot but aren’t critical fixes. They would (IMHO) increase the quality of the release though.

Cheers,
Dave

>> 
>> Cheers,
>> Zheng
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Some oxenstored improvements (v3)

[Zheng Li]

On 29/09/2014 21:55, Dave Scott wrote:
> I’ve just read through the v3 patches and am happy with them. So I’m happy to give them an
>
>    Acked-by: David Scott <dave.scott@citrix.com>
>
> I think this patch:
>
> [PATCH v3 4/9] oxenstored: catch the error when a connection is already deleted
>
> is a useful bug fix — this might explain why oxenstored has occasionally ‘disappeared’ in mysterious circumstances. Zheng: could this patch be taken by itself? If so I think the release should definitely have it.

Yes, [4/9] can be picked up by itself. It's a small patch and should be quite safe.

> The rest of the patches will help scalability a lot but aren’t critical fixes. They would (IMHO) increase the quality of the release though.

To group the these patches:

   * [1/9], [2/9], and [3/9], are about select -> poll;
   * [7/9] and [8/9] are about exhaustive ring scan -> selective ring scan, with [5/9] and [6/9] adding some facilities.
   * [4/9] is a solo bug fix patch as stated;
   * [9/9] is a solo perf improvement patch on syslog load.

Cheers,
Zheng

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

Re: Some oxenstored improvements (v3)

[Konrad Rzeszutek Wilk]

On Mon, Sep 29, 2014 at 08:55:09PM +0000, Dave Scott wrote:
> Hi,
> 
> On 29 Sep 2014, at 15:37, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Thu, Sep 25, 2014 at 06:34:53PM +0100, Zheng Li wrote:
> >> This is 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 v3. The first 6 patches are the same as in v2. For patch 7/8/9, we
> >> 
> >>  * Add a safe net mechanism for ill-behaved legacy clients
> >>  * Make some performance improvement to reduce syslog workload
> >>  * Small refactoring on patch 7/8 to accomodate the new changes
> > 
> > That all looks quite nice but I have no experience with OCaml.
> > 
> > I fear that this patchset will have wait until an OCaml expert can
> > review the code :-(
> 
> I’ve just read through the v3 patches and am happy with them. So I’m happy to give them an
> 
>   Acked-by: David Scott <dave.scott@citrix.com>

Excellent!
> 
> I think this patch:
> 
> [PATCH v3 4/9] oxenstored: catch the error when a connection is already deleted
> 
> is a useful bug fix — this might explain why oxenstored has occasionally ‘disappeared’ in mysterious circumstances. Zheng: could this patch be taken by itself? If so I think the release should definitely have it.
> 
> The rest of the patches will help scalability a lot but aren’t critical fixes. They would (IMHO) increase the quality of the release though.

The bug-fix I believe should go in Xen 4.5.


In regards to the rest (help scalability) I am worried that:
 - This is rather unknown code for me (OCaml) but then so is the ARM assembler
   (I am slowly digging through the manual) - so I am falling back
   here on the maintainers perspective. Dave says OK, so that means
   we are OK.
 - Now on the other hand this is core code. If this goes belly up we a
   screwed - and if this introduces races, it will be quite a headache
   to troubleshoot down to this.

In terms of the positive aspects:
 - It will improve the code quality.
 - It improves scalability.

Dave, when you said "am happy with them" (and giving it an Acked-by) - does
that mean you dug through the code carefully with a microscope looking for
ways to break it? Or was it more of a cursory view?

Asking because if it is the first then an 'Reviewed-by' is more apt
and it also means that the chance of the code going 'belly-up' is lesser.

If you are comfortable giving an 'Reviewed-by' then I believe
it can go in Xen 4.5 (and lowers the 'belly-up' chance). If you don't
have the cycles then I believe it should go in Xen 4.6 to give it some
"soak" time.

If there are any doubts or if the author is not too keen on debugging
issues during the next couple of months (say, if you have vacation
planned or such) - if of course there are bugs - we can also postpone
these patches and put them in Xen 4.6 and have a more time to sort
issues out.

Thank you!

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

Re: Some oxenstored improvements (v3)

[Dave Scott]

Hi,

On 7 Oct 2014, at 14:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Mon, Sep 29, 2014 at 08:55:09PM +0000, Dave Scott wrote:
>> Hi,
>> 
>> On 29 Sep 2014, at 15:37, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> 
>>> On Thu, Sep 25, 2014 at 06:34:53PM +0100, Zheng Li wrote:
>>>> This is 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 v3. The first 6 patches are the same as in v2. For patch 7/8/9, we
>>>> 
>>>> * Add a safe net mechanism for ill-behaved legacy clients
>>>> * Make some performance improvement to reduce syslog workload
>>>> * Small refactoring on patch 7/8 to accomodate the new changes
>>> 
>>> That all looks quite nice but I have no experience with OCaml.
>>> 
>>> I fear that this patchset will have wait until an OCaml expert can
>>> review the code :-(
>> 
>> I’ve just read through the v3 patches and am happy with them. So I’m happy to give them an
>> 
>>  Acked-by: David Scott <dave.scott@citrix.com>
> 
> Excellent!
>> 
>> I think this patch:
>> 
>> [PATCH v3 4/9] oxenstored: catch the error when a connection is already deleted
>> 
>> is a useful bug fix — this might explain why oxenstored has occasionally ‘disappeared’ in mysterious circumstances. Zheng: could this patch be taken by itself? If so I think the release should definitely have it.
>> 
>> The rest of the patches will help scalability a lot but aren’t critical fixes. They would (IMHO) increase the quality of the release though.
> 
> The bug-fix I believe should go in Xen 4.5.
> 
> 
> In regards to the rest (help scalability) I am worried that:
> - This is rather unknown code for me (OCaml) but then so is the ARM assembler
>   (I am slowly digging through the manual) - so I am falling back
>   here on the maintainers perspective. Dave says OK, so that means
>   we are OK.
> - Now on the other hand this is core code. If this goes belly up we a
>   screwed - and if this introduces races, it will be quite a headache
>   to troubleshoot down to this.
> 
> In terms of the positive aspects:
> - It will improve the code quality.
> - It improves scalability.
> 
> Dave, when you said "am happy with them" (and giving it an Acked-by) - does
> that mean you dug through the code carefully with a microscope looking for
> ways to break it? Or was it more of a cursory view?
> 
> Asking because if it is the first then an 'Reviewed-by' is more apt
> and it also means that the chance of the code going 'belly-up' is lesser.
> 
> If you are comfortable giving an 'Reviewed-by' then I believe
> it can go in Xen 4.5 (and lowers the 'belly-up' chance). If you don't
> have the cycles then I believe it should go in Xen 4.6 to give it some
> "soak" time.

I’ve re-examined the patches and can’t see anything wrong with them. I talked
to Zheng off-list about how the patches were tested. Zheng has run a lot of
test sequences, many of them are very xenstore-intensive. Reassuringly he
found a bug — not in his patches, but in en earlier version of my ‘xenstore
reconnection’ patch (subsequently fixed).

So I’m confident to say:

Reviewed-by: David Scott <dave.scott@citrix.com>

Cheers,
Dave


> 
> If there are any doubts or if the author is not too keen on debugging
> issues during the next couple of months (say, if you have vacation
> planned or such) - if of course there are bugs - we can also postpone
> these patches and put them in Xen 4.6 and have a more time to sort
> issues out.
> 
> Thank you!
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Some oxenstored improvements (v3)

[Konrad Rzeszutek Wilk]

On Wed, Oct 08, 2014 at 12:59:32PM +0000, Dave Scott wrote:
> Hi,
> 
> On 7 Oct 2014, at 14:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Mon, Sep 29, 2014 at 08:55:09PM +0000, Dave Scott wrote:
> >> Hi,
> >> 
> >> On 29 Sep 2014, at 15:37, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >> 
> >>> On Thu, Sep 25, 2014 at 06:34:53PM +0100, Zheng Li wrote:
> >>>> This is 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 v3. The first 6 patches are the same as in v2. For patch 7/8/9, we
> >>>> 
> >>>> * Add a safe net mechanism for ill-behaved legacy clients
> >>>> * Make some performance improvement to reduce syslog workload
> >>>> * Small refactoring on patch 7/8 to accomodate the new changes
> >>> 
> >>> That all looks quite nice but I have no experience with OCaml.
> >>> 
> >>> I fear that this patchset will have wait until an OCaml expert can
> >>> review the code :-(
> >> 
> >> I’ve just read through the v3 patches and am happy with them. So I’m happy to give them an
> >> 
> >>  Acked-by: David Scott <dave.scott@citrix.com>
> > 
> > Excellent!
> >> 
> >> I think this patch:
> >> 
> >> [PATCH v3 4/9] oxenstored: catch the error when a connection is already deleted
> >> 
> >> is a useful bug fix — this might explain why oxenstored has occasionally ‘disappeared’ in mysterious circumstances. Zheng: could this patch be taken by itself? If so I think the release should definitely have it.
> >> 
> >> The rest of the patches will help scalability a lot but aren’t critical fixes. They would (IMHO) increase the quality of the release though.
> > 
> > The bug-fix I believe should go in Xen 4.5.
> > 
> > 
> > In regards to the rest (help scalability) I am worried that:
> > - This is rather unknown code for me (OCaml) but then so is the ARM assembler
> >   (I am slowly digging through the manual) - so I am falling back
> >   here on the maintainers perspective. Dave says OK, so that means
> >   we are OK.
> > - Now on the other hand this is core code. If this goes belly up we a
> >   screwed - and if this introduces races, it will be quite a headache
> >   to troubleshoot down to this.
> > 
> > In terms of the positive aspects:
> > - It will improve the code quality.
> > - It improves scalability.
> > 
> > Dave, when you said "am happy with them" (and giving it an Acked-by) - does
> > that mean you dug through the code carefully with a microscope looking for
> > ways to break it? Or was it more of a cursory view?
> > 
> > Asking because if it is the first then an 'Reviewed-by' is more apt
> > and it also means that the chance of the code going 'belly-up' is lesser.
> > 
> > If you are comfortable giving an 'Reviewed-by' then I believe
> > it can go in Xen 4.5 (and lowers the 'belly-up' chance). If you don't
> > have the cycles then I believe it should go in Xen 4.6 to give it some
> > "soak" time.
> 
> I’ve re-examined the patches and can’t see anything wrong with them. I talked
> to Zheng off-list about how the patches were tested. Zheng has run a lot of
> test sequences, many of them are very xenstore-intensive. Reassuringly he
> found a bug — not in his patches, but in en earlier version of my ‘xenstore
> reconnection’ patch (subsequently fixed).
> 
> So I’m confident to say:
> 
> Reviewed-by: David Scott <dave.scott@citrix.com>

Excellent, hence:

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> 
> Cheers,
> Dave
> 
> 
> > 
> > If there are any doubts or if the author is not too keen on debugging
> > issues during the next couple of months (say, if you have vacation
> > planned or such) - if of course there are bugs - we can also postpone
> > these patches and put them in Xen 4.6 and have a more time to sort
> > issues out.
> > 
> > Thank you!
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

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

Re: Some oxenstored improvements (v3)

[Ian Campbell]

On Wed, 2014-10-08 at 09:01 -0400, Konrad Rzeszutek Wilk wrote:
> > 
> > So I’m confident to say:
> > 
> > Reviewed-by: David Scott <dave.scott@citrix.com>
> 
> Excellent, hence:
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I've applied all of them.

Including the final patch "oxenstored: reduce syslog call overhead (fix
typo)", there was some back and forth on that one but Ian J confirmed
over IRC that he was satisfied.

> 
> Thank you!
> > 
> > Cheers,
> > Dave
> > 
> > 
> > > 
> > > If there are any doubts or if the author is not too keen on debugging
> > > issues during the next couple of months (say, if you have vacation
> > > planned or such) - if of course there are bugs - we can also postpone
> > > these patches and put them in Xen 4.6 and have a more time to sort
> > > issues out.
> > > 
> > > Thank you!
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel



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