[ovs-dev] [PATCH ovs V3 15/25] dpif-netlink: Delete a flow from netdev

[Paul Blakey]

On 14/02/2017 17:54, Simon Horman wrote:
> On Wed, Feb 08, 2017 at 05:29:28PM +0200, Roi Dayan wrote:
>> From: Paul Blakey <paulb at mellanox.com>
>>
>> If a flow was offloaded to a netdev we delete it using netdev
>> flow api.
>>
>> Signed-off-by: Paul Blakey <paulb at mellanox.com>
>> Reviewed-by: Roi Dayan <roid at mellanox.com>
>> ---
>>  lib/dpif-netlink.c | 12 +++++++++++-
>>  lib/netdev.c       | 15 +++++++++++++++
>>  2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index a7525de..b5f5694 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -2086,7 +2086,17 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>>                         put->actions, put->actions_len, put->ufid, "PUT");
>>          return parse_flow_put(dpif, put);
>>      }
>> -    case DPIF_OP_FLOW_DEL:
>> +    case DPIF_OP_FLOW_DEL: {
>> +        struct dpif_flow_del *del = &op->u.flow_del;
>> +
>> +        if (!del->ufid) {
>> +            break;
>> +        }
>> +        dbg_print_flow(del->key, del->key_len, NULL, 0, NULL, 0,
>> +                       del->ufid, "DEL");
>> +        return netdev_ports_flow_del(DPIF_HMAP_KEY(&dpif->dpif), del->ufid,
>> +                                     del->stats);
>> +    }
>
> I'm not sure that I understand how the above implements the logic
> to delete a flow from hardware if it was offloaded. Instead it seems to:
>
> 1. Not delete flows that do not have a UID and otherwise
UFID is a criteria for offloading in put, so its the same for delete as
per dpif_flow_del, if a UFID was specified in creating a flow it must
be specified while deleting a flow.

> 2. Not delete flows that were not offloaded.
Deleting a flow that is not offloaded is done by returning false which
signifies that it wasn't offloaded, the caller then handles that case 
and calls the kernel datapath FLOW_DEL (see operate in dpif_netlink).

>
> In any case I agree with Sugesh's comment that it seems useful to use a
> hash of offloaded flows to determine which flows to delete from hardware
> (and get from hardware in patch 17).
>
>>      case DPIF_OP_FLOW_GET:
>>      case DPIF_OP_EXECUTE:
>>      default:
>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index a0206b0..c6a4582 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -2269,6 +2269,21 @@ netdev_ports_flow_dumps_create(const void *obj, int *ports)
>>      return dumps;
>>  }
>>
>> +int
>> +netdev_ports_flow_del(const void *obj, const ovs_u128 *ufid,
>> +                      struct dpif_flow_stats *stats)
>> +{
>> +    struct port_to_netdev_data *data;
>> +
>> +    HMAP_FOR_EACH(data, node, &port_to_netdev) {
>> +        if (data->obj == obj && !netdev_flow_del(data->netdev, stats, ufid)) {
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    return ENOENT;
>> +}
>> +
>>  bool netdev_flow_api_enabled = false;
>>
>>  void
>> --
>> 2.7.4
>>