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 <[email protected]>
>>> Reviewed-by: Oz Shlomo <[email protected]>
>>> ---
>>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
