Eelco Chaudron <[email protected]> 于2022年9月30日周五 22:20写道：
> > > 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. > you mean for modify error, we should modify the ukey into UKEY_EVICTING instead of UKEY_EVICTED? If so, should we add a new op to delete such datapath flow? > > 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? > The comment is included in the original code, I just move it here. And the code is to only update stats for datapath flow deleting ? > > > continue; > > } > > > > -- > > 2.25.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
