[ovs-dev] [PATCH] dpctl: Fix broken flow deletion via ovs-dpctl due to missing ufid.

Ilya Maximets i.maximets at ovn.org
Mon Oct 5 14:12:32 UTC 2020


On 10/5/20 2:04 PM, Eelco Chaudron wrote:
> 
> 
> On 5 Oct 2020, at 12:09, Ilya Maximets wrote:
> 
>> Current code generates UFID for flows installed by ovs-dpctl.  This
>> leads to inability to remove such flows by the same command.  Ex:
>>
>>   ovs-dpctl add-dp test
>>   ovs-dpctl add-if test vport0
>>   ovs-dpctl add-flow test "in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.0.1)" 0
>>   ovs-dpctl del-flow test "in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.0.1)"
>>
>>   dpif|WARN|system at test: failed to flow_del (No such file or directory)
>>       ufid:e4457189-3990-4a01-bdcf-1e5f8b208711 in_port(0),
>>       eth(src=00:00:00:00:00:00,dst=00:00:00:00:00:00),eth_type(0x0800),
>>       ipv4(src=100.1.0.1,dst=0.0.0.0,proto=0,tos=0,ttl=0,frag=no)
>>
>>   ovs-dpctl: deleting flow (No such file or directory)
>>   Perhaps you need to specify a UFID?
>>
>> During del-flow operation UFID is generated too, however resulted
>> value is different from one generated during add-flow.  This happens
>> because odp_flow_key_hash() function uses random base value for flow
>> hashes which is different on every invocation.  That is not an issue
>> while running 'ovs-appctl dpctl/{add,del}-flow' because execution
>> of these requests happens in context of the OVS main process, i.e.
>> there will be same random seed.
>>
>> Commit e61984e781e6 was intended to allow offloading for flows
>> added by dpctl/add-flow unixctl command, so it's better to generate
>> UFIDs conditionally inside dpctl command handler only for appctl
>> invocations.  Offloading is not possible from ovs-dpctl utility anyway.
>>
>> There are still couple of corner case:  It will not be possible to
>> remove flow by 'ovs-appctl dpctl/del-flow' without specifying UFID if
>> main OVS process was restarted since flow addition and it will not
>> be possible to remove flow by ovs-dpctl without specifying UUID if
>> it was added by 'ovs-appctl dpctl/add-flow'.  But these scenarios
>> seems minor since these commands intended for testing only.
>>
>> Reported-by: Eelco Chaudron <echaudro at redhat.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374863.html
>> Fixes: e61984e781e6 ("dpif-netlink: Generate ufids for installing TC flowers")
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
> 
> This change looks good to me, with one small comment below...
> Also, verified this in my scenario, and all works as expected.
> 
> Acked-by: Eelco Chaudron <echaudro at redhat.com>
> Tested-by: Eelco Chaudron <echaudro at redhat.com>
> 
>>
>> Eelco, Tonghao, please test this with your scenarios.  I only tested
>> with 'make check'.
>>
>>  lib/dpctl.c        | 21 ++++++++++++++++++++-
>>  lib/dpif-netlink.c | 45 ---------------------------------------------
>>  2 files changed, 20 insertions(+), 46 deletions(-)
>>
>> diff --git a/lib/dpctl.c b/lib/dpctl.c
>> index 09ae97f25..2f859a753 100644
>> --- a/lib/dpctl.c
>> +++ b/lib/dpctl.c
>> @@ -1157,6 +1157,16 @@ dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
>>          goto out_freeactions;
>>      }
>>
>> +    if (!ufid_present && dpctl_p->is_appctl) {
>> +        /* Generating UFID for this flow so it could be offloaded to HW.  We're
>> +         * not doing that if invoked from ovs-dpctl utility because
>> +         * odp_flow_key_hash() uses randomly generated base for flow hashes
>> +         * that will be different for each invocation.  And, anyway, offloading
>> +         * is only available via appctl. */
>> +        odp_flow_key_hash(key.data, key.size, &ufid);
>> +        ufid_present = true;
>> +    }
>> +
>>      /* The flow will be added on all pmds currently in the datapath. */
>>      error = dpif_flow_put(dpif, flags,
>>                            key.data, key.size,
>> @@ -1268,6 +1278,7 @@ dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
>>      struct ofpbuf mask; /* To be ignored. */
>>      struct dpif *dpif;
>>      ovs_u128 ufid;
>> +    bool ufid_generated;
>>      bool ufid_present;
>>      struct simap port_names;
>>      int n, error;
>> @@ -1303,6 +1314,14 @@ dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
>>          goto out;
>>      }
>>
>> +    if (!ufid_present && dpctl_p->is_appctl) {
>> +        /* While adding flow via appctl we're generating UFID to make HW
> 
> Should be “While deleting…”

Nope.  I tried to describe that we're generating UFID here only because we're
doing that inside dpctl_put_flow().  So, if we'll not, we will not be able
to remove the flow.  If i wasn't clear, maybe you could suggest another wording?

"While deleting flow via appctl we're generating UFID to make HW offloading
possible."  is not actually a fully correct sentence.  We're generating UFID
while deleting to be able to actually delete it, regardless of HW offload.

> 
>> +         * offloading possible.  Generating UFID here to be sure that such
>> +         * flows could be removed the same way they were added. */
>> +        odp_flow_key_hash(key.data, key.size, &ufid);
>> +        ufid_present = ufid_generated = true;
>> +    }
>> +
>>      /* The flow will be deleted from all pmds currently in the datapath. */
>>      error = dpif_flow_del(dpif, key.data, key.size,
>>                            ufid_present ? &ufid : NULL, PMD_ID_NULL,
>> @@ -1310,7 +1329,7 @@ dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
>>
>>      if (error) {
>>          dpctl_error(dpctl_p, error, "deleting flow");
>> -        if (error == ENOENT && !ufid_present) {
>> +        if (error == ENOENT && (!ufid_present || ufid_generated)) {
>>              struct ds s;
>>
>>              ds_init(&s);
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 7da4fb54d..2f881e4fa 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -2237,55 +2237,12 @@ dpif_netlink_operate_chunks(struct dpif_netlink *dpif, struct dpif_op **ops,
>>      }
>>  }
>>
>> -static void
>> -dpif_netlink_try_update_ufid__(struct dpif_op *op, ovs_u128 *ufid)
>> -{
>> -    switch (op->type) {
>> -    case DPIF_OP_FLOW_PUT:
>> -        if (!op->flow_put.ufid) {
>> -            odp_flow_key_hash(op->flow_put.key, op->flow_put.key_len,
>> -                              ufid);
>> -            op->flow_put.ufid = ufid;
>> -        }
>> -        break;
>> -    case DPIF_OP_FLOW_DEL:
>> -        if (!op->flow_del.ufid) {
>> -            odp_flow_key_hash(op->flow_del.key, op->flow_del.key_len,
>> -                              ufid);
>> -            op->flow_del.ufid = ufid;
>> -        }
>> -        break;
>> -    case DPIF_OP_FLOW_GET:
>> -        if (!op->flow_get.ufid) {
>> -            odp_flow_key_hash(op->flow_get.key, op->flow_get.key_len,
>> -                              ufid);
>> -            op->flow_get.ufid = ufid;
>> -        }
>> -        break;
>> -    case DPIF_OP_EXECUTE:
>> -    default:
>> -        break;
>> -    }
>> -}
>> -
>> -static void
>> -dpif_netlink_try_update_ufid(struct dpif_op **ops, ovs_u128 *ufid,
>> -                             size_t n_ops)
>> -{
>> -    int i;
>> -
>> -    for (i = 0; i < n_ops; i++) {
>> -        dpif_netlink_try_update_ufid__(ops[i], &ufid[i]);
>> -    }
>> -}
>> -
>>  static void
>>  dpif_netlink_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops,
>>                       enum dpif_offload_type offload_type)
>>  {
>>      struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>      struct dpif_op *new_ops[OPERATE_MAX_OPS];
>> -    ovs_u128 ufids[OPERATE_MAX_OPS];
>>      int count = 0;
>>      int i = 0;
>>      int err = 0;
>> @@ -2295,8 +2252,6 @@ dpif_netlink_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops,
>>          return;
>>      }
>>
>> -    dpif_netlink_try_update_ufid(ops, ufids, n_ops);
>> -
>>      if (offload_type != DPIF_OFFLOAD_NEVER && netdev_is_flow_api_enabled()) {
>>          while (n_ops > 0) {
>>              count = 0;
>> -- 
>> 2.25.4
> 



More information about the dev mailing list