[ovs-dev,07/20] netdev-offload-dpdk: Introduce flow flush callback

Introduce flow flush callback for dpdk offloaded flows.

Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
---
 lib/netdev-offload-dpdk.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

On 20.11.2019 16:28, Eli Britstein wrote:
> Introduce flow flush callback for dpdk offloaded flows.
> 
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
> ---
>  lib/netdev-offload-dpdk.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 6e1ca8a0d..64873759d 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
>      return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow);
>  }
>  
> +static int
> +netdev_offload_dpdk_flow_flush(struct netdev *netdev)
> +{
> +    struct rte_flow_error error;
> +    int ret;
> +
> +    ret = netdev_dpdk_rte_flow_flush(netdev, &error);

I don't think that it's enough to only flush flows from the device,
you need to destroy all the structures allocated for already offloaded
flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate()
for each of them.

> +    if (ret) {
> +        VLOG_ERR("%s: rte flow flush error: %u : message : %s\n",
> +                 netdev_get_name(netdev), error.type, error.message);
> +    }
> +
> +    return ret;
> +}
> +
>  static int
>  netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>  {
> @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>  
>  const struct netdev_flow_api netdev_offload_dpdk = {
>      .type = "dpdk_flow_api",
> +    .flow_flush = netdev_offload_dpdk_flow_flush,
>      .flow_put = netdev_offload_dpdk_flow_put,
>      .flow_del = netdev_offload_dpdk_flow_del,
>      .init_flow_api = netdev_offload_dpdk_init_flow_api,
>

On 11/22/2019 6:06 PM, Ilya Maximets wrote:
> On 20.11.2019 16:28, Eli Britstein wrote:
>> Introduce flow flush callback for dpdk offloaded flows.
>>
>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
>> ---
>>   lib/netdev-offload-dpdk.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> index 6e1ca8a0d..64873759d 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
>>       return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow);
>>   }
>>   
>> +static int
>> +netdev_offload_dpdk_flow_flush(struct netdev *netdev)
>> +{
>> +    struct rte_flow_error error;
>> +    int ret;
>> +
>> +    ret = netdev_dpdk_rte_flow_flush(netdev, &error);
> I don't think that it's enough to only flush flows from the device,
> you need to destroy all the structures allocated for already offloaded
> flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate()
> for each of them.

Well, I admit I haven't followed all the code of it. I took 
netdev-offload-tc.c flush and adapted. It also didn't remove mappings. 
Anyway, if you think the TC code is also bad, I'll drop those 2 commits 
from this series for now

>
>> +    if (ret) {
>> +        VLOG_ERR("%s: rte flow flush error: %u : message : %s\n",
>> +                 netdev_get_name(netdev), error.type, error.message);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static int
>>   netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>>   {
>> @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>>   
>>   const struct netdev_flow_api netdev_offload_dpdk = {
>>       .type = "dpdk_flow_api",
>> +    .flow_flush = netdev_offload_dpdk_flow_flush,
>>       .flow_put = netdev_offload_dpdk_flow_put,
>>       .flow_del = netdev_offload_dpdk_flow_del,
>>       .init_flow_api = netdev_offload_dpdk_init_flow_api,
>>

On 24.11.2019 14:22, Eli Britstein wrote:
> 
> On 11/22/2019 6:06 PM, Ilya Maximets wrote:
>> On 20.11.2019 16:28, Eli Britstein wrote:
>>> Introduce flow flush callback for dpdk offloaded flows.
>>>
>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>>> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
>>> ---
>>>   lib/netdev-offload-dpdk.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>> index 6e1ca8a0d..64873759d 100644
>>> --- a/lib/netdev-offload-dpdk.c
>>> +++ b/lib/netdev-offload-dpdk.c
>>> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
>>>       return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow);
>>>   }
>>>   
>>> +static int
>>> +netdev_offload_dpdk_flow_flush(struct netdev *netdev)
>>> +{
>>> +    struct rte_flow_error error;
>>> +    int ret;
>>> +
>>> +    ret = netdev_dpdk_rte_flow_flush(netdev, &error);
>> I don't think that it's enough to only flush flows from the device,
>> you need to destroy all the structures allocated for already offloaded
>> flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate()
>> for each of them.
> 
> Well, I admit I haven't followed all the code of it. I took 
> netdev-offload-tc.c flush and adapted. It also didn't remove mappings. 
> Anyway, if you think the TC code is also bad, I'll drop those 2 commits 
> from this series for now


Yes, netdev-offload-tc code is not fully correct.  You may see that Roi
is trying to fix it:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-December/365443.html

So, please, implement it correctly or drop.  Incorrect implementation is
not a good thing to have.

Best regards, Ilya Maximets.