| Message ID | 20230609150331.817774-1-hepeng.0320@bytedance.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,ovs-dev,v11] ofproto-dpif-upcall: fix push_dp_ops | expand |
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 <hepeng.0320@bytedance.com> 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
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 <hepeng.0320@bytedance.com> > > 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 >
Ilya Maximets <i.maximets@ovn.org> 于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 <hepeng.0320@bytedance.com> > > > > 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 > > > >
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); /* 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 */ + 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); + reval_op_init(&ops[n_ops++], UKEY_DELETE, udpif, ukey, + &recircs, &odp_actions); + ovs_mutex_unlock(&ukey->mutex); + continue; + } + + 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 +AT_CHECK([grep -q -E ".*failed to put.*$" ovs-vswitchd.log]) + +dnl Remove warning logs to let testsuite pass. +OVS_VSWITCHD_STOP([dnl +"/.*failed to put.*$/d +/.*failed to flow_del.*$/d" +]) +AT_CLEANUP
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 <hepeng.0320@bytedance.com> --- ofproto/ofproto-dpif-upcall.c | 51 +++++++++++++++++++++++++---------- tests/dpif-netdev.at | 44 ++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 14 deletions(-)