On 12/2/2019 4:25 PM, Ilya Maximets wrote:
> On 01.12.2019 9:29, Eli Britstein wrote:
>> On 11/30/2019 1:59 PM, Ilya Maximets wrote:
>>> 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.
>> Actually, there is no need to call it at all, as we get here only if we
>> found the netdev by netdev_ports_get, which means we found a device that
>> can have offloads.
> 'port_to_netdev' hash map contains all the netdevs created by the ofproto
> regardless of the flow API initialization. This is done to allow delayed
> initialization if hw offloading was enabled in runtime. 'netdev_ports_get'
> just traverses above hash map. So, the flow API could be not initialized.
I don't care if flow API is not initialized on the output netdev. I
check validity of its DPDK port and that should be enough. Please see in V2.
BTW, AFAIK, changing hw-offload is not supported on the fly and requires
OVS to be restarted.
>
>> I enhanced for error handling of netdev_dpdk_get_port_id instead of
>> introducing such new code, that I think not required.
>>
>>> 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.
>> what for? if we fail by rte_flow_create, we will handle the error. the
>> validate may be used in the future to query driver capabilities, but
>> this is something much more complex and not related to this patch-set.
> OK.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
