Eelco Chaudron via dev <[email protected]> writes:
> This patch adds support for configuring the priority of
> offload providers. 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 <[email protected]>
> ---
>
> v2 changes:
> - Fixed a memory leak with the 'dpif_offload_provider_priority_list'
> variable.
>
> v3 changes:
> - Fixed some new line style issues.
> - Removed duplicate code in dpif_offload_attach_providers_().
> ---
> lib/dpif-offload.c | 124 ++++++++++++++++++++++++++++++++++------
> lib/dpif-offload.h | 4 ++
> tests/ofproto-dpif.at | 37 +++++++++++-
> tests/ofproto-macros.at | 9 ++-
> vswitchd/bridge.c | 2 +
> vswitchd/vswitch.xml | 19 ++++++
> 6 files changed, 175 insertions(+), 20 deletions(-)
>
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 6aae46356..60ee0a07d 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -43,10 +43,16 @@ static const struct dpif_offload_class
> *base_dpif_offload_classes[] = {
> #ifdef DPDK_NETDEV
> &dpif_offload_dpdk_class,
> #endif
> + /* If you add an offload class to this structure, make sure you also
> + * update the dpif_offload_provider_priority_list below. */
> &dpif_offload_dummy_class,
> &dpif_offload_dummy_x_class,
> };
>
> +#define DEFAULT_PROVIDER_PRIORITY_LIST "tc,dpdk,dummy,dummy_x"
> +
> +static char *dpif_offload_provider_priority_list = NULL;
> +
> static int
> dpif_offload_register_provider__(const struct dpif_offload_class *class)
> OVS_REQUIRES(dpif_offload_mutex)
> @@ -131,6 +137,11 @@ dp_offload_initialize(void)
> return;
> }
>
> + if (!dpif_offload_provider_priority_list) {
> + dpif_offload_provider_priority_list = xstrdup(
> + DEFAULT_PROVIDER_PRIORITY_LIST);
> + }
> +
> unixctl_command_register("dpif/offload/classes", NULL, 0, 0,
> dpif_offload_show_classes, NULL);
>
> @@ -190,13 +201,37 @@ dpif_offload_attach_dp_offload(struct dpif *dpif,
> return 0;
> }
>
> +static void
> +dpif_offload_attach_providers__(struct dpif *dpif,
> + struct dp_offload *dp_offload,
> + struct dpif_offload *offload)
> + OVS_REQUIRES(dpif_offload_mutex)
> +{
> + int error;
> +
> + ovs_list_remove(&offload->dpif_list_node);
> + error = dpif_offload_attach_provider_to_dp_offload(dp_offload,
> + offload);
> + if (error) {
> + VLOG_WARN(
> + "failed to add dpif offload provider %s to %s: %s",
> + offload->class->type, dpif_name(dpif),
> + ovs_strerror(error));
> +
> + offload->class->close(offload);
> + }
> +}
> +
> static int
> dpif_offload_attach_providers_(struct dpif *dpif)
> OVS_REQUIRES(dpif_offload_mutex)
> {
> + struct ovs_list provider_list = OVS_LIST_INITIALIZER(&provider_list);
> const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif));
> struct dp_offload *dp_offload;
> + struct dpif_offload *offload;
> struct shash_node *node;
> + char *tokens, *saveptr;
>
> /* Allocate and attach dp_offload to dpif. */
> dp_offload = xmalloc(sizeof *dp_offload);
> @@ -206,37 +241,51 @@ dpif_offload_attach_providers_(struct dpif *dpif)
> ovs_list_init(&dp_offload->offload_providers);
> shash_add(&dpif_offload_providers, dp_offload->dpif_name, dp_offload);
>
> - /* Attach all the providers supporting this dpif type. */
> + /* Open all the providers supporting this dpif type. */
> SHASH_FOR_EACH (node, &dpif_offload_classes) {
> const struct dpif_offload_class *class = node->data;
>
> for (size_t i = 0; class->supported_dpif_types[i] != NULL; i++) {
> if (!strcmp(class->supported_dpif_types[i], dpif_type_str)) {
> - struct dpif_offload *offload;
> - int error;
> -
> - error = class->open(class, dpif, &offload);
> - if (!error) {
> - error = dpif_offload_attach_provider_to_dp_offload(
> - dp_offload, offload);
> - if (error) {
> - VLOG_WARN("failed to add dpif offload provider "
> - "%s to %s: %s",
> - class->type, dpif_name(dpif),
> - ovs_strerror(error));
> - class->close(offload);
> - }
> - } else {
> + int error = class->open(class, dpif, &offload);
> +
> + if (error) {
> VLOG_WARN("failed to initialize dpif offload provider "
> "%s for %s: %s",
> class->type, dpif_name(dpif),
> ovs_strerror(error));
> + } else {
> + ovs_list_push_back(&provider_list,
> + &offload->dpif_list_node);
> }
> break;
Weird - I must be misunderstanding, but I would expect that this
wouldn't actually properly seek through the list, because the first
match would land on this break. Did I miss something?
> }
> }
> }
>
> + /* Attach all the providers based on the priority list. */
> + tokens = xstrdup(dpif_offload_provider_priority_list);
> +
> + for (char *name = strtok_r(tokens, ",", &saveptr);
> + name;
> + name = strtok_r(NULL, ",", &saveptr)) {
> +
> + LIST_FOR_EACH_SAFE (offload, dpif_list_node, &provider_list) {
> + if (strcmp(name, offload->class->type)) {
> + continue;
> + }
> +
> + dpif_offload_attach_providers__(dpif, dp_offload, offload);
> + break;
> + }
> + }
> + free(tokens);
> +
> + /* Add remaining entries in order. */
> + LIST_FOR_EACH_SAFE (offload, dpif_list_node, &provider_list) {
> + dpif_offload_attach_providers__(dpif, dp_offload, offload);
> + }
> +
> /* Attach dp_offload to dpif. */
> ovsrcu_set(&dpif->dp_offload, dp_offload);
>
> @@ -399,3 +448,46 @@ dpif_offload_dump_done(struct dpif_offload_dump *dump)
> {
> return dump->error == EOF ? 0 : dump->error;
> }
> +
> +void
> +dpif_offload_set_global_cfg(const struct smap *other_cfg)
> +{
> + static struct ovsthread_once init_once = OVSTHREAD_ONCE_INITIALIZER;
> + const char *priority = smap_get(other_cfg, "hw-offload-priority");
> +
> + /* The 'hw-offload-priority' parameter can only be set at startup,
> + * any successive change needs a restart. */
> + 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. */
> + dp_offload_initialize();
> +
> + /* If priority is not set keep the default value. */
> + if (priority) {
> + char *tokens = xstrdup(priority);
> + char *saveptr;
> +
> + free(dpif_offload_provider_priority_list);
> + dpif_offload_provider_priority_list = xstrdup(priority);
> +
> + /* Log a warning for unknown offload providers. */
> + for (char *name = strtok_r(tokens, ",", &saveptr);
> + name;
> + name = strtok_r(NULL, ",", &saveptr)) {
> +
> + if (!shash_find(&dpif_offload_classes, name)) {
> + VLOG_WARN("'hw-offload-priority' configuration has an "
> + "unknown type; %s", name);
If a weird config gets into this, maybe it's better to also remove it
from the in-memory list. WDYT?
> + }
> + }
> + free(tokens);
> + }
> + ovsthread_once_done(&init_once);
> + } else {
> + if (priority && strcmp(priority,
> + dpif_offload_provider_priority_list)) {
> + VLOG_INFO_ONCE("'hw-offload-priority' configuration changed; "
> + "restart required");
> + }
> + }
> +}
> diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
> index d61d8e0bf..43d8b6b74 100644
> --- a/lib/dpif-offload.h
> +++ b/lib/dpif-offload.h
> @@ -30,6 +30,10 @@ struct dpif_offload_dump {
> void *state;
> };
>
> +
> +/* Global functions. */
> +void dpif_offload_set_global_cfg(const struct smap *other_cfg);
> +
>
> /* Per dpif specific functions. */
> void dpif_offload_init(struct dpif_offload *,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 11f9e346a..1d80fc439 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -10108,6 +10108,41 @@ AT_SETUP([ofproto-dpif - offload - ovs-appctl
> dpif/offload/])
> AT_KEYWORDS([dpif-offload])
> OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy])
>
> +AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl
> +dummy@ovs-dummy:
> + dummy
> + dummy_x
> +])
> +
> +AT_CHECK([ovs-appctl --format json --pretty dpif/offload/show], [0], [dnl
> +{
> + "dummy@ovs-dummy": {
> + "providers": [[
> + "dummy",
> + "dummy_x"]]}}
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - offload - priority order - warning])
> +AT_KEYWORDS([dpif-offload])
> +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy], [],
> [],
> + [], [-- set Open_vSwitch . other_config:hw-offload-priority=none,dummy])
> +
> +OVS_WAIT_UNTIL(
> + [grep "'hw-offload-priority' configuration has an unknown type; none" \
> + ovs-vswitchd.log])
> +
> +OVS_VSWITCHD_STOP(
> + ["/'hw-offload-priority' configuration has an unknown type;/d"])
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - offload - priority order])
> +AT_KEYWORDS([dpif-offload])
> +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy], [],
> [],
> + [], [-- set Open_vSwitch . other_config:hw-offload-priority=dummy_x,dummy])
> +
> AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl
> dummy@ovs-dummy:
> dummy_x
> @@ -10122,7 +10157,7 @@ AT_CHECK([ovs-appctl --format json --pretty
> dpif/offload/show], [0], [dnl
> "dummy"]]}}
> ])
>
> -OVS_VSWITCHD_STOP
> +OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> dnl ----------------------------------------------------------------------
> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> index 3cd49d7a7..71cd2aed0 100644
> --- a/tests/ofproto-macros.at
> +++ b/tests/ofproto-macros.at
> @@ -227,11 +227,12 @@ m4_define([_OVS_VSWITCHD_START],
> /netdev_offload|INFO|netdev: Flow API Enabled/d
> /probe tc:/d
> /setting extended ack support failed/d
> -/tc: Using policy/d']])
> +/tc: Using policy/d
> +/'"'"'hw-offload-priority'"'"' configuration has an unknown type;/d']])
> ])
>
> # OVS_VSWITCHD_START([vsctl-args], [vsctl-output], [=override],
> -# [vswitchd-aux-args])
> +# [vswitchd-aux-args] [dbinit-aux-args])
> #
> # Creates a database and starts ovsdb-server, starts ovs-vswitchd
> # connected to that database, calls ovs-vsctl to create a bridge named
> @@ -248,7 +249,9 @@ m4_define([_OVS_VSWITCHD_START],
> # 'vswitchd-aux-args' provides a way to pass extra command line arguments
> # to ovs-vswitchd
> m4_define([OVS_VSWITCHD_START],
> - [_OVS_VSWITCHD_START([--enable-dummy$3 --disable-system
> --disable-system-route $4])
> + [_OVS_VSWITCHD_START(
> + [--enable-dummy$3 --disable-system --disable-system-route $4],
> + [$5])
> AT_CHECK([add_of_br 0 $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2])
> ])
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 475eefefa..62dec1391 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -28,6 +28,7 @@
> #include "daemon.h"
> #include "dirs.h"
> #include "dpif.h"
> +#include "dpif-offload.h"
> #include "dpdk.h"
> #include "hash.h"
> #include "openvswitch/hmap.h"
> @@ -3395,6 +3396,7 @@ bridge_run(void)
> cfg = ovsrec_open_vswitch_first(idl);
>
> 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 b93ad9344..e235f7e79 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -254,6 +254,25 @@
> </p>
> </column>
>
> + <column name="other_config" key="hw-offload-priority"
> + type='{"type": "string"}'>
> + <p>
> + This configuration sets the order of the hardware offload providers
> + to try when multiple exist for a given datapath implementation.
> The
> + The argument provided should be a list of comma-separated hardware
> + offload provider names.
> + </p>
> + <p>
> + If implementations are missing from the list, they are added at the
> + end in undefined order.
> + </p>
> + <p>
> + The default value is <code>tc,dpdk,dummy,dummy_x</code>. Changing
> + this value requires restarting the daemon if hardware offload is
> + already enabled.
> + </p>
> + </column>
> +
> <column name="other_config" key="n-offload-threads"
> type='{"type": "integer", "minInteger": 1, "maxInteger": 10}'>
> <p>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
