| Message ID | 20201005100955.689672-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | bbe2e39287476e7ba3e71d064adb8c0735cf0e95 |
| Headers | show |
| Series | [ovs-dev] dpctl: Fix broken flow deletion via ovs-dpctl due to missing ufid. | expand |
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@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@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@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@redhat.com> Tested-by: Eelco Chaudron <echaudro@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
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@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@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@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@redhat.com> > Tested-by: Eelco Chaudron <echaudro@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 >
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@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@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@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@redhat.com> >> Tested-by: Eelco Chaudron <echaudro@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 >>
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@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@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@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@redhat.com> > Tested-by: Eelco Chaudron <echaudro@redhat.com> Thanks, Eelco! I went ahead and applied this patch to master and backported to 2.14. Best regards, Ilya Maximets.
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 + * 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;
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 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@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@ovn.org> --- 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(-)