Eelco Chaudron <echau...@redhat.com> 于2023年7月5日周三 21:24写道：
>
>
> On 1 Jul 2023, at 7:11, 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.
> > v11->v12: fix the test suite.
> >
> > Signed-off-by: Peng He <hepeng.0...@bytedance.com>
>
> Thanks for fixing the comments on v11. The changes look good, with one
> little nit (see below), but those can be applied during commit.
>
> Acked-by: Eelco Chaudron <echau...@redhat.com>
>
> > ---
> > ofproto/ofproto-dpif-upcall.c | 50 +++++++++++++++++++++++++----------
> > tests/dpif-netdev.at | 43 ++++++++++++++++++++++++++++++
> > 2 files changed, 79 insertions(+), 14 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > index 04b583f81..cde03abc6 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -53,6 +53,7 @@
> > VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
> >
> > COVERAGE_DEFINE(dumped_duplicate_flow);
> > +COVERAGE_DEFINE(dumped_inconsistent_flow);
> > COVERAGE_DEFINE(dumped_new_flow);
> > COVERAGE_DEFINE(handler_duplicate_upcall);
> > COVERAGE_DEFINE(revalidate_missed_dp_flow);
> > @@ -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;
> > @@ -2490,26 +2497,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);
> > @@ -2857,6 +2869,15 @@ revalidate(struct revalidator *revalidator)
> > continue;
> > }
> >
> > + if (ukey->state == UKEY_INCONSISTENT) {
> > + ukey->dump_seq = dump_seq;
> > + reval_op_init(&ops[n_ops++], UKEY_DELETE, udpif, ukey,
> > + &recircs, &odp_actions);
> > + ovs_mutex_unlock(&ukey->mutex);
> > + COVERAGE_INC(dumped_inconsistent_flow);
> > + continue;
> > + }
> > +
> > if (ukey->state <= UKEY_OPERATIONAL) {
> > /* The flow is now confirmed to be in the datapath. */
> > transition_ukey(ukey, UKEY_OPERATIONAL);
> > @@ -2945,13 +2966,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 67adf27fb..ce7662aca 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -812,3 +812,46 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> > ])
> > 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 --
> > + ])
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 'table=0,in_port=p1,actions=p2'])
> > +AT_CHECK([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 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.
> > +AT_CHECK([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 -])
> > +AT_CHECK([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 log for the flow modification error.
> > +AT_CHECK([grep -q -E ".*failed to put.*$" ovs-vswitchd.log])
> > +
> > +dnl Remove warning logs to let test suite pass.
> > +OVS_VSWITCHD_STOP(["dnl
> > +/.*failed to put.*$/d
> > +/.*failed to flow_del.*$/d"])
>
> You missed the indentation suggested by Ilya:
>
>
Which kind of the email client are you using...
The indentation here is really easy to miss....
>
>
> dnl Remove warning logs to let test suite pass.
> OVS_VSWITCHD_STOP(["dnl
> -/.*failed to put.*$/d
> -/.*failed to flow_del.*$/d"])
> + /.*failed to put.*$/d
> + /.*failed to flow_del.*$/d"])
> AT_CLEANUP
>
>
> > +AT_CLEANUP
> > --
> > 2.25.1
>
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
