git sha: a004e6f76a3e4e6fbf3a6f5577644d60f03d2281
Author: Eelco Chaudron <[email protected]>
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);
[ ... ]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
