Message ID | 20180712072934.13001-5-sriharsha.basavapatna@broadcom.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Support dynamic rebalancing of offloaded flows | expand |
On Thu, Jul 12, 2018 at 12:59:34PM +0530, Sriharsha Basavapatna via dev wrote: > This is the fourth patch in the patch-set to support dynamic rebalancing > of offloaded flows. > > A new OVS configuration parameter "offload-rebalance", is added to ovsdb. > The default value of this is "disable". To enable this feature, set the > value of this parameter to "pps-rate", which provides packets-per-second > rate based policy to dynamically offload and un-offload flows. > > Note: This option can be enabled only when 'hw-offload' policy is enabled. > It also requires 'tc-policy' to be set to 'skip_sw'; otherwise, flow > offload errors (specifically ENOSPC error this feature depends on) reported > by an offloaded device are supressed by TC-Flower kernel module. > > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com> > Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com> > Reviewed-by: Sathya Perla <sathya.perla@broadcom.com> Thanks. I would add an update to NEWS to mention the new rebalancing feature. I would fold this patch in with the previous one. Please declare variables at point of first use, when possible, and avoid adding unecessary parentheses to conditions. I think the log message "netdev: Using policy 'disable'" is going to confuse people. If it is necessary to log the policy, the log message should explain better than that. I don't think the enum is going to help anything. I would use a bool. I would drop OFFLOAD_REBALANCE_POLICY_DEFAULT. Please look at the ovs-vswitchd.conf.db(5) manpage. The formatting is not going to look good. I recommend using just a bool, honestly, instead of this enum. Thanks, Ben.
On Sat, Sep 1, 2018 at 1:05 AM, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Jul 12, 2018 at 12:59:34PM +0530, Sriharsha Basavapatna via dev wrote: >> This is the fourth patch in the patch-set to support dynamic rebalancing >> of offloaded flows. >> >> A new OVS configuration parameter "offload-rebalance", is added to ovsdb. >> The default value of this is "disable". To enable this feature, set the >> value of this parameter to "pps-rate", which provides packets-per-second >> rate based policy to dynamically offload and un-offload flows. >> >> Note: This option can be enabled only when 'hw-offload' policy is enabled. >> It also requires 'tc-policy' to be set to 'skip_sw'; otherwise, flow >> offload errors (specifically ENOSPC error this feature depends on) reported >> by an offloaded device are supressed by TC-Flower kernel module. >> >> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> >> Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com> >> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com> >> Reviewed-by: Sathya Perla <sathya.perla@broadcom.com> > > Thanks. Thanks for your comments. > > I would add an update to NEWS to mention the new rebalancing feature. > > I would fold this patch in with the previous one. done. > > Please declare variables at point of first use, when possible, and avoid > adding unecessary parentheses to conditions. done. > > I think the log message "netdev: Using policy 'disable'" is going to > confuse people. If it is necessary to log the policy, the log message > should explain better than that. removed feature log messages. > > I don't think the enum is going to help anything. I would use a bool. done. > > I would drop OFFLOAD_REBALANCE_POLICY_DEFAULT. done. > > Please look at the ovs-vswitchd.conf.db(5) manpage. The formatting is > not going to look good. I recommend using just a bool, honestly, > instead of this enum. done. > > Thanks, > > Ben. Thanks, -Harsha
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 03db4ad32..71205ae65 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2176,7 +2176,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) VLOG_DBG("added flow"); } else if (err != EEXIST) { struct netdev *oor_netdev = NULL; - if (err == ENOSPC) { + if (netdev_is_offload_rebalance_policy_enabled() && (err == ENOSPC)) { oor_netdev = flow_get_tunnel_netdev(&match.flow.tunnel); if (!oor_netdev) { oor_netdev = dev; diff --git a/lib/netdev.c b/lib/netdev.c index b17f0563f..9992b846c 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2474,6 +2474,43 @@ netdev_free_custom_stats_counters(struct netdev_custom_stats *custom_stats) #ifdef __linux__ +enum offload_rebalance_policy { + OFFLOAD_REBALANCE_POLICY_DISABLE, + OFFLOAD_REBALANCE_POLICY_PPS_RATE +}; + +static enum +offload_rebalance_policy netdev_offload_rebalance_policy = + OFFLOAD_REBALANCE_POLICY_DISABLE; + +static void +netdev_offload_rebalance_set_policy(const char *policy) +{ + if (!policy) { + return; + } + + if (!strcmp(policy, "pps-rate")) { + netdev_offload_rebalance_policy = OFFLOAD_REBALANCE_POLICY_PPS_RATE; + } else if (!strcmp(policy, "disable")) { + netdev_offload_rebalance_policy = OFFLOAD_REBALANCE_POLICY_DISABLE; + } else { + VLOG_WARN("netdev: Invalid policy '%s'", policy); + return; + } + + VLOG_INFO("netdev: Using policy '%s'", policy); +} + +bool +netdev_is_offload_rebalance_policy_enabled(void) +{ + if (netdev_offload_rebalance_policy == OFFLOAD_REBALANCE_POLICY_PPS_RATE) { + return true; + } + return false; +} + static void netdev_ports_flow_init(void) { @@ -2494,12 +2531,18 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) if (ovsthread_once_start(&once)) { netdev_flow_api_enabled = true; + const char *offl_rebal_policy = NULL; VLOG_INFO("netdev: Flow API Enabled"); tc_set_policy(smap_get_def(ovs_other_config, "tc-policy", TC_POLICY_DEFAULT)); + offl_rebal_policy = smap_get_def(ovs_other_config, + "offload-rebalance", + OFFLOAD_REBALANCE_POLICY_DEFAULT); + netdev_offload_rebalance_set_policy(offl_rebal_policy); + netdev_ports_flow_init(); ovsthread_once_done(&once); diff --git a/lib/netdev.h b/lib/netdev.h index c941f1e1e..0ac2774c9 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -223,6 +223,8 @@ int netdev_init_flow_api(struct netdev *); uint32_t netdev_get_block_id(struct netdev *); bool netdev_is_flow_api_enabled(void); void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); +bool netdev_is_offload_rebalance_policy_enabled(void); +#define OFFLOAD_REBALANCE_POLICY_DEFAULT "disable" struct dpif_port; int netdev_ports_insert(struct netdev *, const struct dpif_class *, diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index cf1524615..0910b66b1 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -967,7 +967,9 @@ udpif_revalidator(void *arg) dpif_flow_dump_destroy(udpif->dump); seq_change(udpif->dump_seq); - udpif_run_flow_rebalance(udpif); + if (netdev_is_offload_rebalance_policy_enabled()) { + udpif_run_flow_rebalance(udpif); + } duration = MAX(time_msec() - start_time, 1); udpif->dump_duration = duration; @@ -2741,7 +2743,8 @@ revalidate(struct revalidator *revalidator) } ukey->dump_seq = dump_seq; - if (result != UKEY_DELETE) { + if (netdev_is_offload_rebalance_policy_enabled() && + result != UKEY_DELETE) { udpif_update_flow_pps(udpif, ukey, f); } diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 76094852d..a0aad0204 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -475,6 +475,28 @@ </p> </column> + <column name="other_config" key="offload-rebalance" + type='{"type": "string"}'> + <p> + Configures HW offload rebalancing, that allows to dynamically + offload and un-offload flows while an offload-device is out of + resources (OOR). The value specifies the criteria used to + select flows for offloading. The default value of 'disable', + disables this policy. To enable packets-per-second rate based + offload policy, set the value to 'pps-rate'. + Options: + <code>disable</code> - No offload rebalancing + <code>pps-rate</code> - Packets-per-second rate based policy + </p> + <p> + This is only relevant if HW offloading is enabled (hw-offload). + When this policy is enabled, it also requires 'tc-policy' to + be set to 'skip_sw'. + </p> + <p> + The default value is <code>disable</code>. + </p> + </column> </group> <group title="Status">