sorry to miss that part,
will take some time to think if we need it or not.
BTW, looks like the kernel handle_upcalls code path has also this issue.
the code is here:
for (i = 0; i < n_ops; i++) {
struct udpif_key *ukey = ops[i].ukey;
if (ukey) {
ovs_mutex_lock(&ukey->mutex);
if (ops[i].dop.error) {
transition_ukey(ukey, UKEY_EVICTED);
} else if (ukey->state < UKEY_OPERATIONAL) {
transition_ukey(ukey, UKEY_OPERATIONAL);
}
ovs_mutex_unlock(&ukey->mutex);
}
}
Eelco Chaudron <echau...@redhat.com> 于2022年10月19日周三 18:44写道：
>
>
> On 3 Oct 2022, at 6:02, 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.
> >
> > Signed-off-by: Peng He <hepeng.0...@bytedance.com>
> > ---
> > ofproto/ofproto-dpif-upcall.c | 37 ++++++++++++++++++++++++-----------
> > 1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > index 7ad728adf..dd1abfdee 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -2416,26 +2416,41 @@ 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 {
> > + /* modify error has variety reasons, it's easy
> > + * to transition the state into UKEY_EVICTED, and
> > + * let revalidator_sweep remove the ukey.
> > + *
> > + * In the next round of revalidating, the
> revalidator
> > + * can recover the ukey through
> ukey_create_from_dpif_flow,
> > + * if recover fails, the revalidator will remove
> the datapath
> > + * flow itself.
> > + */
> > + transition_ukey(op->ukey, UKEY_EVICTING);
> > + transition_ukey(op->ukey, UKEY_EVICTED);
> > + }
> > ovs_mutex_unlock(&op->ukey->mutex);
> > }
> > + /* if modify or delete fails, there is no need to push
> stats */
> > continue;
> > }
> >
> > + if (op->dop.type != DPIF_OP_FLOW_DEL) {
> > + /* Only deleted flows need their stats pushed. */
>
> The code looks good, however, what was the conclusion on updating stats on
> other flows that now got deleted instead of updated? Do they not need their
> stats pushed? See v3 discussion.
>
> > + 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);
> > --
> > 2.25.1
>
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
