Message ID | 20221127072856.377172-2-hepeng.0320@bytedance.com |
---|---|
State | Accepted |
Commit | 5dfc8309d9df10075c6dd85250acda6daaeca6fd |
Headers | show |
Series | [ovs-dev,ovs-dev,v7,1/3] ofproto-dpif-upcall: fix push_dp_ops | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Peng He, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He <hepeng.0320@bytedance.com> Lines checked: 56, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 11/27/22 08:28, Peng He wrote: > The userspace datapath mananges all the magaflows by a cmap. The cmap > data structrue will grow/shrink during the datapath processing and it > will re-position megaflows. This might result in two revalidator threads > might process a same megaflow during one dump stage. > > Consider a situation that, revalidator 1 processes a megaflow A, and > decides to delete it from the datapath, at the mean time, this megaflow > A is also queued in the process batch of revalidator 2. Normally it's ok > for revalidators to process the same megaflow multiple times, as the > dump_seq shows it's already dumped and the stats will not be contributed > twice. > > Assume that right after A is deleted, a PMD thread generates again > a new megaflow B which has the same match and action of A. The ukey > of megaflow B will replace the one of megaflow A. Now the ukey B is > new to the revalidator system and its dump seq is 0. > > Now since the dump seq of ukey B is 0, when processing megaflow A, > the revalidator 2 will not identify this megaflow A has already been > dumped by revalidator 1 and will contribute the old megaflow A's stats > again, this results in an inconsistent stats between ukeys and megaflows. > > To fix this, the newly generated the ukey B should take the dump_seq > of the replaced ukey A to avoid a same megaflow being revalidated > twice in one dump stage. > > We observe in the production environment, the OpenFlow rules' stats > sometimes are amplified compared to the actual value. > > Signed-off-by: Peng He <hepeng.0320@bytedance.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > ofproto/ofproto-dpif-upcall.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index c2cefbeb8..8b5db6103 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1877,6 +1877,7 @@ try_ukey_replace(struct umap *umap, struct udpif_key *old_ukey, > ovs_mutex_lock(&new_ukey->mutex); > cmap_replace(&umap->cmap, &old_ukey->cmap_node, > &new_ukey->cmap_node, new_ukey->hash); > + new_ukey->dump_seq = old_ukey->dump_seq; > ovsrcu_postpone(ukey_delete__, old_ukey); > transition_ukey(old_ukey, UKEY_DELETED); > transition_ukey(new_ukey, UKEY_VISIBLE); While there is still some discussion on other patches from the set, I applied this one patch. Backported down to 2.17. Thanks! Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index c2cefbeb8..8b5db6103 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1877,6 +1877,7 @@ try_ukey_replace(struct umap *umap, struct udpif_key *old_ukey, ovs_mutex_lock(&new_ukey->mutex); cmap_replace(&umap->cmap, &old_ukey->cmap_node, &new_ukey->cmap_node, new_ukey->hash); + new_ukey->dump_seq = old_ukey->dump_seq; ovsrcu_postpone(ukey_delete__, old_ukey); transition_ukey(old_ukey, UKEY_DELETED); transition_ukey(new_ukey, UKEY_VISIBLE);