| Message ID | 20190515153601.31636-2-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | netdev: Dynamic per-port Flow API +Offload Split Up. | expand |
On 15/05/2019 18:35, Ilya Maximets wrote: > Current issues with Flow API: > > * OVS calls offloading functions regardless of successful > flow API initialization. (ex. on init_flow_api failure) > * Static initilaization of Flow API for a netdev_class forbids > having different offloading types for different instances > of netdev with the same netdev_class. (ex. different vports in > 'system' and 'netdev' datapaths at the same time) > > Solution: > > * Move Flow API from the netdev_class to netdev instance. > * Make Flow API dynamic, i.e. probe the APIs and choose the > suitable one. > > Side effects: > > * Flow API providers localized as possible in their modules. > * Now we have an ability to make runtime checks. For example, > we could check if particular device supports features we > need, like if dpdk device supports RSS+MARK action. > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > lib/automake.mk | 2 +- > lib/dpdk.c | 2 + > lib/netdev-dpdk.c | 24 +++- > lib/netdev-dpdk.h | 3 + > lib/netdev-dummy.c | 24 +++- > lib/netdev-linux.c | 3 - > lib/netdev-linux.h | 10 -- > lib/netdev-offload-provider.h | 99 +++++++++++++++ > lib/netdev-provider.h | 67 +---------- > lib/netdev-rte-offloads.c | 40 +++++- > lib/netdev-rte-offloads.h | 41 +------ > lib/netdev-tc-offloads.c | 39 ++++-- > lib/netdev-tc-offloads.h | 44 ------- > lib/netdev-vport.c | 6 +- > lib/netdev.c | 221 +++++++++++++++++++++++++++------- > tests/dpif-netdev.at | 4 +- > tests/ofproto-macros.at | 1 - > 17 files changed, 398 insertions(+), 232 deletions(-) > create mode 100644 lib/netdev-offload-provider.h > delete mode 100644 lib/netdev-tc-offloads.h > > diff --git a/lib/automake.mk b/lib/automake.mk > index cc5dccf39..c70fda3f8 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -137,6 +137,7 @@ lib_libopenvswitch_la_SOURCES = \ > lib/namemap.c \ > lib/netdev-dpdk.h \ > lib/netdev-dummy.c \ > + lib/netdev-offload-provider.h \ > lib/netdev-provider.h \ > lib/netdev-rte-offloads.h \ > lib/netdev-vport.c \ > @@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \ > lib/netdev-linux.c \ > lib/netdev-linux.h \ > lib/netdev-tc-offloads.c \ > - lib/netdev-tc-offloads.h \ > lib/netlink-conntrack.c \ > lib/netlink-conntrack.h \ > lib/netlink-notifier.c \ > diff --git a/lib/dpdk.c b/lib/dpdk.c > index dc6171546..6c6298635 100644 > --- a/lib/dpdk.c > +++ b/lib/dpdk.c > @@ -34,6 +34,7 @@ > #include "dirs.h" > #include "fatal-signal.h" > #include "netdev-dpdk.h" > +#include "netdev-rte-offloads.h" > #include "openvswitch/dynamic-string.h" > #include "openvswitch/vlog.h" > #include "ovs-numa.h" > @@ -442,6 +443,7 @@ dpdk_init__(const struct smap *ovs_other_config) > > /* Finally, register the dpdk classes */ > netdev_dpdk_register(); > + netdev_dpdk_flow_api_register(); > return true; > } > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 47153dc60..c06c9ef81 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -4204,6 +4204,27 @@ unlock: > return err; > } > > +bool > +netdev_dpdk_flow_api_supported(struct netdev *netdev) > +{ > + struct netdev_dpdk *dev; > + bool ret = false; > + > + if (!is_dpdk_class(netdev->netdev_class)) { > + goto out; > + } > + > + dev = netdev_dpdk_cast(netdev); > + ovs_mutex_lock(&dev->mutex); > + if (dev->type == DPDK_DEV_ETH) { > + /* TODO: Check if we able to offload some minimal flow. */ > + ret = true; > + } > + ovs_mutex_unlock(&dev->mutex); > +out: > + return ret; > +} > + > int > netdev_dpdk_rte_flow_destroy(struct netdev *netdev, > struct rte_flow *rte_flow, > @@ -4268,8 +4289,7 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev, > .get_features = netdev_dpdk_get_features, \ > .get_status = netdev_dpdk_get_status, \ > .reconfigure = netdev_dpdk_reconfigure, \ > - .rxq_recv = netdev_dpdk_rxq_recv, \ > - DPDK_FLOW_OFFLOAD_API > + .rxq_recv = netdev_dpdk_rxq_recv > > static const struct netdev_class dpdk_class = { > .type = "dpdk", > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h > index 9bbb8d8d6..60631c4f0 100644 > --- a/lib/netdev-dpdk.h > +++ b/lib/netdev-dpdk.h > @@ -34,6 +34,9 @@ struct rte_flow_action; > > void netdev_dpdk_register(void); > void free_dpdk_buf(struct dp_packet *); > + > +bool netdev_dpdk_flow_api_supported(struct netdev *); > + > int > netdev_dpdk_rte_flow_destroy(struct netdev *netdev, > struct rte_flow *rte_flow, > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 3f90ffa09..18eed4cf4 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -24,6 +24,7 @@ > #include "dp-packet.h" > #include "dpif-netdev.h" > #include "flow.h" > +#include "netdev-offload-provider.h" > #include "netdev-provider.h" > #include "netdev-vport.h" > #include "odp-util.h" > @@ -1523,10 +1524,6 @@ exit: > return error ? -1 : 0; > } > > -#define DUMMY_FLOW_OFFLOAD_API \ > - .flow_put = netdev_dummy_flow_put, \ > - .flow_del = netdev_dummy_flow_del > - > #define NETDEV_DUMMY_CLASS_COMMON \ > .run = netdev_dummy_run, \ > .wait = netdev_dummy_wait, \ > @@ -1559,8 +1556,7 @@ exit: > .rxq_dealloc = netdev_dummy_rxq_dealloc, \ > .rxq_recv = netdev_dummy_rxq_recv, \ > .rxq_wait = netdev_dummy_rxq_wait, \ > - .rxq_drain = netdev_dummy_rxq_drain, \ > - DUMMY_FLOW_OFFLOAD_API > + .rxq_drain = netdev_dummy_rxq_drain > > static const struct netdev_class dummy_class = { > NETDEV_DUMMY_CLASS_COMMON, > @@ -1578,6 +1574,20 @@ static const struct netdev_class dummy_pmd_class = { > .is_pmd = true, > .reconfigure = netdev_dummy_reconfigure > }; > + > +static int > +netdev_dummy_offloads_init_flow_api(struct netdev *netdev) > +{ > + return is_dummy_class(netdev->netdev_class) ? 0 : EOPNOTSUPP; > +} > + > +static const struct netdev_flow_api netdev_dummy_offloads = { > + .type = "dummy", > + .flow_put = netdev_dummy_flow_put, > + .flow_del = netdev_dummy_flow_del, > + .init_flow_api = netdev_dummy_offloads_init_flow_api, > +}; > + > > /* Helper functions. */ > > @@ -2024,5 +2034,7 @@ netdev_dummy_register(enum dummy_level level) > netdev_register_provider(&dummy_internal_class); > netdev_register_provider(&dummy_pmd_class); > > + netdev_register_flow_api_provider(&netdev_dummy_offloads); > + > netdev_vport_tunnel_register(); > } > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index f75d73fd3..e4ea94cf9 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -55,7 +55,6 @@ > #include "hash.h" > #include "openvswitch/hmap.h" > #include "netdev-provider.h" > -#include "netdev-tc-offloads.h" > #include "netdev-vport.h" > #include "netlink-notifier.h" > #include "netlink-socket.h" > @@ -3321,7 +3320,6 @@ exit: > > const struct netdev_class netdev_linux_class = { > NETDEV_LINUX_CLASS_COMMON, > - LINUX_FLOW_OFFLOAD_API, > .type = "system", > .construct = netdev_linux_construct, > .get_stats = netdev_linux_get_stats, > @@ -3341,7 +3339,6 @@ const struct netdev_class netdev_tap_class = { > > const struct netdev_class netdev_internal_class = { > NETDEV_LINUX_CLASS_COMMON, > - LINUX_FLOW_OFFLOAD_API, > .type = "internal", > .construct = netdev_linux_construct, > .get_stats = netdev_internal_get_stats, > diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h > index 17ca91201..e1e30f806 100644 > --- a/lib/netdev-linux.h > +++ b/lib/netdev-linux.h > @@ -29,14 +29,4 @@ int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag, > const char *flag_name, bool enable); > int linux_get_ifindex(const char *netdev_name); > > -#define LINUX_FLOW_OFFLOAD_API \ > - .flow_flush = netdev_tc_flow_flush, \ > - .flow_dump_create = netdev_tc_flow_dump_create, \ > - .flow_dump_destroy = netdev_tc_flow_dump_destroy, \ > - .flow_dump_next = netdev_tc_flow_dump_next, \ > - .flow_put = netdev_tc_flow_put, \ > - .flow_get = netdev_tc_flow_get, \ > - .flow_del = netdev_tc_flow_del, \ > - .init_flow_api = netdev_tc_init_flow_api > - > #endif /* netdev-linux.h */ > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > new file mode 100644 > index 000000000..ceeaa50b0 > --- /dev/null > +++ b/lib/netdev-offload-provider.h > @@ -0,0 +1,99 @@ > +/* > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc. > + * Copyright (c) 2019 Samsung Electronics Co.,Ltd. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.apache.org%2Flicenses%2FLICENSE-2.0&data=02%7C01%7Croid%40mellanox.com%7C3585dc2d68ec4323504708d6d94b10cd%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636935313756402511&sdata=poE9wwzSLshxGaVKrSMP12qZSdPJsmLBCCxxkhsFrFQ%3D&reserved=0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#ifndef NETDEV_FLOW_API_PROVIDER_H > +#define NETDEV_FLOW_API_PROVIDER_H 1 > + > +#include "flow.h" > +#include "openvswitch/types.h" > +#include "packets.h" > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +struct netdev_flow_api { > + char *type; > + /* Flush all offloaded flows from a netdev. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_flush)(struct netdev *); > + > + /* Flow dumping interface. > + * > + * This is the back-end for the flow dumping interface described in > + * dpif.h. Please read the comments there first, because this code > + * closely follows it. > + * > + * On success returns 0 and allocates data, on failure returns > + * positive errno. */ > + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); > + int (*flow_dump_destroy)(struct netdev_flow_dump *); > + > + /* Returns true if there are more flows to dump. > + * 'rbuffer' is used as a temporary buffer and needs to be pre allocated > + * by the caller. While there are more flows the same 'rbuffer' > + * should be provided. 'wbuffer' is used to store dumped actions and needs > + * to be pre allocated by the caller. */ > + bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, > + struct nlattr **actions, > + struct dpif_flow_stats *stats, > + struct dpif_flow_attrs *attrs, ovs_u128 *ufid, > + struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); > + > + /* Offload the given flow on netdev. > + * To modify a flow, use the same ufid. > + * 'actions' are in netlink format, as with struct dpif_flow_put. > + * 'info' is extra info needed to offload the flow. > + * 'stats' is populated according to the rules set out in the description > + * above 'struct dpif_flow_put'. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, > + size_t actions_len, const ovs_u128 *ufid, > + struct offload_info *info, struct dpif_flow_stats *); > + > + /* Queries a flow specified by ufid on netdev. > + * Fills output buffer as 'wbuffer' in flow_dump_next, which > + * needs to be be pre allocated. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, > + const ovs_u128 *ufid, struct dpif_flow_stats *, > + struct dpif_flow_attrs *, struct ofpbuf *wbuffer); > + > + /* Delete a flow specified by ufid from netdev. > + * 'stats' is populated according to the rules set out in the description > + * above 'struct dpif_flow_del'. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_del)(struct netdev *, const ovs_u128 *ufid, > + struct dpif_flow_stats *); > + > + /* Initializies the netdev flow api. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*init_flow_api)(struct netdev *); > +}; > + > +int netdev_register_flow_api_provider(const struct netdev_flow_api *); > +int netdev_unregister_flow_api_provider(const char *type); > + > +#ifdef __linux__ > +extern const struct netdev_flow_api netdev_tc_offloads; > +#endif > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif /* NETDEV_FLOW_API_PROVIDER_H */ > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h > index fb0c27e6e..653854cbd 100644 > --- a/lib/netdev-provider.h > +++ b/lib/netdev-provider.h > @@ -23,6 +23,7 @@ > #include "netdev.h" > #include "openvswitch/list.h" > #include "ovs-numa.h" > +#include "ovs-rcu.h" > #include "packets.h" > #include "seq.h" > #include "openvswitch/shash.h" > @@ -93,6 +94,9 @@ struct netdev { > int n_rxq; > struct shash_node *node; /* Pointer to element in global map. */ > struct ovs_list saved_flags_list; /* Contains "struct netdev_saved_flags". */ > + > + /* Functions to control flow offloading. */ > + OVSRCU_TYPE(const struct netdev_flow_api *) flow_api; > struct netdev_hw_info hw_info; /* offload-capable netdev info */ > }; > > @@ -822,69 +826,6 @@ struct netdev_class { > /* Discards all packets waiting to be received from 'rx'. */ > int (*rxq_drain)(struct netdev_rxq *rx); > > - /* ## -------------------------------- ## */ > - /* ## netdev flow offloading functions ## */ > - /* ## -------------------------------- ## */ > - > - /* If a particular netdev class does not support offloading flows, > - * all these function pointers must be NULL. */ > - > - /* Flush all offloaded flows from a netdev. > - * Return 0 if successful, otherwise returns a positive errno value. */ > - int (*flow_flush)(struct netdev *); > - > - /* Flow dumping interface. > - * > - * This is the back-end for the flow dumping interface described in > - * dpif.h. Please read the comments there first, because this code > - * closely follows it. > - * > - * On success returns 0 and allocates data, on failure returns > - * positive errno. */ > - int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); > - int (*flow_dump_destroy)(struct netdev_flow_dump *); > - > - /* Returns true if there are more flows to dump. > - * 'rbuffer' is used as a temporary buffer and needs to be pre allocated > - * by the caller. While there are more flows the same 'rbuffer' > - * should be provided. 'wbuffer' is used to store dumped actions and needs > - * to be pre allocated by the caller. */ > - bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, > - struct nlattr **actions, > - struct dpif_flow_stats *stats, > - struct dpif_flow_attrs *attrs, ovs_u128 *ufid, > - struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); > - > - /* Offload the given flow on netdev. > - * To modify a flow, use the same ufid. > - * 'actions' are in netlink format, as with struct dpif_flow_put. > - * 'info' is extra info needed to offload the flow. > - * 'stats' is populated according to the rules set out in the description > - * above 'struct dpif_flow_put'. > - * Return 0 if successful, otherwise returns a positive errno value. */ > - int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, > - size_t actions_len, const ovs_u128 *ufid, > - struct offload_info *info, struct dpif_flow_stats *); > - > - /* Queries a flow specified by ufid on netdev. > - * Fills output buffer as 'wbuffer' in flow_dump_next, which > - * needs to be be pre allocated. > - * Return 0 if successful, otherwise returns a positive errno value. */ > - int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, > - const ovs_u128 *ufid, struct dpif_flow_stats *, > - struct dpif_flow_attrs *, struct ofpbuf *wbuffer); > - > - /* Delete a flow specified by ufid from netdev. > - * 'stats' is populated according to the rules set out in the description > - * above 'struct dpif_flow_del'. > - * Return 0 if successful, otherwise returns a positive errno value. */ > - int (*flow_del)(struct netdev *, const ovs_u128 *ufid, > - struct dpif_flow_stats *); > - > - /* Initializies the netdev flow api. > - * Return 0 if successful, otherwise returns a positive errno value. */ > - int (*init_flow_api)(struct netdev *); > - > /* Get a block_id from the netdev. > * Returns the block_id or 0 if none exists for netdev. */ > uint32_t (*get_block_id)(struct netdev *); > diff --git a/lib/netdev-rte-offloads.c b/lib/netdev-rte-offloads.c > index e9ab08624..683763f43 100644 > --- a/lib/netdev-rte-offloads.c > +++ b/lib/netdev-rte-offloads.c > @@ -21,6 +21,7 @@ > > #include "cmap.h" > #include "dpif-netdev.h" > +#include "netdev-offload-provider.h" > #include "netdev-provider.h" > #include "openvswitch/match.h" > #include "openvswitch/vlog.h" > @@ -29,6 +30,23 @@ > > VLOG_DEFINE_THIS_MODULE(netdev_rte_offloads); > > +/* Thread-safety > + * ============= > + * > + * Below API is NOT thread safe in following terms: > + * > + * - The caller must be sure that none of these functions will be called > + * simultaneously. Even for different 'netdev's. > + * > + * - The caller must be sure that 'netdev' will not be destructed/deallocated. > + * > + * - The caller must be sure that 'netdev' configuration will not be changed. > + * For example, simultaneous call of 'netdev_reconfigure()' for the same > + * 'netdev' is forbidden. > + * > + * For current implementation all above restrictions could be fulfilled by > + * taking the datapath 'port_mutex' in lib/dpif-netdev.c. */ > + > /* > * A mapping from ufid to dpdk rte_flow. > */ > @@ -689,7 +707,7 @@ netdev_rte_offloads_destroy_flow(struct netdev *netdev, > return ret; > } > > -int > +static int > netdev_rte_offloads_flow_put(struct netdev *netdev, struct match *match, > struct nlattr *actions, size_t actions_len, > const ovs_u128 *ufid, struct offload_info *info, > @@ -719,7 +737,7 @@ netdev_rte_offloads_flow_put(struct netdev *netdev, struct match *match, > actions_len, ufid, info); > } > > -int > +static int > netdev_rte_offloads_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > struct dpif_flow_stats *stats OVS_UNUSED) > { > @@ -731,3 +749,21 @@ netdev_rte_offloads_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > > return netdev_rte_offloads_destroy_flow(netdev, ufid, rte_flow); > } > + > +static int > +netdev_rte_offloads_init_flow_api(struct netdev *netdev) > +{ > + return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP; > +} > + > +static const struct netdev_flow_api netdev_dpdk_offloads = { > + .type = "dpdk_flow_api", > + .flow_put = netdev_rte_offloads_flow_put, > + .flow_del = netdev_rte_offloads_flow_del, > + .init_flow_api = netdev_rte_offloads_init_flow_api, > +}; > + > +void netdev_dpdk_flow_api_register(void) > +{ > + netdev_register_flow_api_provider(&netdev_dpdk_offloads); > +} > diff --git a/lib/netdev-rte-offloads.h b/lib/netdev-rte-offloads.h > index 18c8a7558..b9b292114 100644 > --- a/lib/netdev-rte-offloads.h > +++ b/lib/netdev-rte-offloads.h > @@ -14,44 +14,9 @@ > * limitations under the License. > */ > > -#ifndef NETDEV_VPORT_OFFLOADS_H > -#define NETDEV_VPORT_OFFLOADS_H 1 > +#ifndef NETDEV_DPDK_OFFLOADS_H > +#define NETDEV_DPDK_OFFLOADS_H 1 > > -#include "openvswitch/types.h" > - > -struct netdev; > -struct match; > -struct nlattr; > -struct offload_info; > -struct dpif_flow_stats; > - > -/* Thread-safety > - * ============= > - * > - * Below API is NOT thread safe in following terms: > - * > - * - The caller must be sure that none of these functions will be called > - * simultaneously. Even for different 'netdev's. > - * > - * - The caller must be sure that 'netdev' will not be destructed/deallocated. > - * > - * - The caller must be sure that 'netdev' configuration will not be changed. > - * For example, simultaneous call of 'netdev_reconfigure()' for the same > - * 'netdev' is forbidden. > - * > - * For current implementation all above restrictions could be fulfilled by > - * taking the datapath 'port_mutex' in lib/dpif-netdev.c. */ > - > -int netdev_rte_offloads_flow_put(struct netdev *netdev, struct match *match, > - struct nlattr *actions, size_t actions_len, > - const ovs_u128 *ufid, > - struct offload_info *info, > - struct dpif_flow_stats *stats); > -int netdev_rte_offloads_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > - struct dpif_flow_stats *stats); > - > -#define DPDK_FLOW_OFFLOAD_API \ > - .flow_put = netdev_rte_offloads_flow_put, \ > - .flow_del = netdev_rte_offloads_flow_del > +void netdev_dpdk_flow_api_register(void); > > #endif /* netdev-rte-offloads.h */ > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c > index d5c66acc1..b9a4a7302 100644 > --- a/lib/netdev-tc-offloads.c > +++ b/lib/netdev-tc-offloads.c > @@ -16,7 +16,6 @@ > */ > > #include <config.h> > -#include "netdev-tc-offloads.h" > > #include <errno.h> > #include <linux/if_ether.h> > @@ -31,6 +30,8 @@ > #include "openvswitch/util.h" > #include "openvswitch/vlog.h" > #include "netdev-linux.h" > +#include "netdev-offload-provider.h" > +#include "netdev-provider.h" > #include "netlink.h" > #include "netlink-socket.h" > #include "odp-netlink.h" > @@ -350,7 +351,7 @@ get_block_id_from_netdev(struct netdev *netdev) > return 0; > } > > -int > +static int > netdev_tc_flow_flush(struct netdev *netdev) > { > enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev); > @@ -368,7 +369,7 @@ netdev_tc_flow_flush(struct netdev *netdev) > return tc_flush(ifindex, block_id, hook); > } > > -int > +static int > netdev_tc_flow_dump_create(struct netdev *netdev, > struct netdev_flow_dump **dump_out) > { > @@ -395,7 +396,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev, > return 0; > } > > -int > +static int > netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump) > { > nl_dump_done(dump->nl_dump); > @@ -729,7 +730,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, > return 0; > } > > -bool > +static bool > netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, > struct match *match, > struct nlattr **actions, > @@ -1082,7 +1083,7 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, > flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; > } > > -int > +static int > netdev_tc_flow_put(struct netdev *netdev, struct match *match, > struct nlattr *actions, size_t actions_len, > const ovs_u128 *ufid, struct offload_info *info, > @@ -1375,7 +1376,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, > return err; > } > > -int > +static int > netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED, > struct match *match, > struct nlattr **actions, > @@ -1430,7 +1431,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED, > return 0; > } > > -int > +static int > netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, > const ovs_u128 *ufid, > struct dpif_flow_stats *stats) > @@ -1550,7 +1551,7 @@ probe_tc_block_support(int ifindex) > } > } > > -int > +static int > netdev_tc_init_flow_api(struct netdev *netdev) > { > static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER; > @@ -1562,8 +1563,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) > > ifindex = netdev_get_ifindex(netdev); > if (ifindex < 0) { > - VLOG_ERR_RL(&error_rl, "init: failed to get ifindex for %s: %s", > - netdev_get_name(netdev), ovs_strerror(-ifindex)); > + VLOG_INFO("init: failed to get ifindex for %s: %s", > + netdev_get_name(netdev), ovs_strerror(-ifindex)); > return -ifindex; > } > > @@ -1584,8 +1585,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) > error = tc_add_del_qdisc(ifindex, true, block_id, hook); > > if (error && error != EEXIST) { > - VLOG_ERR("failed adding ingress qdisc required for offloading: %s", > - ovs_strerror(error)); > + VLOG_INFO("failed adding ingress qdisc required for offloading: %s", > + ovs_strerror(error)); > return error; > } > > @@ -1593,3 +1594,15 @@ netdev_tc_init_flow_api(struct netdev *netdev) > > return 0; > } > + > +const struct netdev_flow_api netdev_tc_offloads = { > + .type = "linux_tc", > + .flow_flush = netdev_tc_flow_flush, > + .flow_dump_create = netdev_tc_flow_dump_create, > + .flow_dump_destroy = netdev_tc_flow_dump_destroy, > + .flow_dump_next = netdev_tc_flow_dump_next, > + .flow_put = netdev_tc_flow_put, > + .flow_get = netdev_tc_flow_get, > + .flow_del = netdev_tc_flow_del, > + .init_flow_api = netdev_tc_init_flow_api, > +}; > diff --git a/lib/netdev-tc-offloads.h b/lib/netdev-tc-offloads.h > deleted file mode 100644 > index ebd8ca884..000000000 > --- a/lib/netdev-tc-offloads.h > +++ /dev/null > @@ -1,44 +0,0 @@ > -/* > - * Copyright (c) 2016 Mellanox Technologies, Ltd. > - * > - * Licensed under the Apache License, Version 2.0 (the "License"); > - * you may not use this file except in compliance with the License. > - * You may obtain a copy of the License at: > - * > - * https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.apache.org%2Flicenses%2FLICENSE-2.0&data=02%7C01%7Croid%40mellanox.com%7C3585dc2d68ec4323504708d6d94b10cd%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636935313756402511&sdata=poE9wwzSLshxGaVKrSMP12qZSdPJsmLBCCxxkhsFrFQ%3D&reserved=0 > - * > - * Unless required by applicable law or agreed to in writing, software > - * distributed under the License is distributed on an "AS IS" BASIS, > - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > - * See the License for the specific language governing permissions and > - * limitations under the License. > - */ > - > -#ifndef NETDEV_TC_OFFLOADS_H > -#define NETDEV_TC_OFFLOADS_H 1 > - > -#include "netdev-provider.h" > - > -int netdev_tc_flow_flush(struct netdev *); > -int netdev_tc_flow_dump_create(struct netdev *, struct netdev_flow_dump **); > -int netdev_tc_flow_dump_destroy(struct netdev_flow_dump *); > -bool netdev_tc_flow_dump_next(struct netdev_flow_dump *, struct match *, > - struct nlattr **actions, > - struct dpif_flow_stats *, > - struct dpif_flow_attrs *, > - ovs_u128 *ufid, > - struct ofpbuf *rbuffer, > - struct ofpbuf *wbuffer); > -int netdev_tc_flow_put(struct netdev *, struct match *, > - struct nlattr *actions, size_t actions_len, > - const ovs_u128 *, struct offload_info *, > - struct dpif_flow_stats *); > -int netdev_tc_flow_get(struct netdev *, struct match *, > - struct nlattr **actions, const ovs_u128 *, > - struct dpif_flow_stats *, > - struct dpif_flow_attrs *, struct ofpbuf *); > -int netdev_tc_flow_del(struct netdev *, const ovs_u128 *, > - struct dpif_flow_stats *); > -int netdev_tc_init_flow_api(struct netdev *); > - > -#endif /* netdev-tc-offloads.h */ > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index ab591667f..92a256af1 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -47,7 +47,6 @@ > #include "unaligned.h" > #include "unixctl.h" > #include "openvswitch/vlog.h" > -#include "netdev-tc-offloads.h" > #ifdef __linux__ > #include "netdev-linux.h" > #endif > @@ -1116,10 +1115,8 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) > } > > #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex > -#define NETDEV_FLOW_OFFLOAD_API , LINUX_FLOW_OFFLOAD_API > #else /* !__linux__ */ > #define NETDEV_VPORT_GET_IFINDEX NULL > -#define NETDEV_FLOW_OFFLOAD_API > #endif /* __linux__ */ > > #define VPORT_FUNCTIONS_COMMON \ > @@ -1133,8 +1130,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) > .get_etheraddr = netdev_vport_get_etheraddr, \ > .get_stats = netdev_vport_get_stats, \ > .get_pt_mode = netdev_vport_get_pt_mode, \ > - .update_flags = netdev_vport_update_flags \ > - NETDEV_FLOW_OFFLOAD_API > + .update_flags = netdev_vport_update_flags > > #define TUNNEL_FUNCTIONS_COMMON \ > VPORT_FUNCTIONS_COMMON, \ > diff --git a/lib/netdev.c b/lib/netdev.c > index 7d7ecf6f0..de40f72d8 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -39,6 +39,7 @@ > #include "fatal-signal.h" > #include "hash.h" > #include "openvswitch/list.h" > +#include "netdev-offload-provider.h" > #include "netdev-provider.h" > #include "netdev-vport.h" > #include "odp-netlink.h" > @@ -98,6 +99,22 @@ struct netdev_registered_class { > > static bool netdev_flow_api_enabled = false; > > +/* Protects 'netdev_flow_apis'. */ > +static struct ovs_mutex netdev_flow_api_provider_mutex = OVS_MUTEX_INITIALIZER; > + > +/* Contains 'struct netdev_registered_flow_api's. */ > +static struct cmap netdev_flow_apis = CMAP_INITIALIZER; > + > +struct netdev_registered_flow_api { > + struct cmap_node cmap_node; /* In 'netdev_flow_apis', by flow_api->type. */ > + const struct netdev_flow_api *flow_api; > + > + /* Number of references: one for the flow_api itself and one for every > + * instance of the netdev that uses it. */ > + struct ovs_refcount refcnt; > +}; > + > + > /* This is set pretty low because we probably won't learn anything from the > * additional log messages. */ > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); > @@ -146,6 +163,8 @@ netdev_initialize(void) > netdev_register_provider(&netdev_internal_class); > netdev_register_provider(&netdev_tap_class); > netdev_vport_tunnel_register(); > + > + netdev_register_flow_api_provider(&netdev_tc_offloads); > #endif > #if defined(__FreeBSD__) || defined(__NetBSD__) > netdev_register_provider(&netdev_tap_class); > @@ -279,6 +298,87 @@ netdev_unregister_provider(const char *type) > return error; > } > > +static struct netdev_registered_flow_api * > +netdev_lookup_flow_api(const char *type) > +{ > + struct netdev_registered_flow_api *rfa; > + CMAP_FOR_EACH_WITH_HASH (rfa, cmap_node, hash_string(type, 0), > + &netdev_flow_apis) { > + if (!strcmp(type, rfa->flow_api->type)) { > + return rfa; > + } > + } > + return NULL; > +} > + > +/* Registers a new netdev flow api provider. */ > +int > +netdev_register_flow_api_provider(const struct netdev_flow_api *new_flow_api) > + OVS_EXCLUDED(netdev_flow_api_provider_mutex) > +{ > + int error = 0; > + > + if (!new_flow_api->init_flow_api) { > + VLOG_WARN("attempted to register invalid flow api provider: %s", > + new_flow_api->type); > + error = EINVAL; > + } > + > + ovs_mutex_lock(&netdev_flow_api_provider_mutex); > + if (netdev_lookup_flow_api(new_flow_api->type)) { > + VLOG_WARN("attempted to register duplicate flow api provider: %s", > + new_flow_api->type); > + error = EEXIST; > + } else { > + struct netdev_registered_flow_api *rfa; > + > + rfa = xmalloc(sizeof *rfa); > + cmap_insert(&netdev_flow_apis, &rfa->cmap_node, > + hash_string(new_flow_api->type, 0)); > + rfa->flow_api = new_flow_api; > + ovs_refcount_init(&rfa->refcnt); > + VLOG_DBG("netdev: flow API '%s' registered.", new_flow_api->type); > + } > + ovs_mutex_unlock(&netdev_flow_api_provider_mutex); > + > + return error; > +} > + > +/* Unregisters a netdev flow api provider. 'type' must have been previously > + * registered and not currently be in use by any netdevs. After unregistration > + * netdev flow api of that type cannot be used for netdevs. (However, the > + * provider may still be accessible from other threads until the next RCU grace > + * period, so the caller must not free or re-register the same netdev_flow_api > + * until that has passed.) */ > +int > +netdev_unregister_flow_api_provider(const char *type) > + OVS_EXCLUDED(netdev_flow_api_provider_mutex) > +{ > + struct netdev_registered_flow_api *rfa; > + int error; > + > + ovs_mutex_lock(&netdev_flow_api_provider_mutex); > + rfa = netdev_lookup_flow_api(type); > + if (!rfa) { > + VLOG_WARN("attempted to unregister a flow api provider that is not " > + "registered: %s", type); > + error = EAFNOSUPPORT; > + } else if (ovs_refcount_unref(&rfa->refcnt) != 1) { > + ovs_refcount_ref(&rfa->refcnt); > + VLOG_WARN("attempted to unregister in use flow api provider: %s", > + type); > + error = EBUSY; > + } else { > + cmap_remove(&netdev_flow_apis, &rfa->cmap_node, > + hash_string(rfa->flow_api->type, 0)); > + ovsrcu_postpone(free, rfa); > + error = 0; > + } > + ovs_mutex_unlock(&netdev_flow_api_provider_mutex); > + > + return error; > +} > + > /* Clears 'types' and enumerates the types of all currently registered netdev > * providers into it. The caller must first initialize the sset. */ > void > @@ -414,6 +514,7 @@ netdev_open(const char *name, const char *type, struct netdev **netdevp) > netdev->reconfigure_seq = seq_create(); > netdev->last_reconfigure_seq = > seq_read(netdev->reconfigure_seq); > + ovsrcu_set(&netdev->flow_api, NULL); > netdev->hw_info.oor = false; > netdev->node = shash_add(&netdev_shash, name, netdev); > > @@ -562,6 +663,8 @@ netdev_unref(struct netdev *dev) > ovs_assert(dev->ref_cnt); > if (!--dev->ref_cnt) { > const struct netdev_class *class = dev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &dev->flow_api); > struct netdev_registered_class *rc; > > dev->netdev_class->destruct(dev); > @@ -576,6 +679,12 @@ netdev_unref(struct netdev *dev) > > rc = netdev_lookup_class(class->type); > ovs_refcount_unref(&rc->refcnt); > + > + if (flow_api) { > + struct netdev_registered_flow_api *rfa = > + netdev_lookup_flow_api(flow_api->type); > + ovs_refcount_unref(&rfa->refcnt); > + } > } else { > ovs_mutex_unlock(&netdev_mutex); > } > @@ -2148,34 +2257,58 @@ netdev_reconfigure(struct netdev *netdev) > : EOPNOTSUPP); > } > > +static int > +netdev_assign_flow_api(struct netdev *netdev) > +{ > + struct netdev_registered_flow_api *rfa; > + > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > + if (!rfa->flow_api->init_flow_api(netdev)) { > + ovs_refcount_ref(&rfa->refcnt); > + ovsrcu_set(&netdev->flow_api, rfa->flow_api); > + VLOG_INFO("%s: Assigned flow API '%s'.", > + netdev_get_name(netdev), rfa->flow_api->type); > + return 0; > + } > + VLOG_DBG("%s: flow API '%s' is not suitable.", > + netdev_get_name(netdev), rfa->flow_api->type); > + } > + VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev)); > + > + return -1; > +} > + > int > netdev_flow_flush(struct netdev *netdev) > { > - const struct netdev_class *class = netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > > - return (class->flow_flush > - ? class->flow_flush(netdev) > - : EOPNOTSUPP); > + return (flow_api && flow_api->flow_flush) > + ? flow_api->flow_flush(netdev) > + : EOPNOTSUPP; > } > > int > netdev_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump **dump) > { > - const struct netdev_class *class = netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > > - return (class->flow_dump_create > - ? class->flow_dump_create(netdev, dump) > - : EOPNOTSUPP); > + return (flow_api && flow_api->flow_dump_create) > + ? flow_api->flow_dump_create(netdev, dump) > + : EOPNOTSUPP; > } > > int > netdev_flow_dump_destroy(struct netdev_flow_dump *dump) > { > - const struct netdev_class *class = dump->netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &dump->netdev->flow_api); > > - return (class->flow_dump_destroy > - ? class->flow_dump_destroy(dump) > - : EOPNOTSUPP); > + return (flow_api && flow_api->flow_dump_destroy) > + ? flow_api->flow_dump_destroy(dump) > + : EOPNOTSUPP; > } > > bool > @@ -2184,12 +2317,13 @@ netdev_flow_dump_next(struct netdev_flow_dump *dump, struct match *match, > struct dpif_flow_attrs *attrs, ovs_u128 *ufid, > struct ofpbuf *rbuffer, struct ofpbuf *wbuffer) > { > - const struct netdev_class *class = dump->netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &dump->netdev->flow_api); > > - return (class->flow_dump_next > - ? class->flow_dump_next(dump, match, actions, stats, attrs, > - ufid, rbuffer, wbuffer) > - : false); > + return (flow_api && flow_api->flow_dump_next) > + ? flow_api->flow_dump_next(dump, match, actions, stats, attrs, > + ufid, rbuffer, wbuffer) > + : false; > } > > int > @@ -2198,12 +2332,13 @@ netdev_flow_put(struct netdev *netdev, struct match *match, > const ovs_u128 *ufid, struct offload_info *info, > struct dpif_flow_stats *stats) > { > - const struct netdev_class *class = netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > > - return (class->flow_put > - ? class->flow_put(netdev, match, actions, act_len, ufid, > - info, stats) > - : EOPNOTSUPP); > + return (flow_api && flow_api->flow_put) > + ? flow_api->flow_put(netdev, match, actions, act_len, ufid, > + info, stats) > + : EOPNOTSUPP; > } > > int > @@ -2212,36 +2347,43 @@ netdev_flow_get(struct netdev *netdev, struct match *match, > struct dpif_flow_stats *stats, > struct dpif_flow_attrs *attrs, struct ofpbuf *buf) > { > - const struct netdev_class *class = netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > > - return (class->flow_get > - ? class->flow_get(netdev, match, actions, ufid, stats, attrs, buf) > - : EOPNOTSUPP); > + return (flow_api && flow_api->flow_get) > + ? flow_api->flow_get(netdev, match, actions, ufid, > + stats, attrs, buf) > + : EOPNOTSUPP; > } > > int > netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > struct dpif_flow_stats *stats) > { > - const struct netdev_class *class = netdev->netdev_class; > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > > - return (class->flow_del > - ? class->flow_del(netdev, ufid, stats) > - : EOPNOTSUPP); > + return (flow_api && flow_api->flow_del) > + ? flow_api->flow_del(netdev, ufid, stats) > + : EOPNOTSUPP; > } > > int > netdev_init_flow_api(struct netdev *netdev) > { > - const struct netdev_class *class = netdev->netdev_class; > - > if (!netdev_is_flow_api_enabled()) { > return EOPNOTSUPP; > } > > - return (class->init_flow_api > - ? class->init_flow_api(netdev) > - : EOPNOTSUPP); > + if (ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api)) { > + return 0; > + } > + > + if (netdev_assign_flow_api(netdev)) { > + return EOPNOTSUPP; > + } > + > + return 0; > } > > uint32_t > @@ -2573,7 +2715,6 @@ netdev_is_offload_rebalance_policy_enabled(void) > return netdev_offload_rebalance_policy; > } > > -#ifdef __linux__ > static void > netdev_ports_flow_init(void) > { > @@ -2597,8 +2738,10 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) > > VLOG_INFO("netdev: Flow API Enabled"); > > +#ifdef __linux__ > tc_set_policy(smap_get_def(ovs_other_config, "tc-policy", > TC_POLICY_DEFAULT)); > +#endif > > if (smap_get_bool(ovs_other_config, "offload-rebalance", false)) { > netdev_offload_rebalance_policy = true; > @@ -2610,9 +2753,3 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) > } > } > } > -#else > -void > -netdev_set_flow_api_enabled(const struct smap *ovs_other_config OVS_UNUSED) > -{ > -} > -#endif > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at > index 039ee971b..af8a29e44 100644 > --- a/tests/dpif-netdev.at > +++ b/tests/dpif-netdev.at > @@ -361,9 +361,9 @@ AT_CLEANUP > > m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], > [AT_SETUP([dpif-netdev - partial hw offload - $1]) > - AT_SKIP_IF([test "$IS_WIN32" = "yes" || test "$IS_BSD" = "yes"]) > OVS_VSWITCHD_START( > - [add-port br0 p1 -- set interface p1 type=$1 ofport_request=1 options:pstream=punix:$OVS_RUNDIR/p1.sock -- \ > + [add-port br0 p1 -- \ > + set interface p1 type=$1 ofport_request=1 options:pstream=punix:$OVS_RUNDIR/p1.sock options:ifindex=1 -- \ > set bridge br0 datapath-type=dummy \ > other-config:datapath-id=1234 fail-mode=secure], [], [], > [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 2f33feabc..d0101b532 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -350,7 +350,6 @@ 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: Flow API/d > /probe tc:/d > /tc: Using policy/d']]) > ]) > Acked-by: Roi Dayan <roid@mellanox.com>
> -----Original Message----- > From: Roi Dayan > Sent: Tuesday, May 21, 2019 7:48 PM > To: Ilya Maximets <i.maximets@samsung.com>; ovs-dev@openvswitch.org > Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; > Ophir Munk <ophirmu@mellanox.com>; Kevin Traynor > <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn > Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman > <simon.horman@netronome.com> > Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. > > > Acked-by: Roi Dayan <roid@mellanox.com> Hi Ilya, Can you please send a patch for the detection of netdev vport on top of this series (as you have already started suggesting in ML discussions)? I will then test it and will make sure it's applicable with this series. I think it is better to do that before series acceptance. What do you think?
On 22.05.2019 1:12, Ophir Munk wrote: > >> -----Original Message----- >> From: Roi Dayan >> Sent: Tuesday, May 21, 2019 7:48 PM >> To: Ilya Maximets <i.maximets@samsung.com>; ovs-dev@openvswitch.org >> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >> Ophir Munk <ophirmu@mellanox.com>; Kevin Traynor >> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman >> <simon.horman@netronome.com> >> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >> >> >> Acked-by: Roi Dayan <roid@mellanox.com> > > Hi Ilya, > Can you please send a patch for the detection of netdev vport on top of this series (as you have already started suggesting in ML discussions)? > I will then test it and will make sure it's applicable with this series. I think it is better to do that before series acceptance. > What do you think? Hi. Actually patches are already on a list. You only need to add few lines to make them allow vxlan for netdev-offload-dpdk. Apply following patch sets on top of this one: https://patchwork.ozlabs.org/project/openvswitch/list/?series=107534 https://patchwork.ozlabs.org/project/openvswitch/list/?series=107545 Change below should than allow you to use dpdk offloading for vxlan ports: --- diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index b7b0616ec..32f23c401 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev) return EOPNOTSUPP; } + if (!strcmp(netdev_get_name(netdev), "vxlan")) { + return 0; + } + return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP; } --- Best regards, Ilya Maximets.
On 22.05.2019 13:15, Ilya Maximets wrote: > On 22.05.2019 1:12, Ophir Munk wrote: >> >>> -----Original Message----- >>> From: Roi Dayan >>> Sent: Tuesday, May 21, 2019 7:48 PM >>> To: Ilya Maximets <i.maximets@samsung.com>; ovs-dev@openvswitch.org >>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>> Ophir Munk <ophirmu@mellanox.com>; Kevin Traynor >>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman >>> <simon.horman@netronome.com> >>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>> >>> >>> Acked-by: Roi Dayan <roid@mellanox.com> >> >> Hi Ilya, >> Can you please send a patch for the detection of netdev vport on top of this series (as you have already started suggesting in ML discussions)? >> I will then test it and will make sure it's applicable with this series. I think it is better to do that before series acceptance. >> What do you think? > > Hi. > Actually patches are already on a list. You only need to add few lines to > make them allow vxlan for netdev-offload-dpdk. > > Apply following patch sets on top of this one: > > https://patchwork.ozlabs.org/project/openvswitch/list/?series=107534 > https://patchwork.ozlabs.org/project/openvswitch/list/?series=107545 > > Change below should than allow you to use dpdk offloading for vxlan ports: > --- > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index b7b0616ec..32f23c401 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev) > return EOPNOTSUPP; > } > > + if (!strcmp(netdev_get_name(netdev), "vxlan")) { Sorry, s/netdev_get_name/netdev_get_type/ > + return 0; > + } > + > return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP; > } > > --- > > Best regards, Ilya Maximets. >
> -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: Wednesday, May 22, 2019 2:50 PM > To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan > <roid@mellanox.com>; ovs-dev@openvswitch.org > Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; > Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai > <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben Pfaff > <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga > Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd > Dibbiny <majd@mellanox.com> > Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. > > > > On 22.05.2019 13:15, Ilya Maximets wrote: > > On 22.05.2019 1:12, Ophir Munk wrote: > >> > >>> -----Original Message----- > >>> From: Roi Dayan > >>> Sent: Tuesday, May 21, 2019 7:48 PM > >>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- > dev@openvswitch.org > >>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner > >>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin > Traynor > >>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn > >>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon > Horman > >>> <simon.horman@netronome.com> > >>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. > >>> > >>> > >>> Acked-by: Roi Dayan <roid@mellanox.com> > >> > >> Hi Ilya, > >> Can you please send a patch for the detection of netdev vport on top of > this series (as you have already started suggesting in ML discussions)? > >> I will then test it and will make sure it's applicable with this series. I think it > is better to do that before series acceptance. > >> What do you think? > > > > Hi. > > Actually patches are already on a list. You only need to add few lines > > to make them allow vxlan for netdev-offload-dpdk. > > > > Apply following patch sets on top of this one: > > > > https://patchwork.ozlabs.org/project/openvswitch/list/?series=107534 FYI - applying this patch succeeded, however applying the next patch failed unless I applied patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. > > https://patchwork.ozlabs.org/project/openvswitch/list/?series=107545 > > > > > > Change below should than allow you to use dpdk offloading for vxlan ports: Why do you want to use dpdk offloading for vxlan ports? We need to use vport-netdev offloading for vxlan-netdev ports. We need to use dpdk offloading for dpdk ports. Vxlan-netdev offloading and dpdk offloading have a different implementation (unlike the system case where vxlan-system offloading and system offloading are identical). I see four required offloading APIs: 1. system 2. dpdk 3. vport under system (currently it is identical to system API) 4. New vport under netdev. The first three APIs exist. The last (vxlan-netdev) will be sent soon. I see two options for adding vxlan-netdev API. 1. Create a new dedicated vport-netdev offload class. 2. Having vport-netdev API to be identical to dpdk API but since the implementations are different we will have to know the type ("dpdk" versus "vxlan"). In pseudo code: If (type=="dpdk") { // handle dpdk offloading } If (type=="vxlan") { // handle vxlan offloading } I prefer the first option. > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > > index b7b0616ec..32f23c401 100644 > > --- a/lib/netdev-offload-dpdk.c > > +++ b/lib/netdev-offload-dpdk.c > > @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev > *netdev) > > return EOPNOTSUPP; > > } > > > > + if (!strcmp(netdev_get_name(netdev), "vxlan")) { > > Sorry, > s/netdev_get_name/netdev_get_type/ > > > + return 0; Having said all the above - we still need a way to correctly select between vport-netdev API versus vport-system API. Reading your suggestion I am still not sure we have a solution here. Say we have a system bridge and a netdev bridge both with a vxlan port. When the vxlan-netdev is checked first by the system-init API it will pass the checking and it will be added as a vxlan-system. Right? Can you please advise? > > + } > > + > > return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP; > > } > > > > --- > > > > Best regards, Ilya Maximets. > > Regards, Ophir Munk
On 23.05.2019 16:18, Ophir Munk wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: Wednesday, May 22, 2019 2:50 PM >> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >> <roid@mellanox.com>; ovs-dev@openvswitch.org >> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben Pfaff >> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >> Dibbiny <majd@mellanox.com> >> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >> >> >> >> On 22.05.2019 13:15, Ilya Maximets wrote: >>> On 22.05.2019 1:12, Ophir Munk wrote: >>>> >>>>> -----Original Message----- >>>>> From: Roi Dayan >>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >> dev@openvswitch.org >>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >> Traynor >>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >> Horman >>>>> <simon.horman@netronome.com> >>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>> >>>>> >>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>> >>>> Hi Ilya, >>>> Can you please send a patch for the detection of netdev vport on top of >> this series (as you have already started suggesting in ML discussions)? >>>> I will then test it and will make sure it's applicable with this series. I think it >> is better to do that before series acceptance. >>>> What do you think? >>> >>> Hi. >>> Actually patches are already on a list. You only need to add few lines >>> to make them allow vxlan for netdev-offload-dpdk. >>> >>> Apply following patch sets on top of this one: >>> >>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=107534 > > FYI - applying this patch succeeded, however applying the next patch failed unless I applied > patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. Yes. That is expected. > >>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=107545 >>> >>> >>> Change below should than allow you to use dpdk offloading for vxlan ports: > > Why do you want to use dpdk offloading for vxlan ports? Sorry for misunderstanding, but I thought that you're implementing vxlan offloading as part of dpdk offloading. If it'll be a separate module, it's even better. > We need to use vport-netdev offloading for vxlan-netdev ports. > We need to use dpdk offloading for dpdk ports. > Vxlan-netdev offloading and dpdk offloading have a different implementation > (unlike the system case where vxlan-system offloading and system offloading are identical). > > I see four required offloading APIs: > 1. system > 2. dpdk > 3. vport under system (currently it is identical to system API) > 4. New vport under netdev. > > The first three APIs exist. The last (vxlan-netdev) will be sent soon. > > I see two options for adding vxlan-netdev API. > 1. Create a new dedicated vport-netdev offload class. > > 2. Having vport-netdev API to be identical to dpdk API but since the implementations are different we will have to know the type ("dpdk" versus "vxlan"). > In pseudo code: > If (type=="dpdk") > { > // handle dpdk offloading > } > If (type=="vxlan") > { > // handle vxlan offloading > } > > I prefer the first option. Yes. Sure. I alse prefer the separate class if it has separate implementation anyway. So, with all above patches applied you just need to make a new file: netdev-offload-vport-dpdk.c: <...> static int netdev_offload_vport_dpdk_flow_put(...) { ... } static int netdev_offload_vport_dpdk_flow_del(...) { ... } static int netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) { if (netdev_vport_is_vport_class(netdev->netdev_class) && netdev_vport_has_system_port(netdev)) { VLOG_DBG("%s: vport has backing system interface. Skipping.", netdev_get_name(netdev)); return EOPNOTSUPP; } return strcmp(netdev_get_type(netdev), "vxlan"); } const struct netdev_flow_api netdev_offload_vport_dpdk = { .type = "dpdk_flow_api", .flow_put = netdev_offload_vport_dpdk_flow_put, .flow_del = netdev_offload_vport_dpdk_flow_del, .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, }; And add following line to lib/dpdk.c: netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); And following to lib/netdev-offload-provider.h: extern const struct netdev_flow_api netdev_offload_vport_dpdk; > >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>> index b7b0616ec..32f23c401 100644 >>> --- a/lib/netdev-offload-dpdk.c >>> +++ b/lib/netdev-offload-dpdk.c >>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev >> *netdev) >>> return EOPNOTSUPP; >>> } >>> >>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >> >> Sorry, >> s/netdev_get_name/netdev_get_type/ >> >>> + return 0; > > Having said all the above - we still need a way to correctly select between vport-netdev API versus vport-system API. > Reading your suggestion I am still not sure we have a solution here. Say we have a system bridge and a netdev bridge both with a vxlan port. > When the vxlan-netdev is checked first by the system-init API it will pass the checking and it will be added as a vxlan-system. Right? No. See the 'netdev_vport_has_system_port' function from patch "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". > Can you please advise? See the code snippet above. Best regards, Ilya Maximets.
On 23.05.2019 16:53, Ilya Maximets wrote: > On 23.05.2019 16:18, Ophir Munk wrote: >> >> >>> -----Original Message----- >>> From: Ilya Maximets <i.maximets@samsung.com> >>> Sent: Wednesday, May 22, 2019 2:50 PM >>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >>> <roid@mellanox.com>; ovs-dev@openvswitch.org >>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >>> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben Pfaff >>> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >>> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >>> Dibbiny <majd@mellanox.com> >>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>> >>> >>> >>> On 22.05.2019 13:15, Ilya Maximets wrote: >>>> On 22.05.2019 1:12, Ophir Munk wrote: >>>>> >>>>>> -----Original Message----- >>>>>> From: Roi Dayan >>>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >>> dev@openvswitch.org >>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >>> Traynor >>>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >>>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >>> Horman >>>>>> <simon.horman@netronome.com> >>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>> >>>>>> >>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>> >>>>> Hi Ilya, >>>>> Can you please send a patch for the detection of netdev vport on top of >>> this series (as you have already started suggesting in ML discussions)? >>>>> I will then test it and will make sure it's applicable with this series. I think it >>> is better to do that before series acceptance. >>>>> What do you think? >>>> >>>> Hi. >>>> Actually patches are already on a list. You only need to add few lines >>>> to make them allow vxlan for netdev-offload-dpdk. >>>> >>>> Apply following patch sets on top of this one: >>>> >>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=107534 >> >> FYI - applying this patch succeeded, however applying the next patch failed unless I applied >> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. > > Yes. That is expected. > >> >>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=107545 >>>> >>>> >>>> Change below should than allow you to use dpdk offloading for vxlan ports: >> >> Why do you want to use dpdk offloading for vxlan ports? > > Sorry for misunderstanding, but I thought that you're implementing > vxlan offloading as part of dpdk offloading. If it'll be a separate > module, it's even better. > >> We need to use vport-netdev offloading for vxlan-netdev ports. >> We need to use dpdk offloading for dpdk ports. >> Vxlan-netdev offloading and dpdk offloading have a different implementation >> (unlike the system case where vxlan-system offloading and system offloading are identical). >> >> I see four required offloading APIs: >> 1. system >> 2. dpdk >> 3. vport under system (currently it is identical to system API) >> 4. New vport under netdev. >> >> The first three APIs exist. The last (vxlan-netdev) will be sent soon. >> >> I see two options for adding vxlan-netdev API. >> 1. Create a new dedicated vport-netdev offload class. >> >> 2. Having vport-netdev API to be identical to dpdk API but since the implementations are different we will have to know the type ("dpdk" versus "vxlan"). >> In pseudo code: >> If (type=="dpdk") >> { >> // handle dpdk offloading >> } >> If (type=="vxlan") >> { >> // handle vxlan offloading >> } >> >> I prefer the first option. > > Yes. Sure. I alse prefer the separate class if it has separate implementation > anyway. > > > So, with all above patches applied you just need to make a new file: > netdev-offload-vport-dpdk.c: > > <...> > static int netdev_offload_vport_dpdk_flow_put(...) > { > ... > } > > static int netdev_offload_vport_dpdk_flow_del(...) > { > ... > } > > static int > netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) > { > if (netdev_vport_is_vport_class(netdev->netdev_class) > && netdev_vport_has_system_port(netdev)) { > VLOG_DBG("%s: vport has backing system interface. Skipping.", > netdev_get_name(netdev)); > return EOPNOTSUPP; > } > return strcmp(netdev_get_type(netdev), "vxlan"); > } > > const struct netdev_flow_api netdev_offload_vport_dpdk = { > .type = "dpdk_flow_api", s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/ > .flow_put = netdev_offload_vport_dpdk_flow_put, > .flow_del = netdev_offload_vport_dpdk_flow_del, > .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, > }; > > > And add following line to lib/dpdk.c: > > netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); > > > And following to lib/netdev-offload-provider.h: > > extern const struct netdev_flow_api netdev_offload_vport_dpdk; > >> >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>> index b7b0616ec..32f23c401 100644 >>>> --- a/lib/netdev-offload-dpdk.c >>>> +++ b/lib/netdev-offload-dpdk.c >>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev >>> *netdev) >>>> return EOPNOTSUPP; >>>> } >>>> >>>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >>> >>> Sorry, >>> s/netdev_get_name/netdev_get_type/ >>> >>>> + return 0; >> >> Having said all the above - we still need a way to correctly select between vport-netdev API versus vport-system API. >> Reading your suggestion I am still not sure we have a solution here. Say we have a system bridge and a netdev bridge both with a vxlan port. >> When the vxlan-netdev is checked first by the system-init API it will pass the checking and it will be added as a vxlan-system. Right? > > No. See the 'netdev_vport_has_system_port' function from patch > "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". > >> Can you please advise? > > See the code snippet above. > > Best regards, Ilya Maximets. > >
Hi Ophir. I'm curious, what is the current status of your testing? I'd like to apply this series to not block further development. Best regards, Ilya Maximets. On 23.05.2019 16:54, Ilya Maximets wrote: > On 23.05.2019 16:53, Ilya Maximets wrote: >> On 23.05.2019 16:18, Ophir Munk wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@samsung.com> >>>> Sent: Wednesday, May 22, 2019 2:50 PM >>>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >>>> <roid@mellanox.com>; ovs-dev@openvswitch.org >>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>>> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >>>> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben Pfaff >>>> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >>>> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >>>> Dibbiny <majd@mellanox.com> >>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>> >>>> >>>> >>>> On 22.05.2019 13:15, Ilya Maximets wrote: >>>>> On 22.05.2019 1:12, Ophir Munk wrote: >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Roi Dayan >>>>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >>>> dev@openvswitch.org >>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >>>> Traynor >>>>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >>>>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >>>> Horman >>>>>>> <simon.horman@netronome.com> >>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>> >>>>>>> >>>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>>> >>>>>> Hi Ilya, >>>>>> Can you please send a patch for the detection of netdev vport on top of >>>> this series (as you have already started suggesting in ML discussions)? >>>>>> I will then test it and will make sure it's applicable with this series. I think it >>>> is better to do that before series acceptance. >>>>>> What do you think? >>>>> >>>>> Hi. >>>>> Actually patches are already on a list. You only need to add few lines >>>>> to make them allow vxlan for netdev-offload-dpdk. >>>>> >>>>> Apply following patch sets on top of this one: >>>>> >>>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=107534 >>> >>> FYI - applying this patch succeeded, however applying the next patch failed unless I applied >>> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. >> >> Yes. That is expected. >> >>> >>>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=107545 >>>>> >>>>> >>>>> Change below should than allow you to use dpdk offloading for vxlan ports: >>> >>> Why do you want to use dpdk offloading for vxlan ports? >> >> Sorry for misunderstanding, but I thought that you're implementing >> vxlan offloading as part of dpdk offloading. If it'll be a separate >> module, it's even better. >> >>> We need to use vport-netdev offloading for vxlan-netdev ports. >>> We need to use dpdk offloading for dpdk ports. >>> Vxlan-netdev offloading and dpdk offloading have a different implementation >>> (unlike the system case where vxlan-system offloading and system offloading are identical). >>> >>> I see four required offloading APIs: >>> 1. system >>> 2. dpdk >>> 3. vport under system (currently it is identical to system API) >>> 4. New vport under netdev. >>> >>> The first three APIs exist. The last (vxlan-netdev) will be sent soon. >>> >>> I see two options for adding vxlan-netdev API. >>> 1. Create a new dedicated vport-netdev offload class. >>> >>> 2. Having vport-netdev API to be identical to dpdk API but since the implementations are different we will have to know the type ("dpdk" versus "vxlan"). >>> In pseudo code: >>> If (type=="dpdk") >>> { >>> // handle dpdk offloading >>> } >>> If (type=="vxlan") >>> { >>> // handle vxlan offloading >>> } >>> >>> I prefer the first option. >> >> Yes. Sure. I alse prefer the separate class if it has separate implementation >> anyway. >> >> >> So, with all above patches applied you just need to make a new file: >> netdev-offload-vport-dpdk.c: >> >> <...> >> static int netdev_offload_vport_dpdk_flow_put(...) >> { >> ... >> } >> >> static int netdev_offload_vport_dpdk_flow_del(...) >> { >> ... >> } >> >> static int >> netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) >> { >> if (netdev_vport_is_vport_class(netdev->netdev_class) >> && netdev_vport_has_system_port(netdev)) { >> VLOG_DBG("%s: vport has backing system interface. Skipping.", >> netdev_get_name(netdev)); >> return EOPNOTSUPP; >> } >> return strcmp(netdev_get_type(netdev), "vxlan"); >> } >> >> const struct netdev_flow_api netdev_offload_vport_dpdk = { >> .type = "dpdk_flow_api", > > s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/ > >> .flow_put = netdev_offload_vport_dpdk_flow_put, >> .flow_del = netdev_offload_vport_dpdk_flow_del, >> .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, >> }; >> >> >> And add following line to lib/dpdk.c: >> >> netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); >> >> >> And following to lib/netdev-offload-provider.h: >> >> extern const struct netdev_flow_api netdev_offload_vport_dpdk; >> >>> >>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>> index b7b0616ec..32f23c401 100644 >>>>> --- a/lib/netdev-offload-dpdk.c >>>>> +++ b/lib/netdev-offload-dpdk.c >>>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev >>>> *netdev) >>>>> return EOPNOTSUPP; >>>>> } >>>>> >>>>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >>>> >>>> Sorry, >>>> s/netdev_get_name/netdev_get_type/ >>>> >>>>> + return 0; >>> >>> Having said all the above - we still need a way to correctly select between vport-netdev API versus vport-system API. >>> Reading your suggestion I am still not sure we have a solution here. Say we have a system bridge and a netdev bridge both with a vxlan port. >>> When the vxlan-netdev is checked first by the system-init API it will pass the checking and it will be added as a vxlan-system. Right? >> >> No. See the 'netdev_vport_has_system_port' function from patch >> "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". >> >>> Can you please advise? >> >> See the code snippet above. >> >> Best regards, Ilya Maximets.
Hi Ilya, was curious myself. Mark & RSS is working (didn't test with representors yet). I've tested offload is supported on vport using !has_system_port, do you think failing in this test is enough to say that this is netdev bridge port? When I returned supported, "dpdk_put" was called as expected (after removing the disallow). One thing I've encounters is while addin a tap to the dpdk bridge. I got this: 2019-06-06T10:18:21.865Z|00055|netdev_offload_tc|INFO|probe tc: block offload is supported. 2019-06-06T10:18:21.866Z|00056|netdev_offload_tc|INFO|added ingress qdisc to veth0 2019-06-06T10:18:21.866Z|00057|netdev_offload|INFO|veth0: Assigned flow API 'linux_tc'. 2019-06-06T10:18:21.866Z|00058|bridge|INFO|bridge br-int: added interface veth0 on port 1 2019-06-06T10:18:21.867Z|00059|bridge|INFO|bridge br-phy: added interface br-phy on port 65534 Seems that we should block this as well. BR, Roni >-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: Monday, June 3, 2019 6:30 PM >To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan <roid@mellanox.com>; >ovs-dev@openvswitch.org >Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; Kevin >Traynor <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman ><simon.horman@netronome.com>; Olga Shern <olgas@mellanox.com>; Asaf >Penso <asafp@mellanox.com>; Majd Dibbiny <majd@mellanox.com>; Oz Shlomo ><ozsh@mellanox.com> >Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. > >Hi Ophir. >I'm curious, what is the current status of your testing? > >I'd like to apply this series to not block further development. > >Best regards, Ilya Maximets. > >On 23.05.2019 16:54, Ilya Maximets wrote: >> On 23.05.2019 16:53, Ilya Maximets wrote: >>> On 23.05.2019 16:18, Ophir Munk wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Ilya Maximets <i.maximets@samsung.com> >>>>> Sent: Wednesday, May 22, 2019 2:50 PM >>>>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >>>>> <roid@mellanox.com>; ovs-dev@openvswitch.org >>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>>>> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >>>>> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben Pfaff >>>>> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >>>>> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >>>>> Dibbiny <majd@mellanox.com> >>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>> >>>>> >>>>> >>>>> On 22.05.2019 13:15, Ilya Maximets wrote: >>>>>> On 22.05.2019 1:12, Ophir Munk wrote: >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Roi Dayan >>>>>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >>>>> dev@openvswitch.org >>>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >>>>> Traynor >>>>>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >>>>>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >>>>> Horman >>>>>>>> <simon.horman@netronome.com> >>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>>> >>>>>>>> >>>>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>>>> >>>>>>> Hi Ilya, >>>>>>> Can you please send a patch for the detection of netdev vport on top of >>>>> this series (as you have already started suggesting in ML discussions)? >>>>>>> I will then test it and will make sure it's applicable with this series. I think it >>>>> is better to do that before series acceptance. >>>>>>> What do you think? >>>>>> >>>>>> Hi. >>>>>> Actually patches are already on a list. You only need to add few lines >>>>>> to make them allow vxlan for netdev-offload-dpdk. >>>>>> >>>>>> Apply following patch sets on top of this one: >>>>>> >>>>>> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >.ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107534&dat >a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >p;sdata=n%2FVMz34ICfk0dvGOP77Ip4L008RRKdraRXMG%2FtFRM64%3D&re >served=0 >>>> >>>> FYI - applying this patch succeeded, however applying the next patch failed >unless I applied >>>> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. >>> >>> Yes. That is expected. >>> >>>> >>>>>> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >.ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107545&dat >a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >p;sdata=pfTmeLEuMW1DyJck3T%2BypfDI7ZlZ7K2%2FO6810tis4PQ%3D&reser >ved=0 >>>>>> >>>>>> >>>>>> Change below should than allow you to use dpdk offloading for vxlan ports: >>>> >>>> Why do you want to use dpdk offloading for vxlan ports? >>> >>> Sorry for misunderstanding, but I thought that you're implementing >>> vxlan offloading as part of dpdk offloading. If it'll be a separate >>> module, it's even better. >>> >>>> We need to use vport-netdev offloading for vxlan-netdev ports. >>>> We need to use dpdk offloading for dpdk ports. >>>> Vxlan-netdev offloading and dpdk offloading have a different implementation >>>> (unlike the system case where vxlan-system offloading and system offloading >are identical). >>>> >>>> I see four required offloading APIs: >>>> 1. system >>>> 2. dpdk >>>> 3. vport under system (currently it is identical to system API) >>>> 4. New vport under netdev. >>>> >>>> The first three APIs exist. The last (vxlan-netdev) will be sent soon. >>>> >>>> I see two options for adding vxlan-netdev API. >>>> 1. Create a new dedicated vport-netdev offload class. >>>> >>>> 2. Having vport-netdev API to be identical to dpdk API but since the >implementations are different we will have to know the type ("dpdk" versus >"vxlan"). >>>> In pseudo code: >>>> If (type=="dpdk") >>>> { >>>> // handle dpdk offloading >>>> } >>>> If (type=="vxlan") >>>> { >>>> // handle vxlan offloading >>>> } >>>> >>>> I prefer the first option. >>> >>> Yes. Sure. I alse prefer the separate class if it has separate implementation >>> anyway. >>> >>> >>> So, with all above patches applied you just need to make a new file: >>> netdev-offload-vport-dpdk.c: >>> >>> <...> >>> static int netdev_offload_vport_dpdk_flow_put(...) >>> { >>> ... >>> } >>> >>> static int netdev_offload_vport_dpdk_flow_del(...) >>> { >>> ... >>> } >>> >>> static int >>> netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) >>> { >>> if (netdev_vport_is_vport_class(netdev->netdev_class) >>> && netdev_vport_has_system_port(netdev)) { >>> VLOG_DBG("%s: vport has backing system interface. Skipping.", >>> netdev_get_name(netdev)); >>> return EOPNOTSUPP; >>> } >>> return strcmp(netdev_get_type(netdev), "vxlan"); >>> } >>> >>> const struct netdev_flow_api netdev_offload_vport_dpdk = { >>> .type = "dpdk_flow_api", >> >> s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/ >> >>> .flow_put = netdev_offload_vport_dpdk_flow_put, >>> .flow_del = netdev_offload_vport_dpdk_flow_del, >>> .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, >>> }; >>> >>> >>> And add following line to lib/dpdk.c: >>> >>> netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); >>> >>> >>> And following to lib/netdev-offload-provider.h: >>> >>> extern const struct netdev_flow_api netdev_offload_vport_dpdk; >>> >>>> >>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>>> index b7b0616ec..32f23c401 100644 >>>>>> --- a/lib/netdev-offload-dpdk.c >>>>>> +++ b/lib/netdev-offload-dpdk.c >>>>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev >>>>> *netdev) >>>>>> return EOPNOTSUPP; >>>>>> } >>>>>> >>>>>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >>>>> >>>>> Sorry, >>>>> s/netdev_get_name/netdev_get_type/ >>>>> >>>>>> + return 0; >>>> >>>> Having said all the above - we still need a way to correctly select between >vport-netdev API versus vport-system API. >>>> Reading your suggestion I am still not sure we have a solution here. Say we >have a system bridge and a netdev bridge both with a vxlan port. >>>> When the vxlan-netdev is checked first by the system-init API it will pass the >checking and it will be added as a vxlan-system. Right? >>> >>> No. See the 'netdev_vport_has_system_port' function from patch >>> "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". >>> >>>> Can you please advise? >>> >>> See the code snippet above. >>> >>> Best regards, Ilya Maximets.
On 06.06.2019 13:38, Roni Bar Yanai wrote: > Hi Ilya, > was curious myself. Mark & RSS is working (didn't test with representors yet). Hi. Thanks for testing! > I've tested offload is supported on vport using !has_system_port, do you think failing in this test is enough to say that this is netdev bridge port? I think it's OK for now. '!has_system_port' allows to say that it's not a system vport and, since 'dummy' flow API doesn't support vports offloading, we could be sure that only dpdk flow API could be used (if allowed). > When I returned supported, "dpdk_put" was called as expected (after removing the disallow). > > One thing I've encounters is while addin a tap to the dpdk bridge. I got this: > > 2019-06-06T10:18:21.865Z|00055|netdev_offload_tc|INFO|probe tc: block offload is supported. > 2019-06-06T10:18:21.866Z|00056|netdev_offload_tc|INFO|added ingress qdisc to veth0 > 2019-06-06T10:18:21.866Z|00057|netdev_offload|INFO|veth0: Assigned flow API 'linux_tc'. > 2019-06-06T10:18:21.866Z|00058|bridge|INFO|bridge br-int: added interface veth0 on port 1 > 2019-06-06T10:18:21.867Z|00059|bridge|INFO|bridge br-phy: added interface br-phy on port 65534 > > Seems that we should block this as well. I don't think that we should block this, because we should allow 'linux_tc' offloading for linux interfaces. For example, someone could open two linux ports from the userspace datapath and expect that flows could be offloaded to HW/tc layer. I agree that it will not fully work in case of mixing port types within a single OVS bridge, however this should be a valid case. Packets from linux to DPDK ports and backwards will go through OVS, because such flows could not be offloaded. Curious fact is that working this way (opening physical ports via netdev-linux) could be even faster than using DPDK ports in some cases, because 'linux_tc' supports full HW offloading unlike DPDK. We'll also have AF_XDP support some time soon which will give even more benefits to linux interfaces (XDP bypasses tc layer, however it might be possible to install flows to HW and handle unmatched packets in userspace). Best regards, Ilya Maximets. > > BR, > Roni > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: Monday, June 3, 2019 6:30 PM >> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan <roid@mellanox.com>; >> ovs-dev@openvswitch.org >> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; Kevin >> Traynor <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman >> <simon.horman@netronome.com>; Olga Shern <olgas@mellanox.com>; Asaf >> Penso <asafp@mellanox.com>; Majd Dibbiny <majd@mellanox.com>; Oz Shlomo >> <ozsh@mellanox.com> >> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >> >> Hi Ophir. >> I'm curious, what is the current status of your testing? >> >> I'd like to apply this series to not block further development. >> >> Best regards, Ilya Maximets. >> >> On 23.05.2019 16:54, Ilya Maximets wrote: >>> On 23.05.2019 16:53, Ilya Maximets wrote: >>>> On 23.05.2019 16:18, Ophir Munk wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Ilya Maximets <i.maximets@samsung.com> >>>>>> Sent: Wednesday, May 22, 2019 2:50 PM >>>>>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >>>>>> <roid@mellanox.com>; ovs-dev@openvswitch.org >>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>>>>> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >>>>>> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben Pfaff >>>>>> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >>>>>> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >>>>>> Dibbiny <majd@mellanox.com> >>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>> >>>>>> >>>>>> >>>>>> On 22.05.2019 13:15, Ilya Maximets wrote: >>>>>>> On 22.05.2019 1:12, Ophir Munk wrote: >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Roi Dayan >>>>>>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>>>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >>>>>> dev@openvswitch.org >>>>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>>>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >>>>>> Traynor >>>>>>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; Finn >>>>>>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >>>>>> Horman >>>>>>>>> <simon.horman@netronome.com> >>>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>>>> >>>>>>>>> >>>>>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>>>>> >>>>>>>> Hi Ilya, >>>>>>>> Can you please send a patch for the detection of netdev vport on top of >>>>>> this series (as you have already started suggesting in ML discussions)? >>>>>>>> I will then test it and will make sure it's applicable with this series. I think it >>>>>> is better to do that before series acceptance. >>>>>>>> What do you think? >>>>>>> >>>>>>> Hi. >>>>>>> Actually patches are already on a list. You only need to add few lines >>>>>>> to make them allow vxlan for netdev-offload-dpdk. >>>>>>> >>>>>>> Apply following patch sets on top of this one: >>>>>>> >>>>>>> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >> .ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107534&dat >> a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >> p;sdata=n%2FVMz34ICfk0dvGOP77Ip4L008RRKdraRXMG%2FtFRM64%3D&re >> served=0 >>>>> >>>>> FYI - applying this patch succeeded, however applying the next patch failed >> unless I applied >>>>> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. >>>> >>>> Yes. That is expected. >>>> >>>>> >>>>>>> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >> .ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107545&dat >> a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >> p;sdata=pfTmeLEuMW1DyJck3T%2BypfDI7ZlZ7K2%2FO6810tis4PQ%3D&reser >> ved=0 >>>>>>> >>>>>>> >>>>>>> Change below should than allow you to use dpdk offloading for vxlan ports: >>>>> >>>>> Why do you want to use dpdk offloading for vxlan ports? >>>> >>>> Sorry for misunderstanding, but I thought that you're implementing >>>> vxlan offloading as part of dpdk offloading. If it'll be a separate >>>> module, it's even better. >>>> >>>>> We need to use vport-netdev offloading for vxlan-netdev ports. >>>>> We need to use dpdk offloading for dpdk ports. >>>>> Vxlan-netdev offloading and dpdk offloading have a different implementation >>>>> (unlike the system case where vxlan-system offloading and system offloading >> are identical). >>>>> >>>>> I see four required offloading APIs: >>>>> 1. system >>>>> 2. dpdk >>>>> 3. vport under system (currently it is identical to system API) >>>>> 4. New vport under netdev. >>>>> >>>>> The first three APIs exist. The last (vxlan-netdev) will be sent soon. >>>>> >>>>> I see two options for adding vxlan-netdev API. >>>>> 1. Create a new dedicated vport-netdev offload class. >>>>> >>>>> 2. Having vport-netdev API to be identical to dpdk API but since the >> implementations are different we will have to know the type ("dpdk" versus >> "vxlan"). >>>>> In pseudo code: >>>>> If (type=="dpdk") >>>>> { >>>>> // handle dpdk offloading >>>>> } >>>>> If (type=="vxlan") >>>>> { >>>>> // handle vxlan offloading >>>>> } >>>>> >>>>> I prefer the first option. >>>> >>>> Yes. Sure. I alse prefer the separate class if it has separate implementation >>>> anyway. >>>> >>>> >>>> So, with all above patches applied you just need to make a new file: >>>> netdev-offload-vport-dpdk.c: >>>> >>>> <...> >>>> static int netdev_offload_vport_dpdk_flow_put(...) >>>> { >>>> ... >>>> } >>>> >>>> static int netdev_offload_vport_dpdk_flow_del(...) >>>> { >>>> ... >>>> } >>>> >>>> static int >>>> netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) >>>> { >>>> if (netdev_vport_is_vport_class(netdev->netdev_class) >>>> && netdev_vport_has_system_port(netdev)) { >>>> VLOG_DBG("%s: vport has backing system interface. Skipping.", >>>> netdev_get_name(netdev)); >>>> return EOPNOTSUPP; >>>> } >>>> return strcmp(netdev_get_type(netdev), "vxlan"); >>>> } >>>> >>>> const struct netdev_flow_api netdev_offload_vport_dpdk = { >>>> .type = "dpdk_flow_api", >>> >>> s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/ >>> >>>> .flow_put = netdev_offload_vport_dpdk_flow_put, >>>> .flow_del = netdev_offload_vport_dpdk_flow_del, >>>> .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, >>>> }; >>>> >>>> >>>> And add following line to lib/dpdk.c: >>>> >>>> netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); >>>> >>>> >>>> And following to lib/netdev-offload-provider.h: >>>> >>>> extern const struct netdev_flow_api netdev_offload_vport_dpdk; >>>> >>>>> >>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>>>> index b7b0616ec..32f23c401 100644 >>>>>>> --- a/lib/netdev-offload-dpdk.c >>>>>>> +++ b/lib/netdev-offload-dpdk.c >>>>>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct netdev >>>>>> *netdev) >>>>>>> return EOPNOTSUPP; >>>>>>> } >>>>>>> >>>>>>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >>>>>> >>>>>> Sorry, >>>>>> s/netdev_get_name/netdev_get_type/ >>>>>> >>>>>>> + return 0; >>>>> >>>>> Having said all the above - we still need a way to correctly select between >> vport-netdev API versus vport-system API. >>>>> Reading your suggestion I am still not sure we have a solution here. Say we >> have a system bridge and a netdev bridge both with a vxlan port. >>>>> When the vxlan-netdev is checked first by the system-init API it will pass the >> checking and it will be added as a vxlan-system. Right? >>>> >>>> No. See the 'netdev_vport_has_system_port' function from patch >>>> "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". >>>> >>>>> Can you please advise? >>>> >>>> See the code snippet above. >>>> >>>> Best regards, Ilya Maximets.
Hi Ilya, See inline >-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: Thursday, June 6, 2019 2:19 PM >To: Roni Bar Yanai <roniba@mellanox.com>; Ophir Munk ><ophirmu@mellanox.com>; Roi Dayan <roid@mellanox.com>; ovs- >dev@openvswitch.org >Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; Kevin >Traynor <ktraynor@redhat.com>; Finn Christensen <fc@napatech.com>; Ben >Pfaff <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd Dibbiny ><majd@mellanox.com>; Oz Shlomo <ozsh@mellanox.com> >Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. > >On 06.06.2019 13:38, Roni Bar Yanai wrote: >> Hi Ilya, >> was curious myself. Mark & RSS is working (didn't test with representors yet). > >Hi. Thanks for testing! > >> I've tested offload is supported on vport using !has_system_port, do you think >failing in this test is enough to say that this is netdev bridge port? > >I think it's OK for now. '!has_system_port' allows to say that it's not a >system vport and, since 'dummy' flow API doesn't support vports offloading, >we could be sure that only dpdk flow API could be used (if allowed). > >> When I returned supported, "dpdk_put" was called as expected (after removing >the disallow). >> >> One thing I've encounters is while addin a tap to the dpdk bridge. I got this: >> >> 2019-06-06T10:18:21.865Z|00055|netdev_offload_tc|INFO|probe tc: block >offload is supported. >> 2019-06-06T10:18:21.866Z|00056|netdev_offload_tc|INFO|added ingress qdisc >to veth0 >> 2019-06-06T10:18:21.866Z|00057|netdev_offload|INFO|veth0: Assigned flow >API 'linux_tc'. >> 2019-06-06T10:18:21.866Z|00058|bridge|INFO|bridge br-int: added interface >veth0 on port 1 >> 2019-06-06T10:18:21.867Z|00059|bridge|INFO|bridge br-phy: added interface >br-phy on port 65534 >> >> Seems that we should block this as well. > >I don't think that we should block this, because we should allow >'linux_tc' offloading for linux interfaces. For example, someone >could open two linux ports from the userspace datapath and expect >that flows could be offloaded to HW/tc layer. I agree that it will >not fully work in case of mixing port types within a single OVS >bridge, however this should be a valid case. Packets from linux to >DPDK ports and backwards will go through OVS, because such flows >could not be offloaded. > >Curious fact is that working this way (opening physical ports via >netdev-linux) could be even faster than using DPDK ports in some >cases, because 'linux_tc' supports full HW offloading unlike DPDK. Can you explain what exactly are the use cases? DPDK supports full offload when using port representors, it is just a matter of integrating it into OVS. > >We'll also have AF_XDP support some time soon which will give even >more benefits to linux interfaces (XDP bypasses tc layer, however >it might be possible to install flows to HW and handle unmatched >packets in userspace). > >Best regards, Ilya Maximets. > >> >> BR, >> Roni >> >>> -----Original Message----- >>> From: Ilya Maximets <i.maximets@samsung.com> >>> Sent: Monday, June 3, 2019 6:30 PM >>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan <roid@mellanox.com>; >>> ovs-dev@openvswitch.org >>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >Kevin >>> Traynor <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; >Finn >>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman >>> <simon.horman@netronome.com>; Olga Shern <olgas@mellanox.com>; Asaf >>> Penso <asafp@mellanox.com>; Majd Dibbiny <majd@mellanox.com>; Oz >Shlomo >>> <ozsh@mellanox.com> >>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>> >>> Hi Ophir. >>> I'm curious, what is the current status of your testing? >>> >>> I'd like to apply this series to not block further development. >>> >>> Best regards, Ilya Maximets. >>> >>> On 23.05.2019 16:54, Ilya Maximets wrote: >>>> On 23.05.2019 16:53, Ilya Maximets wrote: >>>>> On 23.05.2019 16:18, Ophir Munk wrote: >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Ilya Maximets <i.maximets@samsung.com> >>>>>>> Sent: Wednesday, May 22, 2019 2:50 PM >>>>>>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >>>>>>> <roid@mellanox.com>; ovs-dev@openvswitch.org >>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>>>>>> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >>>>>>> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben >Pfaff >>>>>>> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >>>>>>> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >>>>>>> Dibbiny <majd@mellanox.com> >>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 22.05.2019 13:15, Ilya Maximets wrote: >>>>>>>> On 22.05.2019 1:12, Ophir Munk wrote: >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Roi Dayan >>>>>>>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>>>>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >>>>>>> dev@openvswitch.org >>>>>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>>>>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >>>>>>> Traynor >>>>>>>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; >Finn >>>>>>>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >>>>>>> Horman >>>>>>>>>> <simon.horman@netronome.com> >>>>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>>>>>> >>>>>>>>> Hi Ilya, >>>>>>>>> Can you please send a patch for the detection of netdev vport on top >of >>>>>>> this series (as you have already started suggesting in ML discussions)? >>>>>>>>> I will then test it and will make sure it's applicable with this series. I >think it >>>>>>> is better to do that before series acceptance. >>>>>>>>> What do you think? >>>>>>>> >>>>>>>> Hi. >>>>>>>> Actually patches are already on a list. You only need to add few lines >>>>>>>> to make them allow vxlan for netdev-offload-dpdk. >>>>>>>> >>>>>>>> Apply following patch sets on top of this one: >>>>>>>> >>>>>>>> >>> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >>> >.ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107534&dat >>> >a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >>> >%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >>> >p;sdata=n%2FVMz34ICfk0dvGOP77Ip4L008RRKdraRXMG%2FtFRM64%3D&re >>> served=0 >>>>>> >>>>>> FYI - applying this patch succeeded, however applying the next patch failed >>> unless I applied >>>>>> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. >>>>> >>>>> Yes. That is expected. >>>>> >>>>>> >>>>>>>> >>> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >>> >.ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107545&dat >>> >a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >>> >%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >>> >p;sdata=pfTmeLEuMW1DyJck3T%2BypfDI7ZlZ7K2%2FO6810tis4PQ%3D&reser >>> ved=0 >>>>>>>> >>>>>>>> >>>>>>>> Change below should than allow you to use dpdk offloading for vxlan >ports: >>>>>> >>>>>> Why do you want to use dpdk offloading for vxlan ports? >>>>> >>>>> Sorry for misunderstanding, but I thought that you're implementing >>>>> vxlan offloading as part of dpdk offloading. If it'll be a separate >>>>> module, it's even better. >>>>> >>>>>> We need to use vport-netdev offloading for vxlan-netdev ports. >>>>>> We need to use dpdk offloading for dpdk ports. >>>>>> Vxlan-netdev offloading and dpdk offloading have a different >implementation >>>>>> (unlike the system case where vxlan-system offloading and system >offloading >>> are identical). >>>>>> >>>>>> I see four required offloading APIs: >>>>>> 1. system >>>>>> 2. dpdk >>>>>> 3. vport under system (currently it is identical to system API) >>>>>> 4. New vport under netdev. >>>>>> >>>>>> The first three APIs exist. The last (vxlan-netdev) will be sent soon. >>>>>> >>>>>> I see two options for adding vxlan-netdev API. >>>>>> 1. Create a new dedicated vport-netdev offload class. >>>>>> >>>>>> 2. Having vport-netdev API to be identical to dpdk API but since the >>> implementations are different we will have to know the type ("dpdk" versus >>> "vxlan"). >>>>>> In pseudo code: >>>>>> If (type=="dpdk") >>>>>> { >>>>>> // handle dpdk offloading >>>>>> } >>>>>> If (type=="vxlan") >>>>>> { >>>>>> // handle vxlan offloading >>>>>> } >>>>>> >>>>>> I prefer the first option. >>>>> >>>>> Yes. Sure. I alse prefer the separate class if it has separate implementation >>>>> anyway. >>>>> >>>>> >>>>> So, with all above patches applied you just need to make a new file: >>>>> netdev-offload-vport-dpdk.c: >>>>> >>>>> <...> >>>>> static int netdev_offload_vport_dpdk_flow_put(...) >>>>> { >>>>> ... >>>>> } >>>>> >>>>> static int netdev_offload_vport_dpdk_flow_del(...) >>>>> { >>>>> ... >>>>> } >>>>> >>>>> static int >>>>> netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) >>>>> { >>>>> if (netdev_vport_is_vport_class(netdev->netdev_class) >>>>> && netdev_vport_has_system_port(netdev)) { >>>>> VLOG_DBG("%s: vport has backing system interface. Skipping.", >>>>> netdev_get_name(netdev)); >>>>> return EOPNOTSUPP; >>>>> } >>>>> return strcmp(netdev_get_type(netdev), "vxlan"); >>>>> } >>>>> >>>>> const struct netdev_flow_api netdev_offload_vport_dpdk = { >>>>> .type = "dpdk_flow_api", >>>> >>>> s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/ >>>> >>>>> .flow_put = netdev_offload_vport_dpdk_flow_put, >>>>> .flow_del = netdev_offload_vport_dpdk_flow_del, >>>>> .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, >>>>> }; >>>>> >>>>> >>>>> And add following line to lib/dpdk.c: >>>>> >>>>> netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); >>>>> >>>>> >>>>> And following to lib/netdev-offload-provider.h: >>>>> >>>>> extern const struct netdev_flow_api netdev_offload_vport_dpdk; >>>>> >>>>>> >>>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>>>>> index b7b0616ec..32f23c401 100644 >>>>>>>> --- a/lib/netdev-offload-dpdk.c >>>>>>>> +++ b/lib/netdev-offload-dpdk.c >>>>>>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct >netdev >>>>>>> *netdev) >>>>>>>> return EOPNOTSUPP; >>>>>>>> } >>>>>>>> >>>>>>>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >>>>>>> >>>>>>> Sorry, >>>>>>> s/netdev_get_name/netdev_get_type/ >>>>>>> >>>>>>>> + return 0; >>>>>> >>>>>> Having said all the above - we still need a way to correctly select between >>> vport-netdev API versus vport-system API. >>>>>> Reading your suggestion I am still not sure we have a solution here. Say we >>> have a system bridge and a netdev bridge both with a vxlan port. >>>>>> When the vxlan-netdev is checked first by the system-init API it will pass >the >>> checking and it will be added as a vxlan-system. Right? >>>>> >>>>> No. See the 'netdev_vport_has_system_port' function from patch >>>>> "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". >>>>> >>>>>> Can you please advise? >>>>> >>>>> See the code snippet above. >>>>> >>>>> Best regards, Ilya Maximets.
On 10.06.2019 10:02, Roni Bar Yanai wrote: > Hi Ilya, > > See inline > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: Thursday, June 6, 2019 2:19 PM >> To: Roni Bar Yanai <roniba@mellanox.com>; Ophir Munk >> <ophirmu@mellanox.com>; Roi Dayan <roid@mellanox.com>; ovs- >> dev@openvswitch.org >> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; Kevin >> Traynor <ktraynor@redhat.com>; Finn Christensen <fc@napatech.com>; Ben >> Pfaff <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd Dibbiny >> <majd@mellanox.com>; Oz Shlomo <ozsh@mellanox.com> >> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >> >> On 06.06.2019 13:38, Roni Bar Yanai wrote: >>> Hi Ilya, >>> was curious myself. Mark & RSS is working (didn't test with representors yet). >> >> Hi. Thanks for testing! >> >>> I've tested offload is supported on vport using !has_system_port, do you think >> failing in this test is enough to say that this is netdev bridge port? >> >> I think it's OK for now. '!has_system_port' allows to say that it's not a >> system vport and, since 'dummy' flow API doesn't support vports offloading, >> we could be sure that only dpdk flow API could be used (if allowed). >> >>> When I returned supported, "dpdk_put" was called as expected (after removing >> the disallow). >>> >>> One thing I've encounters is while addin a tap to the dpdk bridge. I got this: >>> >>> 2019-06-06T10:18:21.865Z|00055|netdev_offload_tc|INFO|probe tc: block >> offload is supported. >>> 2019-06-06T10:18:21.866Z|00056|netdev_offload_tc|INFO|added ingress qdisc >> to veth0 >>> 2019-06-06T10:18:21.866Z|00057|netdev_offload|INFO|veth0: Assigned flow >> API 'linux_tc'. >>> 2019-06-06T10:18:21.866Z|00058|bridge|INFO|bridge br-int: added interface >> veth0 on port 1 >>> 2019-06-06T10:18:21.867Z|00059|bridge|INFO|bridge br-phy: added interface >> br-phy on port 65534 >>> >>> Seems that we should block this as well. >> >> I don't think that we should block this, because we should allow >> 'linux_tc' offloading for linux interfaces. For example, someone >> could open two linux ports from the userspace datapath and expect >> that flows could be offloaded to HW/tc layer. I agree that it will >> not fully work in case of mixing port types within a single OVS >> bridge, however this should be a valid case. Packets from linux to >> DPDK ports and backwards will go through OVS, because such flows >> could not be offloaded. >> >> Curious fact is that working this way (opening physical ports via >> netdev-linux) could be even faster than using DPDK ports in some >> cases, because 'linux_tc' supports full HW offloading unlike DPDK. > > Can you explain what exactly are the use cases? > DPDK supports full offload when using port representors, it is just a matter of integrating it into OVS. Yeah. I know. I just meant that netdev-rte-offload doesn't support full offload yet, while linux_tc supports it already. The use-case is the same: running userspace datapath with port representors, but opening them via netdev-linux (i.e. usual linux port representors). This should be possible to do that. Right now there could be no much profit from this setup, but, after netdev-afxdp integration, it should be possible to use xdp for port representors. This should give full HW offloading (via linux_tc) + good performance for unmatched traffic (via AF_XDP) for OVS. This might be interesting out-of-the-box solution for users who don't want to use DPDK for some reasons. Another use-case for linux_tc offload from the userspace datapath is the optimization of packet flows between linux interfaces. Some setups like OpenStack installations has various linux interfaces (tap, veth, eth) for the management purposes. They handle dhcp, some kind of routing, monitoring and administration. Using linux_tc offloading we could keep most of this traffic inside the kernel without need to handle it by the OVS main thread in userspace. > >> >> We'll also have AF_XDP support some time soon which will give even >> more benefits to linux interfaces (XDP bypasses tc layer, however >> it might be possible to install flows to HW and handle unmatched >> packets in userspace). >> >> Best regards, Ilya Maximets. >> >>> >>> BR, >>> Roni >>> >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@samsung.com> >>>> Sent: Monday, June 3, 2019 6:30 PM >>>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan <roid@mellanox.com>; >>>> ovs-dev@openvswitch.org >>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >> Kevin >>>> Traynor <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; >> Finn >>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon Horman >>>> <simon.horman@netronome.com>; Olga Shern <olgas@mellanox.com>; Asaf >>>> Penso <asafp@mellanox.com>; Majd Dibbiny <majd@mellanox.com>; Oz >> Shlomo >>>> <ozsh@mellanox.com> >>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>> >>>> Hi Ophir. >>>> I'm curious, what is the current status of your testing? >>>> >>>> I'd like to apply this series to not block further development. >>>> >>>> Best regards, Ilya Maximets. >>>> >>>> On 23.05.2019 16:54, Ilya Maximets wrote: >>>>> On 23.05.2019 16:53, Ilya Maximets wrote: >>>>>> On 23.05.2019 16:18, Ophir Munk wrote: >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Ilya Maximets <i.maximets@samsung.com> >>>>>>>> Sent: Wednesday, May 22, 2019 2:50 PM >>>>>>>> To: Ophir Munk <ophirmu@mellanox.com>; Roi Dayan >>>>>>>> <roid@mellanox.com>; ovs-dev@openvswitch.org >>>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>; >>>>>>>> Kevin Traynor <ktraynor@redhat.com>; Roni Bar Yanai >>>>>>>> <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ben >> Pfaff >>>>>>>> <blp@ovn.org>; Simon Horman <simon.horman@netronome.com>; Olga >>>>>>>> Shern <olgas@mellanox.com>; Asaf Penso <asafp@mellanox.com>; Majd >>>>>>>> Dibbiny <majd@mellanox.com> >>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 22.05.2019 13:15, Ilya Maximets wrote: >>>>>>>>> On 22.05.2019 1:12, Ophir Munk wrote: >>>>>>>>>> >>>>>>>>>>> -----Original Message----- >>>>>>>>>>> From: Roi Dayan >>>>>>>>>>> Sent: Tuesday, May 21, 2019 7:48 PM >>>>>>>>>>> To: Ilya Maximets <i.maximets@samsung.com>; ovs- >>>>>>>> dev@openvswitch.org >>>>>>>>>>> Cc: Ian Stokes <ian.stokes@intel.com>; Flavio Leitner >>>>>>>>>>> <fbl@sysclose.org>; Ophir Munk <ophirmu@mellanox.com>; Kevin >>>>>>>> Traynor >>>>>>>>>>> <ktraynor@redhat.com>; Roni Bar Yanai <roniba@mellanox.com>; >> Finn >>>>>>>>>>> Christensen <fc@napatech.com>; Ben Pfaff <blp@ovn.org>; Simon >>>>>>>> Horman >>>>>>>>>>> <simon.horman@netronome.com> >>>>>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>>>>>>> >>>>>>>>>> Hi Ilya, >>>>>>>>>> Can you please send a patch for the detection of netdev vport on top >> of >>>>>>>> this series (as you have already started suggesting in ML discussions)? >>>>>>>>>> I will then test it and will make sure it's applicable with this series. I >> think it >>>>>>>> is better to do that before series acceptance. >>>>>>>>>> What do you think? >>>>>>>>> >>>>>>>>> Hi. >>>>>>>>> Actually patches are already on a list. You only need to add few lines >>>>>>>>> to make them allow vxlan for netdev-offload-dpdk. >>>>>>>>> >>>>>>>>> Apply following patch sets on top of this one: >>>>>>>>> >>>>>>>>> >>>> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >>>> >> .ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107534&dat >>>> >> a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >>>> >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >>>> >> p;sdata=n%2FVMz34ICfk0dvGOP77Ip4L008RRKdraRXMG%2FtFRM64%3D&re >>>> served=0 >>>>>>> >>>>>>> FYI - applying this patch succeeded, however applying the next patch failed >>>> unless I applied >>>>>>> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch. >>>>>> >>>>>> Yes. That is expected. >>>>>> >>>>>>> >>>>>>>>> >>>> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork >>>> >> .ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107545&dat >>>> >> a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41 >>>> >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am >>>> >> p;sdata=pfTmeLEuMW1DyJck3T%2BypfDI7ZlZ7K2%2FO6810tis4PQ%3D&reser >>>> ved=0 >>>>>>>>> >>>>>>>>> >>>>>>>>> Change below should than allow you to use dpdk offloading for vxlan >> ports: >>>>>>> >>>>>>> Why do you want to use dpdk offloading for vxlan ports? >>>>>> >>>>>> Sorry for misunderstanding, but I thought that you're implementing >>>>>> vxlan offloading as part of dpdk offloading. If it'll be a separate >>>>>> module, it's even better. >>>>>> >>>>>>> We need to use vport-netdev offloading for vxlan-netdev ports. >>>>>>> We need to use dpdk offloading for dpdk ports. >>>>>>> Vxlan-netdev offloading and dpdk offloading have a different >> implementation >>>>>>> (unlike the system case where vxlan-system offloading and system >> offloading >>>> are identical). >>>>>>> >>>>>>> I see four required offloading APIs: >>>>>>> 1. system >>>>>>> 2. dpdk >>>>>>> 3. vport under system (currently it is identical to system API) >>>>>>> 4. New vport under netdev. >>>>>>> >>>>>>> The first three APIs exist. The last (vxlan-netdev) will be sent soon. >>>>>>> >>>>>>> I see two options for adding vxlan-netdev API. >>>>>>> 1. Create a new dedicated vport-netdev offload class. >>>>>>> >>>>>>> 2. Having vport-netdev API to be identical to dpdk API but since the >>>> implementations are different we will have to know the type ("dpdk" versus >>>> "vxlan"). >>>>>>> In pseudo code: >>>>>>> If (type=="dpdk") >>>>>>> { >>>>>>> // handle dpdk offloading >>>>>>> } >>>>>>> If (type=="vxlan") >>>>>>> { >>>>>>> // handle vxlan offloading >>>>>>> } >>>>>>> >>>>>>> I prefer the first option. >>>>>> >>>>>> Yes. Sure. I alse prefer the separate class if it has separate implementation >>>>>> anyway. >>>>>> >>>>>> >>>>>> So, with all above patches applied you just need to make a new file: >>>>>> netdev-offload-vport-dpdk.c: >>>>>> >>>>>> <...> >>>>>> static int netdev_offload_vport_dpdk_flow_put(...) >>>>>> { >>>>>> ... >>>>>> } >>>>>> >>>>>> static int netdev_offload_vport_dpdk_flow_del(...) >>>>>> { >>>>>> ... >>>>>> } >>>>>> >>>>>> static int >>>>>> netdev_offload_vport_dpdk_init_flow_api(struct netdev *netdev) >>>>>> { >>>>>> if (netdev_vport_is_vport_class(netdev->netdev_class) >>>>>> && netdev_vport_has_system_port(netdev)) { >>>>>> VLOG_DBG("%s: vport has backing system interface. Skipping.", >>>>>> netdev_get_name(netdev)); >>>>>> return EOPNOTSUPP; >>>>>> } >>>>>> return strcmp(netdev_get_type(netdev), "vxlan"); >>>>>> } >>>>>> >>>>>> const struct netdev_flow_api netdev_offload_vport_dpdk = { >>>>>> .type = "dpdk_flow_api", >>>>> >>>>> s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/ >>>>> >>>>>> .flow_put = netdev_offload_vport_dpdk_flow_put, >>>>>> .flow_del = netdev_offload_vport_dpdk_flow_del, >>>>>> .init_flow_api = netdev_offload_vport_dpdk_init_flow_api, >>>>>> }; >>>>>> >>>>>> >>>>>> And add following line to lib/dpdk.c: >>>>>> >>>>>> netdev_register_flow_api_provider(&netdev_offload_vport_dpdk); >>>>>> >>>>>> >>>>>> And following to lib/netdev-offload-provider.h: >>>>>> >>>>>> extern const struct netdev_flow_api netdev_offload_vport_dpdk; >>>>>> >>>>>>> >>>>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>>>>>> index b7b0616ec..32f23c401 100644 >>>>>>>>> --- a/lib/netdev-offload-dpdk.c >>>>>>>>> +++ b/lib/netdev-offload-dpdk.c >>>>>>>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct >> netdev >>>>>>>> *netdev) >>>>>>>>> return EOPNOTSUPP; >>>>>>>>> } >>>>>>>>> >>>>>>>>> + if (!strcmp(netdev_get_name(netdev), "vxlan")) { >>>>>>>> >>>>>>>> Sorry, >>>>>>>> s/netdev_get_name/netdev_get_type/ >>>>>>>> >>>>>>>>> + return 0; >>>>>>> >>>>>>> Having said all the above - we still need a way to correctly select between >>>> vport-netdev API versus vport-system API. >>>>>>> Reading your suggestion I am still not sure we have a solution here. Say we >>>> have a system bridge and a netdev bridge both with a vxlan port. >>>>>>> When the vxlan-netdev is checked first by the system-init API it will pass >> the >>>> checking and it will be added as a vxlan-system. Right? >>>>>> >>>>>> No. See the 'netdev_vport_has_system_port' function from patch >>>>>> "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.". >>>>>> >>>>>>> Can you please advise? >>>>>> >>>>>> See the code snippet above. >>>>>> >>>>>> Best regards, Ilya Maximets.
diff --git a/lib/automake.mk b/lib/automake.mk index cc5dccf39..c70fda3f8 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -137,6 +137,7 @@ lib_libopenvswitch_la_SOURCES = \ lib/namemap.c \ lib/netdev-dpdk.h \ lib/netdev-dummy.c \ + lib/netdev-offload-provider.h \ lib/netdev-provider.h \ lib/netdev-rte-offloads.h \ lib/netdev-vport.c \ @@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \ lib/netdev-linux.c \ lib/netdev-linux.h \ lib/netdev-tc-offloads.c \ - lib/netdev-tc-offloads.h \ lib/netlink-conntrack.c \ lib/netlink-conntrack.h \ lib/netlink-notifier.c \ diff --git a/lib/dpdk.c b/lib/dpdk.c index dc6171546..6c6298635 100644 --- a/lib/dpdk.c +++ b/lib/dpdk.c @@ -34,6 +34,7 @@ #include "dirs.h" #include "fatal-signal.h" #include "netdev-dpdk.h" +#include "netdev-rte-offloads.h" #include "openvswitch/dynamic-string.h" #include "openvswitch/vlog.h" #include "ovs-numa.h" @@ -442,6 +443,7 @@ dpdk_init__(const struct smap *ovs_other_config) /* Finally, register the dpdk classes */ netdev_dpdk_register(); + netdev_dpdk_flow_api_register(); return true; } diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 47153dc60..c06c9ef81 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -4204,6 +4204,27 @@ unlock: return err; } +bool +netdev_dpdk_flow_api_supported(struct netdev *netdev) +{ + struct netdev_dpdk *dev; + bool ret = false; + + if (!is_dpdk_class(netdev->netdev_class)) { + goto out; + } + + dev = netdev_dpdk_cast(netdev); + ovs_mutex_lock(&dev->mutex); + if (dev->type == DPDK_DEV_ETH) { + /* TODO: Check if we able to offload some minimal flow. */ + ret = true; + } + ovs_mutex_unlock(&dev->mutex); +out: + return ret; +} + int netdev_dpdk_rte_flow_destroy(struct netdev *netdev, struct rte_flow *rte_flow, @@ -4268,8 +4289,7 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev, .get_features = netdev_dpdk_get_features, \ .get_status = netdev_dpdk_get_status, \ .reconfigure = netdev_dpdk_reconfigure, \ - .rxq_recv = netdev_dpdk_rxq_recv, \ - DPDK_FLOW_OFFLOAD_API + .rxq_recv = netdev_dpdk_rxq_recv static const struct netdev_class dpdk_class = { .type = "dpdk", diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index 9bbb8d8d6..60631c4f0 100644 --- a/lib/netdev-dpdk.h +++ b/lib/netdev-dpdk.h @@ -34,6 +34,9 @@ struct rte_flow_action; void netdev_dpdk_register(void); void free_dpdk_buf(struct dp_packet *); + +bool netdev_dpdk_flow_api_supported(struct netdev *); + int netdev_dpdk_rte_flow_destroy(struct netdev *netdev, struct rte_flow *rte_flow, diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 3f90ffa09..18eed4cf4 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -24,6 +24,7 @@ #include "dp-packet.h" #include "dpif-netdev.h" #include "flow.h" +#include "netdev-offload-provider.h" #include "netdev-provider.h" #include "netdev-vport.h" #include "odp-util.h" @@ -1523,10 +1524,6 @@ exit: return error ? -1 : 0; } -#define DUMMY_FLOW_OFFLOAD_API \ - .flow_put = netdev_dummy_flow_put, \ - .flow_del = netdev_dummy_flow_del - #define NETDEV_DUMMY_CLASS_COMMON \ .run = netdev_dummy_run, \ .wait = netdev_dummy_wait, \ @@ -1559,8 +1556,7 @@ exit: .rxq_dealloc = netdev_dummy_rxq_dealloc, \ .rxq_recv = netdev_dummy_rxq_recv, \ .rxq_wait = netdev_dummy_rxq_wait, \ - .rxq_drain = netdev_dummy_rxq_drain, \ - DUMMY_FLOW_OFFLOAD_API + .rxq_drain = netdev_dummy_rxq_drain static const struct netdev_class dummy_class = { NETDEV_DUMMY_CLASS_COMMON, @@ -1578,6 +1574,20 @@ static const struct netdev_class dummy_pmd_class = { .is_pmd = true, .reconfigure = netdev_dummy_reconfigure }; + +static int +netdev_dummy_offloads_init_flow_api(struct netdev *netdev) +{ + return is_dummy_class(netdev->netdev_class) ? 0 : EOPNOTSUPP; +} + +static const struct netdev_flow_api netdev_dummy_offloads = { + .type = "dummy", + .flow_put = netdev_dummy_flow_put, + .flow_del = netdev_dummy_flow_del, + .init_flow_api = netdev_dummy_offloads_init_flow_api, +}; + /* Helper functions. */ @@ -2024,5 +2034,7 @@ netdev_dummy_register(enum dummy_level level) netdev_register_provider(&dummy_internal_class); netdev_register_provider(&dummy_pmd_class); + netdev_register_flow_api_provider(&netdev_dummy_offloads); + netdev_vport_tunnel_register(); } diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index f75d73fd3..e4ea94cf9 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -55,7 +55,6 @@ #include "hash.h" #include "openvswitch/hmap.h" #include "netdev-provider.h" -#include "netdev-tc-offloads.h" #include "netdev-vport.h" #include "netlink-notifier.h" #include "netlink-socket.h" @@ -3321,7 +3320,6 @@ exit: const struct netdev_class netdev_linux_class = { NETDEV_LINUX_CLASS_COMMON, - LINUX_FLOW_OFFLOAD_API, .type = "system", .construct = netdev_linux_construct, .get_stats = netdev_linux_get_stats, @@ -3341,7 +3339,6 @@ const struct netdev_class netdev_tap_class = { const struct netdev_class netdev_internal_class = { NETDEV_LINUX_CLASS_COMMON, - LINUX_FLOW_OFFLOAD_API, .type = "internal", .construct = netdev_linux_construct, .get_stats = netdev_internal_get_stats, diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h index 17ca91201..e1e30f806 100644 --- a/lib/netdev-linux.h +++ b/lib/netdev-linux.h @@ -29,14 +29,4 @@ int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag, const char *flag_name, bool enable); int linux_get_ifindex(const char *netdev_name); -#define LINUX_FLOW_OFFLOAD_API \ - .flow_flush = netdev_tc_flow_flush, \ - .flow_dump_create = netdev_tc_flow_dump_create, \ - .flow_dump_destroy = netdev_tc_flow_dump_destroy, \ - .flow_dump_next = netdev_tc_flow_dump_next, \ - .flow_put = netdev_tc_flow_put, \ - .flow_get = netdev_tc_flow_get, \ - .flow_del = netdev_tc_flow_del, \ - .init_flow_api = netdev_tc_init_flow_api - #endif /* netdev-linux.h */ diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h new file mode 100644 index 000000000..ceeaa50b0 --- /dev/null +++ b/lib/netdev-offload-provider.h @@ -0,0 +1,99 @@ +/* + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc. + * Copyright (c) 2019 Samsung Electronics Co.,Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef NETDEV_FLOW_API_PROVIDER_H +#define NETDEV_FLOW_API_PROVIDER_H 1 + +#include "flow.h" +#include "openvswitch/types.h" +#include "packets.h" + +#ifdef __cplusplus +extern "C" { +#endif + +struct netdev_flow_api { + char *type; + /* Flush all offloaded flows from a netdev. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_flush)(struct netdev *); + + /* Flow dumping interface. + * + * This is the back-end for the flow dumping interface described in + * dpif.h. Please read the comments there first, because this code + * closely follows it. + * + * On success returns 0 and allocates data, on failure returns + * positive errno. */ + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); + int (*flow_dump_destroy)(struct netdev_flow_dump *); + + /* Returns true if there are more flows to dump. + * 'rbuffer' is used as a temporary buffer and needs to be pre allocated + * by the caller. While there are more flows the same 'rbuffer' + * should be provided. 'wbuffer' is used to store dumped actions and needs + * to be pre allocated by the caller. */ + bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, + struct nlattr **actions, + struct dpif_flow_stats *stats, + struct dpif_flow_attrs *attrs, ovs_u128 *ufid, + struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); + + /* Offload the given flow on netdev. + * To modify a flow, use the same ufid. + * 'actions' are in netlink format, as with struct dpif_flow_put. + * 'info' is extra info needed to offload the flow. + * 'stats' is populated according to the rules set out in the description + * above 'struct dpif_flow_put'. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, + size_t actions_len, const ovs_u128 *ufid, + struct offload_info *info, struct dpif_flow_stats *); + + /* Queries a flow specified by ufid on netdev. + * Fills output buffer as 'wbuffer' in flow_dump_next, which + * needs to be be pre allocated. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, + const ovs_u128 *ufid, struct dpif_flow_stats *, + struct dpif_flow_attrs *, struct ofpbuf *wbuffer); + + /* Delete a flow specified by ufid from netdev. + * 'stats' is populated according to the rules set out in the description + * above 'struct dpif_flow_del'. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_del)(struct netdev *, const ovs_u128 *ufid, + struct dpif_flow_stats *); + + /* Initializies the netdev flow api. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*init_flow_api)(struct netdev *); +}; + +int netdev_register_flow_api_provider(const struct netdev_flow_api *); +int netdev_unregister_flow_api_provider(const char *type); + +#ifdef __linux__ +extern const struct netdev_flow_api netdev_tc_offloads; +#endif + +#ifdef __cplusplus +} +#endif + +#endif /* NETDEV_FLOW_API_PROVIDER_H */ diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index fb0c27e6e..653854cbd 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -23,6 +23,7 @@ #include "netdev.h" #include "openvswitch/list.h" #include "ovs-numa.h" +#include "ovs-rcu.h" #include "packets.h" #include "seq.h" #include "openvswitch/shash.h" @@ -93,6 +94,9 @@ struct netdev { int n_rxq; struct shash_node *node; /* Pointer to element in global map. */ struct ovs_list saved_flags_list; /* Contains "struct netdev_saved_flags". */ + + /* Functions to control flow offloading. */ + OVSRCU_TYPE(const struct netdev_flow_api *) flow_api; struct netdev_hw_info hw_info; /* offload-capable netdev info */ }; @@ -822,69 +826,6 @@ struct netdev_class { /* Discards all packets waiting to be received from 'rx'. */ int (*rxq_drain)(struct netdev_rxq *rx); - /* ## -------------------------------- ## */ - /* ## netdev flow offloading functions ## */ - /* ## -------------------------------- ## */ - - /* If a particular netdev class does not support offloading flows, - * all these function pointers must be NULL. */ - - /* Flush all offloaded flows from a netdev. - * Return 0 if successful, otherwise returns a positive errno value. */ - int (*flow_flush)(struct netdev *); - - /* Flow dumping interface. - * - * This is the back-end for the flow dumping interface described in - * dpif.h. Please read the comments there first, because this code - * closely follows it. - * - * On success returns 0 and allocates data, on failure returns - * positive errno. */ - int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); - int (*flow_dump_destroy)(struct netdev_flow_dump *); - - /* Returns true if there are more flows to dump. - * 'rbuffer' is used as a temporary buffer and needs to be pre allocated - * by the caller. While there are more flows the same 'rbuffer' - * should be provided. 'wbuffer' is used to store dumped actions and needs - * to be pre allocated by the caller. */ - bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, - struct nlattr **actions, - struct dpif_flow_stats *stats, - struct dpif_flow_attrs *attrs, ovs_u128 *ufid, - struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); - - /* Offload the given flow on netdev. - * To modify a flow, use the same ufid. - * 'actions' are in netlink format, as with struct dpif_flow_put. - * 'info' is extra info needed to offload the flow. - * 'stats' is populated according to the rules set out in the description - * above 'struct dpif_flow_put'. - * Return 0 if successful, otherwise returns a positive errno value. */ - int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, - size_t actions_len, const ovs_u128 *ufid, - struct offload_info *info, struct dpif_flow_stats *); - - /* Queries a flow specified by ufid on netdev. - * Fills output buffer as 'wbuffer' in flow_dump_next, which - * needs to be be pre allocated. - * Return 0 if successful, otherwise returns a positive errno value. */ - int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, - const ovs_u128 *ufid, struct dpif_flow_stats *, - struct dpif_flow_attrs *, struct ofpbuf *wbuffer); - - /* Delete a flow specified by ufid from netdev. - * 'stats' is populated according to the rules set out in the description - * above 'struct dpif_flow_del'. - * Return 0 if successful, otherwise returns a positive errno value. */ - int (*flow_del)(struct netdev *, const ovs_u128 *ufid, - struct dpif_flow_stats *); - - /* Initializies the netdev flow api. - * Return 0 if successful, otherwise returns a positive errno value. */ - int (*init_flow_api)(struct netdev *); - /* Get a block_id from the netdev. * Returns the block_id or 0 if none exists for netdev. */ uint32_t (*get_block_id)(struct netdev *); diff --git a/lib/netdev-rte-offloads.c b/lib/netdev-rte-offloads.c index e9ab08624..683763f43 100644 --- a/lib/netdev-rte-offloads.c +++ b/lib/netdev-rte-offloads.c @@ -21,6 +21,7 @@ #include "cmap.h" #include "dpif-netdev.h" +#include "netdev-offload-provider.h" #include "netdev-provider.h" #include "openvswitch/match.h" #include "openvswitch/vlog.h" @@ -29,6 +30,23 @@ VLOG_DEFINE_THIS_MODULE(netdev_rte_offloads); +/* Thread-safety + * ============= + * + * Below API is NOT thread safe in following terms: + * + * - The caller must be sure that none of these functions will be called + * simultaneously. Even for different 'netdev's. + * + * - The caller must be sure that 'netdev' will not be destructed/deallocated. + * + * - The caller must be sure that 'netdev' configuration will not be changed. + * For example, simultaneous call of 'netdev_reconfigure()' for the same + * 'netdev' is forbidden. + * + * For current implementation all above restrictions could be fulfilled by + * taking the datapath 'port_mutex' in lib/dpif-netdev.c. */ + /* * A mapping from ufid to dpdk rte_flow. */ @@ -689,7 +707,7 @@ netdev_rte_offloads_destroy_flow(struct netdev *netdev, return ret; } -int +static int netdev_rte_offloads_flow_put(struct netdev *netdev, struct match *match, struct nlattr *actions, size_t actions_len, const ovs_u128 *ufid, struct offload_info *info, @@ -719,7 +737,7 @@ netdev_rte_offloads_flow_put(struct netdev *netdev, struct match *match, actions_len, ufid, info); } -int +static int netdev_rte_offloads_flow_del(struct netdev *netdev, const ovs_u128 *ufid, struct dpif_flow_stats *stats OVS_UNUSED) { @@ -731,3 +749,21 @@ netdev_rte_offloads_flow_del(struct netdev *netdev, const ovs_u128 *ufid, return netdev_rte_offloads_destroy_flow(netdev, ufid, rte_flow); } + +static int +netdev_rte_offloads_init_flow_api(struct netdev *netdev) +{ + return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP; +} + +static const struct netdev_flow_api netdev_dpdk_offloads = { + .type = "dpdk_flow_api", + .flow_put = netdev_rte_offloads_flow_put, + .flow_del = netdev_rte_offloads_flow_del, + .init_flow_api = netdev_rte_offloads_init_flow_api, +}; + +void netdev_dpdk_flow_api_register(void) +{ + netdev_register_flow_api_provider(&netdev_dpdk_offloads); +} diff --git a/lib/netdev-rte-offloads.h b/lib/netdev-rte-offloads.h index 18c8a7558..b9b292114 100644 --- a/lib/netdev-rte-offloads.h +++ b/lib/netdev-rte-offloads.h @@ -14,44 +14,9 @@ * limitations under the License. */ -#ifndef NETDEV_VPORT_OFFLOADS_H -#define NETDEV_VPORT_OFFLOADS_H 1 +#ifndef NETDEV_DPDK_OFFLOADS_H +#define NETDEV_DPDK_OFFLOADS_H 1 -#include "openvswitch/types.h" - -struct netdev; -struct match; -struct nlattr; -struct offload_info; -struct dpif_flow_stats; - -/* Thread-safety - * ============= - * - * Below API is NOT thread safe in following terms: - * - * - The caller must be sure that none of these functions will be called - * simultaneously. Even for different 'netdev's. - * - * - The caller must be sure that 'netdev' will not be destructed/deallocated. - * - * - The caller must be sure that 'netdev' configuration will not be changed. - * For example, simultaneous call of 'netdev_reconfigure()' for the same - * 'netdev' is forbidden. - * - * For current implementation all above restrictions could be fulfilled by - * taking the datapath 'port_mutex' in lib/dpif-netdev.c. */ - -int netdev_rte_offloads_flow_put(struct netdev *netdev, struct match *match, - struct nlattr *actions, size_t actions_len, - const ovs_u128 *ufid, - struct offload_info *info, - struct dpif_flow_stats *stats); -int netdev_rte_offloads_flow_del(struct netdev *netdev, const ovs_u128 *ufid, - struct dpif_flow_stats *stats); - -#define DPDK_FLOW_OFFLOAD_API \ - .flow_put = netdev_rte_offloads_flow_put, \ - .flow_del = netdev_rte_offloads_flow_del +void netdev_dpdk_flow_api_register(void); #endif /* netdev-rte-offloads.h */ diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c index d5c66acc1..b9a4a7302 100644 --- a/lib/netdev-tc-offloads.c +++ b/lib/netdev-tc-offloads.c @@ -16,7 +16,6 @@ */ #include <config.h> -#include "netdev-tc-offloads.h" #include <errno.h> #include <linux/if_ether.h> @@ -31,6 +30,8 @@ #include "openvswitch/util.h" #include "openvswitch/vlog.h" #include "netdev-linux.h" +#include "netdev-offload-provider.h" +#include "netdev-provider.h" #include "netlink.h" #include "netlink-socket.h" #include "odp-netlink.h" @@ -350,7 +351,7 @@ get_block_id_from_netdev(struct netdev *netdev) return 0; } -int +static int netdev_tc_flow_flush(struct netdev *netdev) { enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev); @@ -368,7 +369,7 @@ netdev_tc_flow_flush(struct netdev *netdev) return tc_flush(ifindex, block_id, hook); } -int +static int netdev_tc_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump **dump_out) { @@ -395,7 +396,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev, return 0; } -int +static int netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump) { nl_dump_done(dump->nl_dump); @@ -729,7 +730,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, return 0; } -bool +static bool netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, struct match *match, struct nlattr **actions, @@ -1082,7 +1083,7 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; } -int +static int netdev_tc_flow_put(struct netdev *netdev, struct match *match, struct nlattr *actions, size_t actions_len, const ovs_u128 *ufid, struct offload_info *info, @@ -1375,7 +1376,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, return err; } -int +static int netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED, struct match *match, struct nlattr **actions, @@ -1430,7 +1431,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED, return 0; } -int +static int netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, const ovs_u128 *ufid, struct dpif_flow_stats *stats) @@ -1550,7 +1551,7 @@ probe_tc_block_support(int ifindex) } } -int +static int netdev_tc_init_flow_api(struct netdev *netdev) { static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER; @@ -1562,8 +1563,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) ifindex = netdev_get_ifindex(netdev); if (ifindex < 0) { - VLOG_ERR_RL(&error_rl, "init: failed to get ifindex for %s: %s", - netdev_get_name(netdev), ovs_strerror(-ifindex)); + VLOG_INFO("init: failed to get ifindex for %s: %s", + netdev_get_name(netdev), ovs_strerror(-ifindex)); return -ifindex; } @@ -1584,8 +1585,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) error = tc_add_del_qdisc(ifindex, true, block_id, hook); if (error && error != EEXIST) { - VLOG_ERR("failed adding ingress qdisc required for offloading: %s", - ovs_strerror(error)); + VLOG_INFO("failed adding ingress qdisc required for offloading: %s", + ovs_strerror(error)); return error; } @@ -1593,3 +1594,15 @@ netdev_tc_init_flow_api(struct netdev *netdev) return 0; } + +const struct netdev_flow_api netdev_tc_offloads = { + .type = "linux_tc", + .flow_flush = netdev_tc_flow_flush, + .flow_dump_create = netdev_tc_flow_dump_create, + .flow_dump_destroy = netdev_tc_flow_dump_destroy, + .flow_dump_next = netdev_tc_flow_dump_next, + .flow_put = netdev_tc_flow_put, + .flow_get = netdev_tc_flow_get, + .flow_del = netdev_tc_flow_del, + .init_flow_api = netdev_tc_init_flow_api, +}; diff --git a/lib/netdev-tc-offloads.h b/lib/netdev-tc-offloads.h deleted file mode 100644 index ebd8ca884..000000000 --- a/lib/netdev-tc-offloads.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright (c) 2016 Mellanox Technologies, Ltd. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at: - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef NETDEV_TC_OFFLOADS_H -#define NETDEV_TC_OFFLOADS_H 1 - -#include "netdev-provider.h" - -int netdev_tc_flow_flush(struct netdev *); -int netdev_tc_flow_dump_create(struct netdev *, struct netdev_flow_dump **); -int netdev_tc_flow_dump_destroy(struct netdev_flow_dump *); -bool netdev_tc_flow_dump_next(struct netdev_flow_dump *, struct match *, - struct nlattr **actions, - struct dpif_flow_stats *, - struct dpif_flow_attrs *, - ovs_u128 *ufid, - struct ofpbuf *rbuffer, - struct ofpbuf *wbuffer); -int netdev_tc_flow_put(struct netdev *, struct match *, - struct nlattr *actions, size_t actions_len, - const ovs_u128 *, struct offload_info *, - struct dpif_flow_stats *); -int netdev_tc_flow_get(struct netdev *, struct match *, - struct nlattr **actions, const ovs_u128 *, - struct dpif_flow_stats *, - struct dpif_flow_attrs *, struct ofpbuf *); -int netdev_tc_flow_del(struct netdev *, const ovs_u128 *, - struct dpif_flow_stats *); -int netdev_tc_init_flow_api(struct netdev *); - -#endif /* netdev-tc-offloads.h */ diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index ab591667f..92a256af1 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -47,7 +47,6 @@ #include "unaligned.h" #include "unixctl.h" #include "openvswitch/vlog.h" -#include "netdev-tc-offloads.h" #ifdef __linux__ #include "netdev-linux.h" #endif @@ -1116,10 +1115,8 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) } #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex -#define NETDEV_FLOW_OFFLOAD_API , LINUX_FLOW_OFFLOAD_API #else /* !__linux__ */ #define NETDEV_VPORT_GET_IFINDEX NULL -#define NETDEV_FLOW_OFFLOAD_API #endif /* __linux__ */ #define VPORT_FUNCTIONS_COMMON \ @@ -1133,8 +1130,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) .get_etheraddr = netdev_vport_get_etheraddr, \ .get_stats = netdev_vport_get_stats, \ .get_pt_mode = netdev_vport_get_pt_mode, \ - .update_flags = netdev_vport_update_flags \ - NETDEV_FLOW_OFFLOAD_API + .update_flags = netdev_vport_update_flags #define TUNNEL_FUNCTIONS_COMMON \ VPORT_FUNCTIONS_COMMON, \ diff --git a/lib/netdev.c b/lib/netdev.c index 7d7ecf6f0..de40f72d8 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -39,6 +39,7 @@ #include "fatal-signal.h" #include "hash.h" #include "openvswitch/list.h" +#include "netdev-offload-provider.h" #include "netdev-provider.h" #include "netdev-vport.h" #include "odp-netlink.h" @@ -98,6 +99,22 @@ struct netdev_registered_class { static bool netdev_flow_api_enabled = false; +/* Protects 'netdev_flow_apis'. */ +static struct ovs_mutex netdev_flow_api_provider_mutex = OVS_MUTEX_INITIALIZER; + +/* Contains 'struct netdev_registered_flow_api's. */ +static struct cmap netdev_flow_apis = CMAP_INITIALIZER; + +struct netdev_registered_flow_api { + struct cmap_node cmap_node; /* In 'netdev_flow_apis', by flow_api->type. */ + const struct netdev_flow_api *flow_api; + + /* Number of references: one for the flow_api itself and one for every + * instance of the netdev that uses it. */ + struct ovs_refcount refcnt; +}; + + /* This is set pretty low because we probably won't learn anything from the * additional log messages. */ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); @@ -146,6 +163,8 @@ netdev_initialize(void) netdev_register_provider(&netdev_internal_class); netdev_register_provider(&netdev_tap_class); netdev_vport_tunnel_register(); + + netdev_register_flow_api_provider(&netdev_tc_offloads); #endif #if defined(__FreeBSD__) || defined(__NetBSD__) netdev_register_provider(&netdev_tap_class); @@ -279,6 +298,87 @@ netdev_unregister_provider(const char *type) return error; } +static struct netdev_registered_flow_api * +netdev_lookup_flow_api(const char *type) +{ + struct netdev_registered_flow_api *rfa; + CMAP_FOR_EACH_WITH_HASH (rfa, cmap_node, hash_string(type, 0), + &netdev_flow_apis) { + if (!strcmp(type, rfa->flow_api->type)) { + return rfa; + } + } + return NULL; +} + +/* Registers a new netdev flow api provider. */ +int +netdev_register_flow_api_provider(const struct netdev_flow_api *new_flow_api) + OVS_EXCLUDED(netdev_flow_api_provider_mutex) +{ + int error = 0; + + if (!new_flow_api->init_flow_api) { + VLOG_WARN("attempted to register invalid flow api provider: %s", + new_flow_api->type); + error = EINVAL; + } + + ovs_mutex_lock(&netdev_flow_api_provider_mutex); + if (netdev_lookup_flow_api(new_flow_api->type)) { + VLOG_WARN("attempted to register duplicate flow api provider: %s", + new_flow_api->type); + error = EEXIST; + } else { + struct netdev_registered_flow_api *rfa; + + rfa = xmalloc(sizeof *rfa); + cmap_insert(&netdev_flow_apis, &rfa->cmap_node, + hash_string(new_flow_api->type, 0)); + rfa->flow_api = new_flow_api; + ovs_refcount_init(&rfa->refcnt); + VLOG_DBG("netdev: flow API '%s' registered.", new_flow_api->type); + } + ovs_mutex_unlock(&netdev_flow_api_provider_mutex); + + return error; +} + +/* Unregisters a netdev flow api provider. 'type' must have been previously + * registered and not currently be in use by any netdevs. After unregistration + * netdev flow api of that type cannot be used for netdevs. (However, the + * provider may still be accessible from other threads until the next RCU grace + * period, so the caller must not free or re-register the same netdev_flow_api + * until that has passed.) */ +int +netdev_unregister_flow_api_provider(const char *type) + OVS_EXCLUDED(netdev_flow_api_provider_mutex) +{ + struct netdev_registered_flow_api *rfa; + int error; + + ovs_mutex_lock(&netdev_flow_api_provider_mutex); + rfa = netdev_lookup_flow_api(type); + if (!rfa) { + VLOG_WARN("attempted to unregister a flow api provider that is not " + "registered: %s", type); + error = EAFNOSUPPORT; + } else if (ovs_refcount_unref(&rfa->refcnt) != 1) { + ovs_refcount_ref(&rfa->refcnt); + VLOG_WARN("attempted to unregister in use flow api provider: %s", + type); + error = EBUSY; + } else { + cmap_remove(&netdev_flow_apis, &rfa->cmap_node, + hash_string(rfa->flow_api->type, 0)); + ovsrcu_postpone(free, rfa); + error = 0; + } + ovs_mutex_unlock(&netdev_flow_api_provider_mutex); + + return error; +} + /* Clears 'types' and enumerates the types of all currently registered netdev * providers into it. The caller must first initialize the sset. */ void @@ -414,6 +514,7 @@ netdev_open(const char *name, const char *type, struct netdev **netdevp) netdev->reconfigure_seq = seq_create(); netdev->last_reconfigure_seq = seq_read(netdev->reconfigure_seq); + ovsrcu_set(&netdev->flow_api, NULL); netdev->hw_info.oor = false; netdev->node = shash_add(&netdev_shash, name, netdev); @@ -562,6 +663,8 @@ netdev_unref(struct netdev *dev) ovs_assert(dev->ref_cnt); if (!--dev->ref_cnt) { const struct netdev_class *class = dev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &dev->flow_api); struct netdev_registered_class *rc; dev->netdev_class->destruct(dev); @@ -576,6 +679,12 @@ netdev_unref(struct netdev *dev) rc = netdev_lookup_class(class->type); ovs_refcount_unref(&rc->refcnt); + + if (flow_api) { + struct netdev_registered_flow_api *rfa = + netdev_lookup_flow_api(flow_api->type); + ovs_refcount_unref(&rfa->refcnt); + } } else { ovs_mutex_unlock(&netdev_mutex); } @@ -2148,34 +2257,58 @@ netdev_reconfigure(struct netdev *netdev) : EOPNOTSUPP); } +static int +netdev_assign_flow_api(struct netdev *netdev) +{ + struct netdev_registered_flow_api *rfa; + + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (!rfa->flow_api->init_flow_api(netdev)) { + ovs_refcount_ref(&rfa->refcnt); + ovsrcu_set(&netdev->flow_api, rfa->flow_api); + VLOG_INFO("%s: Assigned flow API '%s'.", + netdev_get_name(netdev), rfa->flow_api->type); + return 0; + } + VLOG_DBG("%s: flow API '%s' is not suitable.", + netdev_get_name(netdev), rfa->flow_api->type); + } + VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev)); + + return -1; +} + int netdev_flow_flush(struct netdev *netdev) { - const struct netdev_class *class = netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); - return (class->flow_flush - ? class->flow_flush(netdev) - : EOPNOTSUPP); + return (flow_api && flow_api->flow_flush) + ? flow_api->flow_flush(netdev) + : EOPNOTSUPP; } int netdev_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump **dump) { - const struct netdev_class *class = netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); - return (class->flow_dump_create - ? class->flow_dump_create(netdev, dump) - : EOPNOTSUPP); + return (flow_api && flow_api->flow_dump_create) + ? flow_api->flow_dump_create(netdev, dump) + : EOPNOTSUPP; } int netdev_flow_dump_destroy(struct netdev_flow_dump *dump) { - const struct netdev_class *class = dump->netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &dump->netdev->flow_api); - return (class->flow_dump_destroy - ? class->flow_dump_destroy(dump) - : EOPNOTSUPP); + return (flow_api && flow_api->flow_dump_destroy) + ? flow_api->flow_dump_destroy(dump) + : EOPNOTSUPP; } bool @@ -2184,12 +2317,13 @@ netdev_flow_dump_next(struct netdev_flow_dump *dump, struct match *match, struct dpif_flow_attrs *attrs, ovs_u128 *ufid, struct ofpbuf *rbuffer, struct ofpbuf *wbuffer) { - const struct netdev_class *class = dump->netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &dump->netdev->flow_api); - return (class->flow_dump_next - ? class->flow_dump_next(dump, match, actions, stats, attrs, - ufid, rbuffer, wbuffer) - : false); + return (flow_api && flow_api->flow_dump_next) + ? flow_api->flow_dump_next(dump, match, actions, stats, attrs, + ufid, rbuffer, wbuffer) + : false; } int @@ -2198,12 +2332,13 @@ netdev_flow_put(struct netdev *netdev, struct match *match, const ovs_u128 *ufid, struct offload_info *info, struct dpif_flow_stats *stats) { - const struct netdev_class *class = netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); - return (class->flow_put - ? class->flow_put(netdev, match, actions, act_len, ufid, - info, stats) - : EOPNOTSUPP); + return (flow_api && flow_api->flow_put) + ? flow_api->flow_put(netdev, match, actions, act_len, ufid, + info, stats) + : EOPNOTSUPP; } int @@ -2212,36 +2347,43 @@ netdev_flow_get(struct netdev *netdev, struct match *match, struct dpif_flow_stats *stats, struct dpif_flow_attrs *attrs, struct ofpbuf *buf) { - const struct netdev_class *class = netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); - return (class->flow_get - ? class->flow_get(netdev, match, actions, ufid, stats, attrs, buf) - : EOPNOTSUPP); + return (flow_api && flow_api->flow_get) + ? flow_api->flow_get(netdev, match, actions, ufid, + stats, attrs, buf) + : EOPNOTSUPP; } int netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, struct dpif_flow_stats *stats) { - const struct netdev_class *class = netdev->netdev_class; + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); - return (class->flow_del - ? class->flow_del(netdev, ufid, stats) - : EOPNOTSUPP); + return (flow_api && flow_api->flow_del) + ? flow_api->flow_del(netdev, ufid, stats) + : EOPNOTSUPP; } int netdev_init_flow_api(struct netdev *netdev) { - const struct netdev_class *class = netdev->netdev_class; - if (!netdev_is_flow_api_enabled()) { return EOPNOTSUPP; } - return (class->init_flow_api - ? class->init_flow_api(netdev) - : EOPNOTSUPP); + if (ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api)) { + return 0; + } + + if (netdev_assign_flow_api(netdev)) { + return EOPNOTSUPP; + } + + return 0; } uint32_t @@ -2573,7 +2715,6 @@ netdev_is_offload_rebalance_policy_enabled(void) return netdev_offload_rebalance_policy; } -#ifdef __linux__ static void netdev_ports_flow_init(void) { @@ -2597,8 +2738,10 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) VLOG_INFO("netdev: Flow API Enabled"); +#ifdef __linux__ tc_set_policy(smap_get_def(ovs_other_config, "tc-policy", TC_POLICY_DEFAULT)); +#endif if (smap_get_bool(ovs_other_config, "offload-rebalance", false)) { netdev_offload_rebalance_policy = true; @@ -2610,9 +2753,3 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) } } } -#else -void -netdev_set_flow_api_enabled(const struct smap *ovs_other_config OVS_UNUSED) -{ -} -#endif diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at index 039ee971b..af8a29e44 100644 --- a/tests/dpif-netdev.at +++ b/tests/dpif-netdev.at @@ -361,9 +361,9 @@ AT_CLEANUP m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], [AT_SETUP([dpif-netdev - partial hw offload - $1]) - AT_SKIP_IF([test "$IS_WIN32" = "yes" || test "$IS_BSD" = "yes"]) OVS_VSWITCHD_START( - [add-port br0 p1 -- set interface p1 type=$1 ofport_request=1 options:pstream=punix:$OVS_RUNDIR/p1.sock -- \ + [add-port br0 p1 -- \ + set interface p1 type=$1 ofport_request=1 options:pstream=punix:$OVS_RUNDIR/p1.sock options:ifindex=1 -- \ set bridge br0 datapath-type=dummy \ other-config:datapath-id=1234 fail-mode=secure], [], [], [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 2f33feabc..d0101b532 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -350,7 +350,6 @@ 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: Flow API/d /probe tc:/d /tc: Using policy/d']]) ])
Current issues with Flow API: * OVS calls offloading functions regardless of successful flow API initialization. (ex. on init_flow_api failure) * Static initilaization of Flow API for a netdev_class forbids having different offloading types for different instances of netdev with the same netdev_class. (ex. different vports in 'system' and 'netdev' datapaths at the same time) Solution: * Move Flow API from the netdev_class to netdev instance. * Make Flow API dynamic, i.e. probe the APIs and choose the suitable one. Side effects: * Flow API providers localized as possible in their modules. * Now we have an ability to make runtime checks. For example, we could check if particular device supports features we need, like if dpdk device supports RSS+MARK action. Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/automake.mk | 2 +- lib/dpdk.c | 2 + lib/netdev-dpdk.c | 24 +++- lib/netdev-dpdk.h | 3 + lib/netdev-dummy.c | 24 +++- lib/netdev-linux.c | 3 - lib/netdev-linux.h | 10 -- lib/netdev-offload-provider.h | 99 +++++++++++++++ lib/netdev-provider.h | 67 +---------- lib/netdev-rte-offloads.c | 40 +++++- lib/netdev-rte-offloads.h | 41 +------ lib/netdev-tc-offloads.c | 39 ++++-- lib/netdev-tc-offloads.h | 44 ------- lib/netdev-vport.c | 6 +- lib/netdev.c | 221 +++++++++++++++++++++++++++------- tests/dpif-netdev.at | 4 +- tests/ofproto-macros.at | 1 - 17 files changed, 398 insertions(+), 232 deletions(-) create mode 100644 lib/netdev-offload-provider.h delete mode 100644 lib/netdev-tc-offloads.h