On 08.12.2019 10:38, Eli Britstein wrote: > > On 12/5/2019 6:46 PM, Sriharsha Basavapatna wrote: >> On Wed, Dec 4, 2019 at 8:55 PM Eli Britstein <el...@mellanox.com> wrote: >>> >>> On 12/3/2019 5:19 PM, Sriharsha Basavapatna wrote: >>>> On Wed, Nov 20, 2019 at 9:07 PM Eli Britstein <el...@mellanox.com> 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)) { >>>>> + VLOG_DBG_RL(&error_rl, >>>>> + "Output to %s cannot be offloaded", >>>>> + netdev_get_name(outdev)); >>>>> + return -1; >>>>> + } >>>>> + >>>>> + netdev_dpdk_flow_add_count_action(actions); >>>>> + netdev_dpdk_flow_add_port_id_action(actions, outdev); >>>>> + netdev_close(outdev); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> int >>>>> netdev_dpdk_flow_actions_add(struct flow_actions *actions, >>>>> struct nlattr *nl_actions, >>>>> size_t nl_actions_len, >>>>> - struct offload_info *info OVS_UNUSED) >>>>> + struct offload_info *info) >>>>> { >>>>> struct nlattr *nla; >>>>> size_t left; >>>>> >>>>> NL_ATTR_FOR_EACH_UNSAFE (nla, left, nl_actions, nl_actions_len) { >>>>> - VLOG_DBG_RL(&error_rl, >>>>> - "Unsupported action type %d", nl_attr_type(nla)); >>>>> - return -1; >>>>> + if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT && >>>>> + left <= NLA_ALIGN(nla->nla_len)) { >>>>> + if (netdev_dpdk_flow_add_output_action(actions, nla, info )) >>>>> { >>>>> + return -1; >>>>> + } >>>> The check in netdev_dpdk_flow_add_output_action() is insufficient to >>>> decide whether the device is capable of full-offload, since it assumes >>>> that the output action is supported if netdev_dpdk type is >>>> DPDK_DEV_ETH. >>>> >>>> There are a couple of issues with this approach: >>>> >>>> 1. For a device that is not capable of full-offload, it results in 2 >>>> calls to the PMD for every flow-offload request. The first attempt for >>>> full-offload fails, followed by another call for partial-offload. >>> We cannot avoid this, as we want to do full offload, and only fallback >>> to partial if it doesn't work. >>>> 2. Even for devices that support full-offload, the offload might fail >>>> if the in-port and out-port are not in the same switch-domain. For >>>> example, if only partial-offload is configured (vhost-user ports) in >>>> the vSwitch, then the PMD should check if the in-port and out-port are >>>> on the same switch (domain-id must match) and fail the request >>>> otherwise. This check is important to alert the user of >>>> misconfigurations. If OVS doesn't do this check, all pmd drivers will >>>> have to do this check and that duplication is not desirable. >>> I don't think it's in the OVS scope to do such checks, but it should be >>> in the driver's level (PMD), the same way it is in the kernel offloading >>> using TC. >>> >> IMO, it is not out-of-scope for OVS to add these checks to make better >> offload decisions: >> - This layer of OVS (netdev-offload-dpdk) is already offload-aware and >> it is using a set of offload APIs (rte_flow) > It is aware of the offload APIs, but not on specific HW capabilities, so > in some cases > > trying to make such restrictions won't cover all use cases in best case and > result in > redundant code. > > For example, Mellanox ConnectX-5/ConnectX-6 are dual port NICs, with > embedded eswitch. In ConnectX-5 we cannot forward from pf0vf0 to pf1vf0, but > we can in ConnectX-6. > > It is not something exposed at all to the user, specifically to OVS. > > So, I think those kind of decisions that involve HW specifics should be done > in the > layer that has the HW knowledge, e.g. - the driver. > >> - Full offload may not be the preferred configuration for a customer >> (since it requires additional config like SRIOV). That is, the code >> should not make the distinction that full-offload is always preferred >> and partial-offload is only a fallback option. > At the moment, any kind of offload requires SRIOV.
I don't think that SRIOV required to use Flow Director on Intel NICs. So, partial offloading should work on Intel NICs wihtout enabling SRIOV. > BTW, to resolve this, > after full > > offload is accepted, we plan to resubmit vDPA (see > https://patchwork.ozlabs.org/cover/1178472/) that will allow using offloads > for > virtio as well. > >> - Without such checks, offload rate of partial-offload configuration >> could be potentially impacted with a large number of flows. > I don't think failing in the PMD level will be much slower than failing > in OVS level. >> - Similar improvement (checking/logging in OVS as opposed to >> individual drivers) could be made in TC kernel offloads too > In order to add such checks, we can maybe think in the future of a > capability > > querying mechanism, to query each device (at init time, maybe using > rte_flow_validate) and make decisions based on that. > > However, I think this mechanism is neither a simple one, nor should be a > prerequisite for this patch-set. > >> >>> I agree that the argument of logging is important, but this is something >>> that I think out of scope of this series. All prints in DPDK go to >>> /dev/null, and not logged into OVS' log. >>> >>> BTW, in kernel, there is a similar issue with drivers that logs using >>> extack. Those messages are not dumped to dmesg, and OVS also doesn't >>> read them to its log. Also out of scope of this series... >>> >>>> Here's how this could be addressed, by identifying whether the device >>>> is capable of full-offload or partial-offload. >>>> >>>> 1. Check if the target device (in-port) is full-offload capable. This >>>> could be done by checking if rte_flow api is supported, along with a >>>> valid switch-domain-id. We could even improve this further, similar to >>>> the kernel switchdev framework in which the device mode is >>>> configurable (legacy/eswitch modes) and available to consumers, but >>>> this might require some more DPDK framework changes. >>> I think we can enhance to check if both are ETH. However, As I wrote >>> above, I think checking other HW properties should not be in OVS scope, >>> but in driver's domain. >>>> 2. If the device is full-offload capable, and if output action is >>>> specified, check if in-port and out-port are on the same switch >>>> (switch-domain-id match mentioned above). >>>> >>>> If both 1 and 2 succeed, then the device is capable of full-offload >>>> and we can proceed with full-offload item/action setup, followed by >>>> rte_flow_create(). >>>> >>>> Otherwise, we fallback to partial-offload code path. Here, we can >>>> handle the case where a vhost-user interface is either the in-port or >>>> out-port of the flow being offloaded. Depending on whether the >>>> vhost-user interface is the in-port or out-port, if the other port >>>> associated with the flow is offload capable (DPDK_DEV_ETH), then we >>>> offload the flow to that device. This would help us build further >>>> extensions to the existing partial-offload framework by adding more >>>> actions such as VLAN and Tunnel actions (apart from MARK/RSS), except >>>> the output action that still needs to be processed in software. >>>> >>>> Thanks, >>>> -Harsha >>>> >>>>> + } else { >>>>> + VLOG_DBG_RL(&error_rl, >>>>> + "Unsupported action type %d", nl_attr_type(nla)); >>>>> + return -1; >>>>> + } >>>>> } >>>>> >>>>> add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); >>>>> -- >>>>> 2.14.5 >>>>> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> d...@openvswitch.org >>>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Celibr%40mellanox.com%7Cc11727da2a894b08f61608d779a2c470%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637111612310807084&sdata=kturtC%2BTMsvvgZ1iaBa9WqHfS60CjaJ8Oqp%2BGDvbfgY%3D&reserved=0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev