Eelco Chaudron <echau...@redhat.com> 于2022年11月16日周三 18:14写道：
>
>
> On 6 Nov 2022, at 8:12, 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.
> >
> > We observe in the production environment that sometimes a megaflow
> > with wrong actions keep staying in datapath. The coverage command shows
> > revalidators have dumped several times, however the correct
> > actions are not set. This implies (暗示) that the ukey's action does not
> > equal to the meagaflow's, i.e. revalidators think the underlying (基础)
> > megaflow's actions are correct however they are not.
> >
> > We also check the megaflow using the ofproto/trace command, and the
> > actions are not matched with the ones in the actual magaflow. By
> > performing a revalidator/purge command, the right actions are set.
> >
> > 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.
> >
> > Signed-off-by: Peng He <hepeng.0...@bytedance.com>
>
> Hi Peng,
>
> Thanks for looking at the statistics part, see some comments inline!
>
> In addition, I already acked patch (补丁) 2 out of this series, but it
> mentions patch x/3, but I do not see patch 3 in this series. Is this
> missing? Or are there only two patches (补丁) left?
there are only two patches. the third one is about the race comments, which
is not in this patchset.
I guess I made some mistake.
>
>
> Cheers,
>
> Eelco
>
>
> > ---
> > ofproto/ofproto-dpif-upcall.c | 39 ++++++++++++++++++++++-------------
> > 1 file (文件) changed, 25 insertions(+), 14 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > index 7ad728adf..a7970fa9b 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -254,6 +254,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 modified
> (修改) but failed */
> > 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 (推迟) . */
> > @@ -1966,6 +1967,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
> > @@ -1974,7 +1979,8 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> > * 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)) {
> > + dst < UKEY_DELETED) ||
> > + (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
> > ukey->state = dst;
> > } else {
> > struct ds ds = DS_EMPTY_INITIALIZER;
> > @@ -2416,26 +2422,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)
>
> We should {} to the if branch.
>
OK
>
> > + transition_ukey(op->ukey, UKEY_EVICTED);
> > + else {
> > + transition_ukey(op->ukey, UKEY_INCONSISTENT);
>
> Is there a specific (特定) reason to have delete go through a different
> state on failure? If not, it might be more (更多) consistent especially when
> debugging.
As is stated in the commit comments,
"
> 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.
"
So I have to introduce another state here.
>
> > + }
> > ovs_mutex_unlock(&op->ukey->mutex);
> > }
> > + /* if modify (修改) or delete fails, there is no need to push
> stats */
>
> Captial for “If” and ending with a dot.
>
Sure, will do that.
>
> > 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);
> > @@ -2848,14 +2859,14 @@ revalidator_sweep__(struct revalidator
> *revalidator, bool purge (吹扫) )
> > continue;
> > }
> > ukey_state = ukey->state;
> > - if (ukey_state == UKEY_OPERATIONAL
> > + 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;
> > --
> > 2.25.1
> >
> > _______________________________________________
> > dev mailing list (列表)
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
