On 6/23/23 14:20, Eelco Chaudron wrote: > > > On 9 Jun 2023, at 17:03, Peng He wrote: > >> push_dp_ops only handles delete ops errors but ignores the modify >> ops results. It's better to handle all the dp operation errors in >> a consistent way. >> >> This patch prevents the inconsistency by considering modify failure >> in revalidators. >> >> To note, we cannot perform two state transitions and change ukey_state >> into UKEY_EVICTED directly here, because, if we do so, the >> sweep will remove the ukey alone and leave dp flow alive. Later, the >> dump will retrieve the dp flow and might even recover it. This will >> contribute the stats of this dp flow twice. >> >> v8->v9: add testsuite and delete INCONSISTENT ukey at revalidate_sweep. >> v9->v10: change the commit message and refine the test case. >> v10->v11: fix indentation and refine the test case. >> >> Signed-off-by: Peng He <[email protected]> > > Thanks for making these changes, there is a merge conflict on the tests case > and some small comments below. > > So maybe you can send a v12 and I’ll do a quick check and ACK it. What do you > think? > > Cheers, > > Eelco > > >> --- >> ofproto/ofproto-dpif-upcall.c | 51 +++++++++++++++++++++++++---------- >> tests/dpif-netdev.at | 44 ++++++++++++++++++++++++++++++ >> 2 files changed, 81 insertions(+), 14 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index cd57fdbd9..c920c749c 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit); >> COVERAGE_DEFINE(upcall_flow_limit_kill); >> COVERAGE_DEFINE(upcall_ukey_contention); >> COVERAGE_DEFINE(upcall_ukey_replace); >> +COVERAGE_DEFINE(dumped_inconsistent_flow); > > Can you add this in alphabetical order? > >> >> /* A thread that reads upcalls from dpif, forwards each upcall's packet, >> * and possibly sets up a kernel flow as a cache. */ >> @@ -258,6 +259,7 @@ enum ukey_state { >> UKEY_CREATED = 0, >> UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is >> queued. */ >> UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */ >> + UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is inconsistent. >> */ >> UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued. >> */ >> UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */ >> UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */ >> @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum >> ukey_state dst, >> * UKEY_VISIBLE -> UKEY_EVICTED >> * A handler attempts to install the flow, but the datapath rejects it. >> * Consider that the datapath has already destroyed it. >> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT >> + * A revalidator modifies the flow with error returns. >> + * UKEY_INCONSISTENT -> UKEY_EVICTING >> + * A revalidator decides to evict the datapath flow. >> * UKEY_OPERATIONAL -> UKEY_EVICTING >> * A revalidator decides to evict the datapath flow. >> * UKEY_EVICTING -> UKEY_EVICTED >> @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum >> ukey_state dst, >> * UKEY_EVICTED -> UKEY_DELETED >> * A revalidator has removed the ukey from the umap and is deleting it. >> */ >> - if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE && >> - dst < UKEY_DELETED)) { >> + if (ukey->state == dst - 1 || >> + (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) || >> + (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) { >> ukey->state = dst; >> } else { >> struct ds ds = DS_EMPTY_INITIALIZER; >> @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op >> *ops, size_t n_ops) >> >> for (i = 0; i < n_ops; i++) { >> struct ukey_op *op = &ops[i]; >> - struct dpif_flow_stats *push, *stats, push_buf; >> - >> - stats = op->dop.flow_del.stats; >> - push = &push_buf; >> - >> - if (op->dop.type != DPIF_OP_FLOW_DEL) { >> - /* Only deleted flows need their stats pushed. */ >> - continue; >> - } >> >> if (op->dop.error) { >> - /* flow_del error, 'stats' is unusable. */ >> if (op->ukey) { >> ovs_mutex_lock(&op->ukey->mutex); >> - transition_ukey(op->ukey, UKEY_EVICTED); >> + if (op->dop.type == DPIF_OP_FLOW_DEL) { >> + transition_ukey(op->ukey, UKEY_EVICTED); >> + } else { >> + /* Modification of the flow failed */ > > You forgot to copy the dot. > >> + transition_ukey(op->ukey, UKEY_INCONSISTENT); >> + } >> ovs_mutex_unlock(&op->ukey->mutex); >> } >> continue; >> } >> >> + if (op->dop.type != DPIF_OP_FLOW_DEL) { >> + /* Only deleted flows need their stats pushed. */ >> + continue; >> + } >> + >> + struct dpif_flow_stats *push, *stats, push_buf; >> + >> + stats = op->dop.flow_del.stats; >> + push = &push_buf; >> + >> if (op->ukey) { >> ovs_mutex_lock(&op->ukey->mutex); >> transition_ukey(op->ukey, UKEY_EVICTED); >> @@ -2839,6 +2851,16 @@ revalidate(struct revalidator *revalidator) >> continue; >> } >> >> + if (ukey->state == UKEY_INCONSISTENT) { >> + ukey->dump_seq = dump_seq; >> + COVERAGE_INC(dumped_inconsistent_flow); > > I suggested moving the coverage counter outside of the mutex. > >> + reval_op_init(&ops[n_ops++], UKEY_DELETE, udpif, ukey, >> + &recircs, &odp_actions); >> + ovs_mutex_unlock(&ukey->mutex); >> + continue; >> + } >> + >> + > > One empty newline will be enough here. > >> if (ukey->state <= UKEY_OPERATIONAL) { >> /* The flow is now confirmed to be in the datapath. */ >> transition_ukey(ukey, UKEY_OPERATIONAL); >> @@ -2927,13 +2949,14 @@ revalidator_sweep__(struct revalidator *revalidator, >> bool purge) >> } >> ukey_state = ukey->state; >> if (ukey_state == UKEY_OPERATIONAL >> + || (ukey_state == UKEY_INCONSISTENT) >> || (ukey_state == UKEY_VISIBLE && purge)) { >> struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; >> bool seq_mismatch = (ukey->dump_seq != dump_seq >> && ukey->reval_seq != reval_seq); >> enum reval_result result; >> >> - if (purge) { >> + if (purge || ukey_state == UKEY_INCONSISTENT) { >> result = UKEY_DELETE; >> } else if (!seq_mismatch) { >> result = UKEY_KEEP; >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index baab60a22..2d0dc14c1 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -716,3 +716,47 @@ AT_CHECK([test `ovs-vsctl get Interface p2 >> statistics:tx_q0_packets` -gt 0 -a dn >> >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> + >> +AT_SETUP([dpif-netdev - revalidators handle dp modification fail correctly]) >> +OVS_VSWITCHD_START( >> + [add-port br0 p1 \ >> + -- set interface p1 type=dummy \ >> + -- set bridge br0 datapath-type=dummy \ >> + -- add-port br0 p2 \ >> + -- set interface p2 type=dummy -- >> + ]) >> + >> +ovs-ofctl add-flow br0 'table=0,in_port=p1,actions=p2' >> +ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.0.0.2),tcp(src=1,dst=2)' >> +ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.0.0.2),tcp(src=1,dst=2)'
Could you, please, also wrap above commands in AT_CHECK ? It not only checks the result, but it also adds these commands into the testsuite log. Otherwise it will be hard to understand what went wrong if these ever fail. >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*thread://' | >> strip_xout_keep_actions ], [0], [ >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), >> packets:0, bytes:0, used:0.0s, actions:2 >> +]) >> + >> +dnl Wait for the dp flow to enter OPERATIONAL state. >> +ovs-appctl revalidator/wait Same here. >> + >> +AT_CHECK([ovs-appctl revalidator/pause]) >> + >> +dnl Delete all dp flows, so flow modification will fail. >> +AT_CHECK([ovs-appctl dpctl/del-flows]) >> + >> +AT_CHECK([ovs-appctl revalidator/resume]) >> + >> +dnl Replace OpenFlow rules, trigger revalidation and wait for it to >> complete. >> +AT_CHECK([echo 'table=0,in_port=p1,ip actions=ct(commit)' | ovs-ofctl >> --bundle replace-flows br0 -]) >> +ovs-appctl revalidator/wait And here. >> + >> +dnl Inconsistent ukey should be deleted. >> +AT_CHECK([ovs-appctl upcall/show | grep keys | grep -q -v 0], [1]) >> + >> +dnl Check the error log of the flow modification > > You forgot to add a dot. But maybe: > > +dnl Check the log for the flow modification error. > > >> +AT_CHECK([grep -q -E ".*failed to put.*$" ovs-vswitchd.log]) >> + >> +dnl Remove warning logs to let testsuite pass. > > +dnl Remove warning logs to let the test suite pass. > >> +OVS_VSWITCHD_STOP([dnl >> +"/.*failed to put.*$/d >> +/.*failed to flow_del.*$/d" >> +]) Nit: You may put quotes on separate lines and sed commands do not need to start from the line beginning: OVS_VSWITCHD_STOP([" /.*failed to put.*$/d /.*failed to flow_del.*$/d "]) >> +AT_CLEANUP >> -- >> 2.25.1 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
