On 22 Nov 2022, at 2:44, Peng He wrote:
> Hi, > > After a second thought, I think maybe keeping INCONSISTENT just for the > modify error is a better option. > > With current patch: > 1. > the modify error case: > OPERATIONAL -> INCONSISTENT -> EVICTING -> EVICTED > 2. > the delete error case: > EVICTING -> EVICTED > > Change both to INCONSISTENT: > > the modify error case: > did not change. > > the delete error case: > EVICTING -> INCONSISTENT -> EVICTED? > this will make the state machine allows both INCONSISTENT -> EVICTING and > EVICTING -> INCONSISTENT transitions. > which I guess it's more confusing ... > > Another solution is that, drop INCONSISTENT state, if modify fails, just > changes to EVICTING. > and let the revalidate or sweep to take care of EVICTING state ukey and > initial another dp_ops to remove it. > > I now prefer the second solution, what do you think? Yes, the second one sounds more straightforward, but would it not cause issues with the statistics? If not we should probably go with this one. If we run into statistics issues, which I think we will, let’s keep the code as in v5, but add some code comments on why the paths are different, i.e. to keep stats happy. If stats are inconsistent, we should also add a test case so further fixes/changes will not mess this up. > Peng He <[email protected]> 于2022年11月22日周二 09:01写道： > >> >> Eelco Chaudron <[email protected]> 于2022年11月18日周五 15:35写道： >> >>> >>> >>> On 18 Nov 2022, at 2:53, Peng He wrote: >>> >>>> Eelco Chaudron <[email protected]> 于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 <[email protected]> >>>>> >>>>> 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. >>> >>> I meant chaining the UKEY_EVICTED also to UKEY_INCONSISTENT. >>> >> >> By chaining, you mean changing, correct? >> I got it. So if error, we should all go to the UKEY_INCONSISTENT. >> yes, it makes sense, will do that in the next version. >> >> >>> >>>> >>>>> >>>>>> + } >>>>>> 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 (列表) >>>>>> [email protected] >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>> >>>>> >>>> >>>> -- >>>> hepeng >>> >>> >> >> -- >> hepeng >> > > > -- > hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
