Ilya Maximets <[email protected]> 于2023年6月23日周五 20:40写道：
> 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 ? > > sure. will send a new version. > 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: > sure. > > OVS_VSWITCHD_STOP([" > /.*failed to put.*$/d > /.*failed to flow_del.*$/d > "]) > > >> +AT_CLEANUP > >> -- > >> 2.25.1 > > > > -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
