On 23 Sep 2022, at 18:29, 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 <[email protected]> > --- > ofproto/ofproto-dpif-upcall.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 7ad728adf..7ea2a30f5 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2416,23 +2416,26 @@ 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); I think we need transition_ukey(ukey, UKEY_EVICTING) also for none delete actions, so the state machine logging is correct. > transition_ukey(op->ukey, UKEY_EVICTED); > ovs_mutex_unlock(&op->ukey->mutex); > } > + /* if it's a flow_del error, 'stats' is unusable, it's ok > + * to discard the stats. > + */ > + continue; > + } > + > + 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. */ Can you explain why deleting the “valid” datapath rule does not need updating the stats? > continue; > } > > -- > 2.25.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
