Eelco Chaudron via dev <[email protected]> writes:
> This patch introduces an API that allows offload providers to
> manage global configurations. It also moves the 'n-offload-threads'
> and 'tc-policy' setting to the appropriate providers using
> this new API.
>
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
> lib/dpif-offload-dpdk.c | 82 ++++++++++++++++++++++++++++++++++---
> lib/dpif-offload-provider.h | 17 +++++++-
> lib/dpif-offload-tc.c | 52 +++++++++++++++++++++--
> lib/dpif-offload.c | 18 ++++++++
> lib/dpif.c | 1 +
> lib/netdev-offload.c | 37 ++++++-----------
> tests/dpif-netdev.at | 6 +--
> tests/ofproto-macros.at | 2 +-
> 8 files changed, 176 insertions(+), 39 deletions(-)
>
> diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c
> index 838a6839b..9a0d62b20 100644
> --- a/lib/dpif-offload-dpdk.c
> +++ b/lib/dpif-offload-dpdk.c
> @@ -20,21 +20,84 @@
> #include "dpif-offload-provider.h"
> #include "util.h"
>
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dpif_offload_dpdk);
> +
> +#define DEFAULT_OFFLOAD_THREAD_NB 1
> +#define MAX_OFFLOAD_THREAD_NB 10
> +
> +static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB;
> +
> +/* dpif offload interface for the dpdk rte_flow implementation. */
> +struct dpif_offload_dpdk {
> + struct dpif_offload offload;
> +
> + /* Configuration specific variables. */
> + struct ovsthread_once once_enable; /* Track first-time enablement. */
> +};
> +
> +static struct dpif_offload_dpdk *
> +dpif_offload_dpdk_cast(const struct dpif_offload *offload)
> +{
> + dpif_offload_assert_class(offload, &dpif_offload_dpdk_class);
> + return CONTAINER_OF(offload, struct dpif_offload_dpdk, offload);
> +}
> +
> static int
> dpif_offload_dpdk_open(const struct dpif_offload_class *offload_class,
> struct dpif *dpif, struct dpif_offload **offload_)
> {
> - struct dpif_offload *offload = xmalloc(sizeof *offload);
> + struct dpif_offload_dpdk *offload;
> +
> + offload = xmalloc(sizeof *offload);
> +
> + dpif_offload_init(&offload->offload, offload_class, dpif);
> + offload->once_enable = (struct ovsthread_once)
> OVSTHREAD_ONCE_INITIALIZER;
>
> - dpif_offload_init(offload, offload_class, dpif);
> - *offload_ = offload;
> + *offload_ = &offload->offload;
> return 0;
> }
>
> static void
> -dpif_offload_dpdk_close(struct dpif_offload *dpif_offload)
> +dpif_offload_dpdk_close(struct dpif_offload *offload_)
> {
> - free(dpif_offload);
> + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_);
> +
> + free(offload);
> +}
> +
> +static void
> +dpif_offload_dpdk_set_config(struct dpif_offload *offload_,
> + const struct smap *other_cfg)
> +{
> + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_);
> +
> + /* We maintain the existing behavior where global configurations
> + * are only accepted when hardware offload is initially enabled.
> + * Once enabled, they cannot be updated or reconfigured. */
> + if (smap_get_bool(other_cfg, "hw-offload", false)) {
For this and the TC block, does it make sense to have a NEWS section and
also a warning that these global configs may be disappearing? Or
rather, I guess the idea is that there can be a global config and then a
per-dp offload state that shadows it? Otherwise, I don't understand the
comment block - maybe it would be better to have that as part of the
commit message. WDYT?
> + if (ovsthread_once_start(&offload->once_enable)) {
> +
> + offload_thread_nb = smap_get_ullong(other_cfg,
> + "n-offload-threads",
> + DEFAULT_OFFLOAD_THREAD_NB);
> + if (offload_thread_nb == 0 ||
> + offload_thread_nb > MAX_OFFLOAD_THREAD_NB) {
> + VLOG_WARN("netdev: Invalid number of threads requested: %u",
> + offload_thread_nb);
> + offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB;
> + }
> +
> + if (smap_get(other_cfg, "n-offload-threads")) {
> + VLOG_INFO("Flow API using %u thread%s",
> + offload_thread_nb,
> + offload_thread_nb > 1 ? "s" : "");
> + }
> +
> + ovsthread_once_done(&offload->once_enable);
> + }
> + }
> }
>
> struct dpif_offload_class dpif_offload_dpdk_class = {
> @@ -44,4 +107,13 @@ struct dpif_offload_class dpif_offload_dpdk_class = {
> NULL},
> .open = dpif_offload_dpdk_open,
> .close = dpif_offload_dpdk_close,
> + .set_config = dpif_offload_dpdk_set_config,
> };
> +
> +/* XXX: Temporary functions below, which will be removed once fully
> + * refactored. */
> +unsigned int dpif_offload_dpdk_get_thread_nb(void);
> +unsigned int dpif_offload_dpdk_get_thread_nb(void)
> +{
> + return offload_thread_nb;
> +}
> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
> index a7869b587..f0c0ea232 100644
> --- a/lib/dpif-offload-provider.h
> +++ b/lib/dpif-offload-provider.h
> @@ -19,6 +19,8 @@
>
> #include "dpif-provider.h"
> #include "ovs-thread.h"
> +#include "smap.h"
> +#include "util.h"
>
> #include "openvswitch/list.h"
>
> @@ -85,6 +87,11 @@ struct dpif_offload_class {
> * open() above. If your implementation accesses this provider using
> * RCU pointers, it's responsible for handling deferred deallocation. */
> void (*close)(struct dpif_offload *);
> + /* Pass custom configuration options to the offload provider. The
> + * implementation might postpone applying the changes until run() is
> + * called. */
> + void (*set_config)(struct dpif_offload *,
> + const struct smap *other_config);
> };
>
>
> @@ -94,8 +101,16 @@ extern struct dpif_offload_class dpif_offload_dpdk_class;
> extern struct dpif_offload_class dpif_offload_tc_class;
>
>
> -/* Global function, called by the dpif layer. */
> +/* Global functions, called by the dpif layer or offload providers. */
> void dp_offload_initialize(void);
> +void dpif_offload_set_config(struct dpif *, const struct smap *other_cfg);
> +
> +static inline void dpif_offload_assert_class(
> + const struct dpif_offload *dpif_offload,
> + const struct dpif_offload_class *dpif_offload_class)
> +{
> + ovs_assert(dpif_offload->class == dpif_offload_class);
> +}
>
>
> #endif /* DPIF_OFFLOAD_PROVIDER_H */
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index 88efdaac8..d0204e1bb 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -18,23 +18,66 @@
>
> #include "dpif-offload.h"
> #include "dpif-offload-provider.h"
> +#include "tc.h"
> #include "util.h"
>
> +/* dpif offload interface for the tc implementation. */
> +struct dpif_offload_tc {
> + struct dpif_offload offload;
> +
> + /* Configuration specific variables. */
> + struct ovsthread_once once_enable; /* Track first-time enablement. */
> +};
> +
> +static struct dpif_offload_tc *
> +dpif_offload_tc_cast(const struct dpif_offload *offload)
> +{
> + dpif_offload_assert_class(offload, &dpif_offload_tc_class);
> + return CONTAINER_OF(offload, struct dpif_offload_tc, offload);
> +}
> +
> static int
> dpif_offload_tc_open(const struct dpif_offload_class *offload_class,
> struct dpif *dpif, struct dpif_offload **dpif_offload)
> {
> - struct dpif_offload *offload = xmalloc(sizeof *offload);
> + struct dpif_offload_tc *offload_tc;
> +
> + offload_tc = xmalloc(sizeof *offload_tc);
> +
> + dpif_offload_init(&offload_tc->offload, offload_class, dpif);
> + offload_tc->once_enable = (struct ovsthread_once) \
> + OVSTHREAD_ONCE_INITIALIZER;
>
> - dpif_offload_init(offload, offload_class, dpif);
> - *dpif_offload = offload;
> + *dpif_offload = &offload_tc->offload;
> return 0;
> }
>
> static void
> dpif_offload_tc_close(struct dpif_offload *dpif_offload)
> {
> - free(dpif_offload);
> + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(dpif_offload);
> +
> + free(offload_tc);
> +}
> +
> +static void
> +dpif_offload_tc_set_config(struct dpif_offload *offload,
> + const struct smap *other_cfg)
> +{
> + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(offload);
> +
> + /* We maintain the existing behavior where global configurations
> + * are only accepted when hardware offload is initially enabled.
> + * Once enabled, they cannot be updated or reconfigured. */
> + if (smap_get_bool(other_cfg, "hw-offload", false)) {
> + if (ovsthread_once_start(&offload_tc->once_enable)) {
> +
> + tc_set_policy(smap_get_def(other_cfg, "tc-policy",
> + TC_POLICY_DEFAULT));
> +
> + ovsthread_once_done(&offload_tc->once_enable);
> + }
> + }
> }
>
> struct dpif_offload_class dpif_offload_tc_class = {
> @@ -44,4 +87,5 @@ struct dpif_offload_class dpif_offload_tc_class = {
> NULL},
> .open = dpif_offload_tc_open,
> .close = dpif_offload_tc_close,
> + .set_config = dpif_offload_tc_set_config,
> };
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 7921207be..1b5f0ae71 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -356,6 +356,23 @@ dpif_offload_detach_providers(struct dpif *dpif)
> }
> }
>
> +void
> +dpif_offload_set_config(struct dpif *dpif, const struct smap *other_cfg)
> +{
> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif);
> + struct dpif_offload *offload;
> +
> + if (!dp_offload) {
> + return;
> + }
> +
> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) {
> + if (offload->class->set_config) {
> + offload->class->set_config(offload, other_cfg);
> + }
> + }
> +}
> +
>
> void
> dpif_offload_init(struct dpif_offload *offload,
> @@ -521,6 +538,7 @@ dpif_offload_set_global_cfg(const struct smap *other_cfg)
>
> if (ovsthread_once_start(&once_enable)) {
> atomic_store_relaxed(&dpif_offload_global_enabled, true);
> + VLOG_INFO("hw-offload API Enabled");
>
> if (smap_get_bool(other_cfg, "offload-rebalance", false)) {
> atomic_store_relaxed(&dpif_offload_rebalance_policy, true);
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 91fa00888..4d4b5127b 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1594,6 +1594,7 @@ dpif_set_config(struct dpif *dpif, const struct smap
> *cfg)
> if (error) {
> log_operation(dpif, "set_config", error);
> }
> + dpif_offload_set_config(dpif, cfg);
> }
>
> return error;
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 01fdadbc3..0fad1b983 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -61,10 +61,16 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload);
>
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> -#define DEFAULT_OFFLOAD_THREAD_NB 1
> +/* XXX: Temporarily duplicates definition in dpif-offload-dpdk.c. */
> #define MAX_OFFLOAD_THREAD_NB 10
> -
> +#define DEFAULT_OFFLOAD_THREAD_NB 1
> static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB;
> +
> +unsigned int dpif_offload_dpdk_get_thread_nb(void); /* XXX: Temporarily
> + * external declaration
> + * until fully
> refactored.
> + */
> +
> DEFINE_EXTERN_PER_THREAD_DATA(netdev_offload_thread_id, OVSTHREAD_ID_UNSET);
>
> /* Protects 'netdev_flow_apis'. */
> @@ -853,37 +859,18 @@ netdev_ports_flow_init(void)
> }
>
> void
> -netdev_set_flow_api_enabled(const struct smap *ovs_other_config)
> +netdev_set_flow_api_enabled(const struct smap *ovs_other_config OVS_UNUSED)
> {
> if (dpif_offload_is_offload_enabled()) {
> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>
> if (ovsthread_once_start(&once)) {
>
> - offload_thread_nb = smap_get_ullong(ovs_other_config,
> - "n-offload-threads",
> - DEFAULT_OFFLOAD_THREAD_NB);
> - if (offload_thread_nb == 0 ||
> - offload_thread_nb > MAX_OFFLOAD_THREAD_NB) {
> - VLOG_WARN("netdev: Invalid number of threads requested: %u",
> - offload_thread_nb);
> - offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB;
> - }
> -
> - if (smap_get(ovs_other_config, "n-offload-threads")) {
> - VLOG_INFO("netdev: Flow API Enabled, using %u thread%s",
> - offload_thread_nb,
> - offload_thread_nb > 1 ? "s" : "");
> - } else {
> - VLOG_INFO("netdev: Flow API Enabled");
> - }
> -
> -#ifdef __linux__
> - tc_set_policy(smap_get_def(ovs_other_config, "tc-policy",
> - TC_POLICY_DEFAULT));
> +#ifdef DPDK_NETDEV
> + offload_thread_nb = dpif_offload_dpdk_get_thread_nb();
> #endif
> - netdev_ports_flow_init();
>
> + netdev_ports_flow_init();
> ovsthread_once_done(&once);
> }
> }
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 10dbbb4c7..6596af810 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -414,7 +414,7 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD],
> AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg
> netdev_dummy:file:dbg])
>
> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
> - OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log])
> + OVS_WAIT_UNTIL([grep "hw-offload API Enabled" ovs-vswitchd.log])
>
> AT_CHECK([ovs-ofctl del-flows br0])
> AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT])
> @@ -477,7 +477,7 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS],
> AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg
> netdev_dummy:file:dbg])
>
> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
> - OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log])
> + OVS_WAIT_UNTIL([grep "hw-offload API Enabled" ovs-vswitchd.log])
>
> AT_CHECK([ovs-ofctl del-flows br0])
>
> @@ -554,7 +554,7 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP],
> AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg
> netdev_dummy:file:dbg])
>
> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
> - OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log])
> + OVS_WAIT_UNTIL([grep "hw-offload API Enabled" ovs-vswitchd.log])
>
> AT_CHECK([ovs-ofctl del-flows br0])
>
> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> index 71cd2aed0..8894dd711 100644
> --- a/tests/ofproto-macros.at
> +++ b/tests/ofproto-macros.at
> @@ -224,7 +224,7 @@ m4_define([_OVS_VSWITCHD_START],
> /ofproto|INFO|datapath ID changed to fedcba9876543210/d
> /dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d
> /netlink_socket|INFO|netlink: could not enable listening to all nsid/d
> -/netdev_offload|INFO|netdev: Flow API Enabled/d
> +/dpif_offload|INFO|hw-offload API Enabled/d
> /probe tc:/d
> /setting extended ack support failed/d
> /tc: Using policy/d
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
