From 5fcc0d004ae0aa080deed51c842d074a83ec1f67 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 28 Nov 2011 10:35:15 -0800 Subject: [PATCH] ofproto: Add "fast path". The key to getting good performance on the netperf CRR test seems to be to handle the first packet of each new flow as quickly as possible. Until now, we've only had one opportunity to do that on each trip through the main poll loop. One way to improve would be to make that poll loop circulate more quickly. My experiments show, however, that even just commenting out the slower parts of the poll loop yield minimal improvement. This commit takes another approach. Instead of making the poll loop overall faster, it invokes the performance-critical parts of it more than once during each poll loop. My measurements show that this commit improves netperf CRR performance by 24% versus the previous commit, for an overall improvement of 87% versus the baseline just before the commit that removed the poll_fd_woke(). With this commit, ovs-benchmark performance has also improved by 13% overall since that baseline. --- lib/dpif.c | 2 ++ ofproto/ofproto-dpif.c | 40 +++++++++++++++++++++++--------------- ofproto/ofproto-provider.h | 13 +++++++++---- ofproto/ofproto.c | 31 ++++++++++++++++++++--------- ofproto/ofproto.h | 1 + vswitchd/bridge.c | 28 ++++++++++++++++---------- vswitchd/bridge.h | 3 ++- vswitchd/ovs-vswitchd.c | 2 ++ 8 files changed, 80 insertions(+), 40 deletions(-) diff --git a/lib/dpif.c b/lib/dpif.c index 5f5417b7..9cbf8771 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1110,6 +1110,8 @@ dpif_recv(struct dpif *dpif, struct dpif_upcall *upcall) ds_destroy(&flow); free(packet); + } else if (error && error != EAGAIN) { + log_operation(dpif, "recv", error); } return error; } diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 448f3d23..1b7bc448 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -722,18 +722,11 @@ destruct(struct ofproto *ofproto_) } static int -run(struct ofproto *ofproto_) +run_fast(struct ofproto *ofproto_) { struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); - struct ofport_dpif *ofport; - struct ofbundle *bundle; unsigned int work; - if (!clogged) { - complete_operations(ofproto); - } - dpif_run(ofproto->dpif); - /* Handle one or more batches of upcalls, until there's nothing left to do * or until we do a fixed total amount of work. * @@ -746,13 +739,30 @@ run(struct ofproto *ofproto_) work = 0; while (work < FLOW_MISS_MAX_BATCH) { int retval = handle_upcalls(ofproto, FLOW_MISS_MAX_BATCH - work); - if (retval < 0) { + if (retval <= 0) { return -retval; - } else if (!retval) { - break; - } else { - work += retval; } + work += retval; + } + return 0; +} + +static int +run(struct ofproto *ofproto_) +{ + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); + struct ofport_dpif *ofport; + struct ofbundle *bundle; + int error; + + if (!clogged) { + complete_operations(ofproto); + } + dpif_run(ofproto->dpif); + + error = run_fast(ofproto_); + if (error) { + return error; } if (timer_expired(&ofproto->next_expiration)) { @@ -2673,9 +2683,6 @@ handle_upcalls(struct ofproto_dpif *ofproto, unsigned int max_batch) error = dpif_recv(ofproto->dpif, upcall); if (error) { - if (error == ENODEV && n_misses == 0) { - return -ENODEV; - } break; } @@ -6032,6 +6039,7 @@ const struct ofproto_class ofproto_dpif_class = { destruct, dealloc, run, + run_fast, wait, flush, get_features, diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index acc28402..6576069f 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -332,12 +332,17 @@ struct ofproto_class { * - Call ofproto_rule_expire() for each OpenFlow flow that has reached * its hard_timeout or idle_timeout, to expire the flow. * - * Returns 0 if successful, otherwise a positive errno value. The ENODEV - * return value specifically means that the datapath underlying 'ofproto' - * has been destroyed (externally, e.g. by an admin running ovs-dpctl). - */ + * Returns 0 if successful, otherwise a positive errno value. */ int (*run)(struct ofproto *ofproto); + /* Performs periodic activity required by 'ofproto' that needs to be done + * with the least possible latency. + * + * This is run multiple times per main loop. An ofproto provider may + * implement it or not, according to whether it provides a performance + * boost for that ofproto implementation. */ + int (*run_fast)(struct ofproto *ofproto); + /* Causes the poll loop to wake up when 'ofproto''s 'run' function needs to * be called, e.g. by calling the timer or fd waiting functions in * poll-loop.h. */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index bb040712..b81bd6b8 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -908,14 +908,8 @@ ofproto_run(struct ofproto *p) int error; error = p->ofproto_class->run(p); - if (error == ENODEV) { - /* Someone destroyed the datapath behind our back. The caller - * better destroy us and give up, because we're just going to - * spin from here on out. */ - static struct vlog_rate_limit rl2 = VLOG_RATE_LIMIT_INIT(1, 5); - VLOG_ERR_RL(&rl2, "%s: datapath was destroyed externally", - p->name); - return ENODEV; + if (error && error != EAGAIN) { + VLOG_ERR_RL(&rl, "%s: run failed (%s)", p->name, strerror(error)); } if (p->ofproto_class->port_poll) { @@ -951,7 +945,26 @@ ofproto_run(struct ofproto *p) NOT_REACHED(); } - return 0; + return error; +} + +/* Performs periodic activity required by 'ofproto' that needs to be done + * with the least possible latency. + * + * It makes sense to call this function a couple of times per poll loop, to + * provide a significant performance boost on some benchmarks with the + * ofproto-dpif implementation. */ +int +ofproto_run_fast(struct ofproto *p) +{ + int error; + + error = p->ofproto_class->run_fast ? p->ofproto_class->run_fast(p) : 0; + if (error && error != EAGAIN) { + VLOG_ERR_RL(&rl, "%s: fastpath run failed (%s)", + p->name, strerror(error)); + } + return error; } void diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index fb001061..ccd82025 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -144,6 +144,7 @@ void ofproto_destroy(struct ofproto *); int ofproto_delete(const char *name, const char *type); int ofproto_run(struct ofproto *); +int ofproto_run_fast(struct ofproto *); void ofproto_wait(struct ofproto *); bool ofproto_is_alive(const struct ofproto *); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index aadb0c7a..d2dcd027 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -1782,13 +1782,28 @@ refresh_cfm_stats(void) } } +/* Performs periodic activity required by bridges that needs to be done with + * the least possible latency. + * + * It makes sense to call this function a couple of times per poll loop, to + * provide a significant performance boost on some benchmarks with ofprotos + * that use the ofproto-dpif implementation. */ +void +bridge_run_fast(void) +{ + struct bridge *br; + + HMAP_FOR_EACH (br, node, &all_bridges) { + ofproto_run_fast(br->ofproto); + } +} + void bridge_run(void) { const struct ovsrec_open_vswitch *cfg; bool vlan_splinters_changed; - bool datapath_destroyed; bool database_changed; struct bridge *br; @@ -1811,15 +1826,8 @@ bridge_run(void) cfg = ovsrec_open_vswitch_first(idl); /* Let each bridge do the work that it needs to do. */ - datapath_destroyed = false; HMAP_FOR_EACH (br, node, &all_bridges) { - int error = ofproto_run(br->ofproto); - if (error) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - VLOG_ERR_RL(&rl, "bridge %s: datapath was destroyed externally, " - "forcing reconfiguration", br->name); - datapath_destroyed = true; - } + ofproto_run(br->ofproto); } /* Re-configure SSL. We do this on every trip through the main loop, @@ -1847,7 +1855,7 @@ bridge_run(void) } } - if (database_changed || datapath_destroyed || vlan_splinters_changed) { + if (database_changed || vlan_splinters_changed) { if (cfg) { struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl); diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h index 32b581e2..2d388338 100644 --- a/vswitchd/bridge.h +++ b/vswitchd/bridge.h @@ -1,4 +1,4 @@ -/* Copyright (c) 2008, 2009, 2010 Nicira Networks +/* Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ void bridge_init(const char *remote); void bridge_exit(void); void bridge_run(void); +void bridge_run_fast(void); void bridge_wait(void); #endif /* bridge.h */ diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c index bdc35330..301d0739 100644 --- a/vswitchd/ovs-vswitchd.c +++ b/vswitchd/ovs-vswitchd.c @@ -92,7 +92,9 @@ main(int argc, char *argv[]) if (signal_poll(sighup)) { vlog_reopen_log_file(); } + bridge_run_fast(); bridge_run(); + bridge_run_fast(); unixctl_server_run(unixctl); netdev_run(); -- 2.30.2