[ovs-dev] [PATCH] dpctl: Fix broken flow deletion via ovs-dpctl due to missing ufid.
Eelco Chaudron
echaudro at redhat.com
Tue Oct 6 09:28:13 UTC 2020
On 5 Oct 2020, at 16:12, Ilya Maximets wrote:
> 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.
Reading it again it makes sense, leave it as is..
>>
>>> + * 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