| Message ID | a004e6f76a3e4e6fbf3a6f5577644d60f03d2281.1762950453.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Architectural refactoring of hardware offload infrastructure | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
git sha: a004e6f76a3e4e6fbf3a6f5577644d60f03d2281 Author: Eelco Chaudron <echaudro@redhat.com> Subject: dpif-offload: Allow per-port offload provider priority config. This patch adds support for configuring the priority of offload providers at a port level. When multiple providers exist and support the same port, the 'hw-offload-priority' option allows specifying the order in which they are tried. > diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c > index bcb964af8..3f7bbac70 100644 > --- a/lib/dpif-offload.c > +++ b/lib/dpif-offload.c > @@ -23,6 +23,8 @@ > #include "netdev-provider.h" > #include "unixctl.h" > #include "util.h" > +#include "vswitch-idl.h" > + > #include "openvswitch/dynamic-string.h" > #include "openvswitch/shash.h" > #include "openvswitch/vlog.h" > @@ -53,6 +55,7 @@ static const struct dpif_offload_class *base_dpif_offload_classes[] = { > static char *dpif_offload_provider_priority_list = "tc,dpdk,dummy,dummy_x"; > static atomic_bool dpif_offload_global_enabled = false; > static atomic_bool dpif_offload_rebalance_policy = false; > +static struct smap port_order_cfg = SMAP_INITIALIZER(&port_order_cfg); Does this global smap need synchronization mechanisms? The code accesses port_order_cfg from dpif_offload_port_add() while it can be modified in dpif_offload_set_global_cfg(). > @@ -418,34 +421,79 @@ dpif_offload_set_netdev_offload(struct netdev *netdev, > ovsrcu_set(&netdev->dpif_offload, offload); > } > > +static bool > +dpif_offload_try_port_add(struct dpif_offload *offload, struct netdev *netdev, > + odp_port_t port_no) > +{ > + if (offload->class->can_offload(offload, netdev)) { > + int err = offload->class->port_add(offload, netdev, port_no); > + if (!err) { > + VLOG_DBG("netdev %s added to dpif-offload provider %s", > + netdev_get_name(netdev), dpif_offload_name(offload)); > + return true; > + } else { > + VLOG_ERR("Failed adding netdev %s to dpif-offload provider " > + "%s, error %s", > + netdev_get_name(netdev), dpif_offload_name(offload), > + ovs_strerror(err)); > + } > + } else { > + VLOG_DBG("netdev %s failed can_offload for dpif-offload provider %s", > + netdev_get_name(netdev), dpif_offload_name(offload)); > + } > + return false; > +} > + > void > dpif_offload_port_add(struct dpif *dpif, struct netdev *netdev, > odp_port_t port_no) > { > struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); > + const char *port_priority = smap_get(&port_order_cfg, > + netdev_get_name(netdev)); > struct dpif_offload *offload; > > if (!dp_offload) { > return; > } > > - LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) { > - if (offload->class->can_offload(offload, netdev)) { > - int err = offload->class->port_add(offload, netdev, port_no); > - if (!err) { > - VLOG_DBG("netdev %s added to dpif-offload provider %s", > - netdev_get_name(netdev), dpif_offload_name(offload)); > + if (port_priority) { > + char *tokens = xstrdup(port_priority); > + char *saveptr; > + > + VLOG_DBG("for netdev %s using port priority %s", > + netdev_get_name(netdev), port_priority); > + > + for (char *name = strtok_r(tokens, ",", &saveptr); > + name; > + name = strtok_r(NULL, ",", &saveptr)) { > + bool provider_added = false; > + > + if (!strcmp("none", name)) { > break; > - } else { > - VLOG_ERR("Failed adding netdev %s to dpif-offload provider " > - "%s, error %s", > - netdev_get_name(netdev), dpif_offload_name(offload), > - ovs_strerror(err)); > } > - } else { > - VLOG_DBG( > - "netdev %s failed can_offload for dpif-offload provider %s", > - netdev_get_name(netdev), dpif_offload_name(offload)); > + > + LIST_FOR_EACH (offload, dpif_list_node, > + &dp_offload->offload_providers) { > + if (!strcmp(name, offload->class->type)) { > + > + provider_added = dpif_offload_try_port_add(offload, netdev, > + port_no); > + break; > + } > + } > + > + if (provider_added) { > + break; > + } > } > + free(tokens); [ ... ] > @@ -559,13 +607,16 @@ dpif_offload_dump_done(struct dpif_offload_dump *dump) > } > > void > -dpif_offload_set_global_cfg(const struct smap *other_cfg) > +dpif_offload_set_global_cfg(const struct ovsrec_open_vswitch *cfg) > { > static struct ovsthread_once init_once = OVSTHREAD_ONCE_INITIALIZER; > - const char *priority = smap_get(other_cfg, "hw-offload-priority"); > + const struct smap *other_cfg = &cfg->other_config; > + const char *priority; > > /* The 'hw-offload-priority' parameter can only be set at startup, > * any successive change needs a restart. */ > + priority = smap_get(other_cfg, "hw-offload-priority"); > + > if (ovsthread_once_start(&init_once)) { > /* Initialize the dpif-offload layer in case it's not yet initialized > * at the first invocation of setting the configuration. */ [ ... ] > + /* Filter out the 'hw-offload-priority' per port setting we need it before > + * ports are added, so we can assign the correct offload-provider. > + * Note that we can safely rebuild the map here, as we only access this > + * from the same (main) thread. */ > + smap_clear(&port_order_cfg); > + for (int i = 0; i < cfg->n_bridges; i++) { > + const struct ovsrec_bridge *br_cfg = cfg->bridges[i]; > + > + for (int j = 0; j < br_cfg->n_ports; j++) { > + const struct ovsrec_port *port_cfg = br_cfg->ports[j]; > + > + priority = smap_get(&port_cfg->other_config, > + "hw-offload-priority"); > + if (priority) { > + smap_add(&port_order_cfg, port_cfg->name, priority); > + } > + } > + } Does this code handle the case when port names change in the configuration? Would smap_clear() followed by smap_add() properly handle ports that were removed from the config but whose entries might still be referenced in dpif_offload_port_add()? > diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h > index 87adcd0fc..2720d2b5f 100644 > --- a/lib/dpif-offload.h > +++ b/lib/dpif-offload.h > @@ -32,7 +32,7 @@ struct dpif_offload_dump { > > > /* Global functions. */ > -void dpif_offload_set_global_cfg(const struct smap *other_cfg); > +void dpif_offload_set_global_cfg(const struct ovsrec_open_vswitch *); > bool dpif_offload_is_offload_enabled(void); > bool dpif_offload_is_offload_rebalance_policy_enabled(void); [ ... ] > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 62dec1391..20eec1e4d 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -3395,8 +3395,11 @@ bridge_run(void) > } > cfg = ovsrec_open_vswitch_first(idl); > > + if (cfg && ovsdb_idl_get_seqno(idl) != idl_seqno) { > + dpif_offload_set_global_cfg(cfg); > + } There are extra spaces before the 'if' statement. Should this follow the standard indentation with 4 spaces instead of 5? > + > if (cfg) { > - dpif_offload_set_global_cfg(&cfg->other_config); > netdev_set_flow_api_enabled(&cfg->other_config); > dpdk_init(&cfg->other_config); > userspace_tso_init(&cfg->other_config); [ ... ]
diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c index bcb964af8..3f7bbac70 100644 --- a/lib/dpif-offload.c +++ b/lib/dpif-offload.c @@ -23,6 +23,8 @@ #include "netdev-provider.h" #include "unixctl.h" #include "util.h" +#include "vswitch-idl.h" + #include "openvswitch/dynamic-string.h" #include "openvswitch/shash.h" #include "openvswitch/vlog.h" @@ -53,6 +55,7 @@ static const struct dpif_offload_class *base_dpif_offload_classes[] = { static char *dpif_offload_provider_priority_list = "tc,dpdk,dummy,dummy_x"; static atomic_bool dpif_offload_global_enabled = false; static atomic_bool dpif_offload_rebalance_policy = false; +static struct smap port_order_cfg = SMAP_INITIALIZER(&port_order_cfg); static int dpif_offload_register_provider__(const struct dpif_offload_class *class) @@ -418,34 +421,79 @@ dpif_offload_set_netdev_offload(struct netdev *netdev, ovsrcu_set(&netdev->dpif_offload, offload); } +static bool +dpif_offload_try_port_add(struct dpif_offload *offload, struct netdev *netdev, + odp_port_t port_no) +{ + if (offload->class->can_offload(offload, netdev)) { + int err = offload->class->port_add(offload, netdev, port_no); + if (!err) { + VLOG_DBG("netdev %s added to dpif-offload provider %s", + netdev_get_name(netdev), dpif_offload_name(offload)); + return true; + } else { + VLOG_ERR("Failed adding netdev %s to dpif-offload provider " + "%s, error %s", + netdev_get_name(netdev), dpif_offload_name(offload), + ovs_strerror(err)); + } + } else { + VLOG_DBG("netdev %s failed can_offload for dpif-offload provider %s", + netdev_get_name(netdev), dpif_offload_name(offload)); + } + return false; +} + void dpif_offload_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t port_no) { struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); + const char *port_priority = smap_get(&port_order_cfg, + netdev_get_name(netdev)); struct dpif_offload *offload; if (!dp_offload) { return; } - LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) { - if (offload->class->can_offload(offload, netdev)) { - int err = offload->class->port_add(offload, netdev, port_no); - if (!err) { - VLOG_DBG("netdev %s added to dpif-offload provider %s", - netdev_get_name(netdev), dpif_offload_name(offload)); + if (port_priority) { + char *tokens = xstrdup(port_priority); + char *saveptr; + + VLOG_DBG("for netdev %s using port priority %s", + netdev_get_name(netdev), port_priority); + + for (char *name = strtok_r(tokens, ",", &saveptr); + name; + name = strtok_r(NULL, ",", &saveptr)) { + bool provider_added = false; + + if (!strcmp("none", name)) { + break; + } + + LIST_FOR_EACH (offload, dpif_list_node, + &dp_offload->offload_providers) { + if (!strcmp(name, offload->class->type)) { + + provider_added = dpif_offload_try_port_add(offload, netdev, + port_no); + break; + } + } + + if (provider_added) { + break; + } + } + free(tokens); + } else { + LIST_FOR_EACH (offload, dpif_list_node, + &dp_offload->offload_providers) { + if (dpif_offload_try_port_add(offload, netdev, port_no)) { break; - } else { - VLOG_ERR("Failed adding netdev %s to dpif-offload provider " - "%s, error %s", - netdev_get_name(netdev), dpif_offload_name(offload), - ovs_strerror(err)); } - } else { - VLOG_DBG( - "netdev %s failed can_offload for dpif-offload provider %s", - netdev_get_name(netdev), dpif_offload_name(offload)); } } } @@ -559,13 +607,16 @@ dpif_offload_dump_done(struct dpif_offload_dump *dump) } void -dpif_offload_set_global_cfg(const struct smap *other_cfg) +dpif_offload_set_global_cfg(const struct ovsrec_open_vswitch *cfg) { static struct ovsthread_once init_once = OVSTHREAD_ONCE_INITIALIZER; - const char *priority = smap_get(other_cfg, "hw-offload-priority"); + const struct smap *other_cfg = &cfg->other_config; + const char *priority; /* The 'hw-offload-priority' parameter can only be set at startup, * any successive change needs a restart. */ + priority = smap_get(other_cfg, "hw-offload-priority"); + if (ovsthread_once_start(&init_once)) { /* Initialize the dpif-offload layer in case it's not yet initialized * at the first invocation of setting the configuration. */ @@ -618,6 +669,25 @@ dpif_offload_set_global_cfg(const struct smap *other_cfg) ovsthread_once_done(&once_enable); } } + + /* Filter out the 'hw-offload-priority' per port setting we need it before + * ports are added, so we can assign the correct offload-provider. + * Note that we can safely rebuild the map here, as we only access this + * from the same (main) thread. */ + smap_clear(&port_order_cfg); + for (int i = 0; i < cfg->n_bridges; i++) { + const struct ovsrec_bridge *br_cfg = cfg->bridges[i]; + + for (int j = 0; j < br_cfg->n_ports; j++) { + const struct ovsrec_port *port_cfg = br_cfg->ports[j]; + + priority = smap_get(&port_cfg->other_config, + "hw-offload-priority"); + if (priority) { + smap_add(&port_order_cfg, port_cfg->name, priority); + } + } + } } diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h index 87adcd0fc..2720d2b5f 100644 --- a/lib/dpif-offload.h +++ b/lib/dpif-offload.h @@ -32,7 +32,7 @@ struct dpif_offload_dump { /* Global functions. */ -void dpif_offload_set_global_cfg(const struct smap *other_cfg); +void dpif_offload_set_global_cfg(const struct ovsrec_open_vswitch *); bool dpif_offload_is_offload_enabled(void); bool dpif_offload_is_offload_rebalance_policy_enabled(void); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 62dec1391..20eec1e4d 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -3395,8 +3395,11 @@ bridge_run(void) } cfg = ovsrec_open_vswitch_first(idl); + if (cfg && ovsdb_idl_get_seqno(idl) != idl_seqno) { + dpif_offload_set_global_cfg(cfg); + } + if (cfg) { - dpif_offload_set_global_cfg(&cfg->other_config); netdev_set_flow_api_enabled(&cfg->other_config); dpdk_init(&cfg->other_config); userspace_tso_init(&cfg->other_config); diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 1c298b10e..f85227aec 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -2599,6 +2599,30 @@ e.g. <code>fake-bridge-bridge-id</code>. </column> + <column name="other_config" key="hw-offload-priority" + type='{"type": "string"}'> + <p> + This configuration sets an explicit list of hardware offload + providers to try on this port. The argument should be a + comma-separated list of hardware offload provider names, or the + word <code>none</code>. + </p> + <p> + If <code>none</code> is encountered in the list, further trying of + offload providers is stopped. For example, if the list only contains + <code>none</code>, hardware offload is disabled on this port even if + it is globally enabled. Note that unknown hardware offload provider + names are ignored. + </p> + <p> + The default value is <code>none</code>, i.e., not set, which uses the + global + <ref table="Open_vSwitch" column="other_config" key="hw-offload-priority"/> + configuration. Changing this value after offload enablement requires + restarting the daemon. + </p> + </column> + <column name="other_config" key="transient" type='{"type": "boolean"}'> <p>
This patch adds support for configuring the priority of offload providers at a port level. When multiple providers exist and support the same port, the 'hw-offload-priority' option allows specifying the order in which they are tried. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/dpif-offload.c | 104 ++++++++++++++++++++++++++++++++++++------- lib/dpif-offload.h | 2 +- vswitchd/bridge.c | 5 ++- vswitchd/vswitch.xml | 24 ++++++++++ 4 files changed, 116 insertions(+), 19 deletions(-)