Eelco Chaudron <[email protected]> 于2022年11月24日周四 00:08写道：
> > > 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. The second one will also have no statistics issues. As long as the megaflow and ukey have the same lifetime, i.e. it's impossible that ukey is dead but megaflow is alive, or reverse (逆向) , there are no statistics issues. So I think the second solution (补丁) does not introduce statistics issues. will submit a v6. > > > 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 > > -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
