mir/ovs
2
0
Fork 0
mirror of https://github.com/openvswitch/ovs synced 2025-08-31 14:25:26 +00:00
Code Issues Releases Wiki Activity

dpctl: Fix broken flow deletion via ovs-dpctl due to missing ufid.

Browse Source 
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@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 e61984e781 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@redhat.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374863.html
Fixes: e61984e781 ("dpif-netlink: Generate ufids for installing TC flowers")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Tested-by: Eelco Chaudron <echaudro@redhat.com>
This commit is contained in:
Ilya Maximets
2020-10-05 12:09:55 +02:00
parent 3c6b3a519a
commit bbe2e39287
2 changed files with 20 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
lib/dpctl.c
View File

@@ -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
* 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);

45
lib/dpif-netlink.c
View File

@@ -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;