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 <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.
>
> 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 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
