On 30 Sep 2022, at 16:42, Peng He wrote:
> 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? I mean that if the flow was not marked for deletion, we can not just change the state to EVICTED, it should be according to the states transitions in transition_ukey_at() or, they should be updated. Because we go from UKEY_OPERATIONAL -> UKEY_EVICTED in the none delete case. >>> 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 ? Guess you might want to research as we update the stats here because the dpath flow will be deleted. And now we do the same, i.e. change failed, so the flow is gone!? So we might need to update it. >> >>> 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
