On 24.11.2019 14:22, Eli Britstein wrote: > > On 11/22/2019 6:19 PM, Ilya Maximets wrote: >> On 20.11.2019 16:28, Eli Britstein wrote: >>> Signed-off-by: Eli Britstein <el...@mellanox.com> >>> Reviewed-by: Oz Shlomo <o...@mellanox.com> >>> --- >>> NEWS | 2 + >>> lib/netdev-offload-dpdk-flow.c | 87 >>> ++++++++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 85 insertions(+), 4 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index 88b818948..ca9c2b230 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -10,6 +10,8 @@ Post-v2.12.0 >>> if supported by libbpf. >>> * Add option to enable, disable and query TCP sequence checking in >>> conntrack. >>> + - DPDK: >>> + * Add hardware offload support for output actions. >>> >>> v2.12.0 - 03 Sep 2019 >>> --------------------- >>> diff --git a/lib/netdev-offload-dpdk-flow.c b/lib/netdev-offload-dpdk-flow.c >>> index dbbc45e99..6e7efb315 100644 >>> --- a/lib/netdev-offload-dpdk-flow.c >>> +++ b/lib/netdev-offload-dpdk-flow.c >>> @@ -274,6 +274,28 @@ ds_put_flow_action(struct ds *s, const struct >>> rte_flow_action *actions) >>> } else { >>> ds_put_cstr(s, " RSS = null\n"); >>> } >>> + } else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) { >>> + const struct rte_flow_action_count *count = actions->conf; >>> + >>> + ds_put_cstr(s, "rte flow count action:\n"); >>> + if (count) { >>> + ds_put_format(s, >>> + " Count: shared=%d, id=%d\n", >>> + count->shared, count->id); >>> + } else { >>> + ds_put_cstr(s, " Count = null\n"); >>> + } >>> + } else if (actions->type == RTE_FLOW_ACTION_TYPE_PORT_ID) { >>> + const struct rte_flow_action_port_id *port_id = actions->conf; >>> + >>> + ds_put_cstr(s, "rte flow port-id action:\n"); >>> + if (port_id) { >>> + ds_put_format(s, >>> + " Port-id: original=%d, id=%d\n", >>> + port_id->original, port_id->id); >>> + } else { >>> + ds_put_cstr(s, " Port-id = null\n"); >>> + } >>> } else { >>> ds_put_format(s, "unknown rte flow action (%d)\n", actions->type); >>> } >>> @@ -531,19 +553,76 @@ netdev_dpdk_flow_patterns_add(struct flow_patterns >>> *patterns, >>> return 0; >>> } >>> >>> +static void >>> +netdev_dpdk_flow_add_count_action(struct flow_actions *actions) >>> +{ >>> + struct rte_flow_action_count *count = xzalloc(sizeof *count); >>> + >>> + count->shared = 0; >>> + /* Each flow has a single count action, so no need of id */ >>> + count->id = 0; >>> + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_COUNT, count); >>> +} >>> + >>> +static void >>> +netdev_dpdk_flow_add_port_id_action(struct flow_actions *actions, >>> + struct netdev *outdev) >>> +{ >>> + struct rte_flow_action_port_id *port_id = xzalloc(sizeof *port_id); >>> + >>> + port_id->id = netdev_dpdk_get_port_id(outdev); >>> + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_PORT_ID, port_id); >>> +} >>> + >>> +static int >>> +netdev_dpdk_flow_add_output_action(struct flow_actions *actions, >>> + const struct nlattr *nla, >>> + struct offload_info *info) >>> +{ >>> + struct netdev *outdev; >>> + odp_port_t port; >>> + >>> + port = nl_attr_get_odp_port(nla); >>> + outdev = netdev_ports_get(port, info->dpif_class); >>> + if (outdev == NULL) { >>> + VLOG_DBG_RL(&error_rl, >>> + "Cannot find netdev for odp port %d", port); >>> + return -1; >>> + } >>> + if (!netdev_dpdk_flow_api_supported(outdev)) { >> This doesn't look correct. I mean, maybe we need to check if port is >> really the port on a same piece of hardware. Will the flow setup fail >> in this case? Will code fallback to the MARK+RSS? > > We cannot check if the port is on the same HW, and it is also wrong from > the application point of view. If the operation cannot be supported, it > is in the driver (DPDK) scope to fail it.
BTW, function netdev_dpdk_flow_api_supported() is not intended to be called in the offloading process. It's only for initialization phase. You can see the "/* TODO: Check if we able to offload some minimal flow. */" in the code and that might be destructive and unwanted for offloading process. You, probably, wanted something like this instead (not tested): diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h index 4e1c4251d..92876f3a3 100644 --- a/lib/netdev-offload-provider.h +++ b/lib/netdev-offload-provider.h @@ -90,6 +90,9 @@ struct netdev_flow_api { int netdev_register_flow_api_provider(const struct netdev_flow_api *); int netdev_unregister_flow_api_provider(const char *type); +bool netdev_flow_api_equals(const struct netdev *, + const struct netdev_flow_api *); + #ifdef __linux__ extern const struct netdev_flow_api netdev_offload_tc; #endif diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index ae01acda6..1970b1bae 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -156,6 +156,16 @@ netdev_unregister_flow_api_provider(const char *type) return error; } +bool +netdev_flow_api_equals(const struct netdev *netdev, + const struct netdev_flow_api *flow_api) +{ + const struct netdev_flow_api *netdev_flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); + + return netdev_flow_api == flow_api; +} + static int netdev_assign_flow_api(struct netdev *netdev) { --- BTW2, We, probably, need to call rte_flow_validate() somewhere. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev