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)' > + > +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 > + > +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 > + > +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" > +]) > +AT_CLEANUP > -- > 2.25.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
