Quantcast

[xen stable-4.4] oxenstored: add a safe net mechanism for existing ill-behaved clients

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[xen stable-4.4] oxenstored: add a safe net mechanism for existing ill-behaved clients

patchbot
commit 7e68f13c3bd607d62297a8cb8c3dc04685b7587e
Author:     Zheng Li <[hidden email]>
AuthorDate: Fri Mar 24 17:04:23 2017 +0000
Commit:     Ian Jackson <[hidden email]>
CommitDate: Wed Apr 5 15:26:37 2017 +0100

    oxenstored: add a safe net mechanism for existing ill-behaved clients
   
    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.
   
    Reported-by: Juergen Gross <[hidden email]>
    Signed-off-by: Zheng Li <[hidden email]>
    Reviewed-by: David Scott <[hidden email]>
---
 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 4a1d027..58a1ffc 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
@@ -316,7 +320,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
@@ -325,11 +330,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.
@@ -342,10 +366,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,
@@ -366,16 +393,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
  Unix.select (spec_fds @ inset) outset [] timeout
@@ -387,7 +418,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
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.4

_______________________________________________
Xen-changelog mailing list
[hidden email]
https://lists.xenproject.org/xen-changelog
Loading...