[ovs-dev] [PATCH] dpctl: Fix broken flow deletion via ovs-dpctl due to missing ufid.
Eelco Chaudron
echaudro at redhat.com
Mon Oct 5 12:04:09 UTC 2020
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…”
> + * 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