| Message ID | 20250123192119.2421-1-vipul.ashri@ericsson.com |
|---|---|
| State | Superseded |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev,v2] ofproto: Bridge del causing trafic drop for all VM. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Vipul Ashri via dev <ovs-dev@openvswitch.org> writes: > RCA: This issue was reported in multi-bridge OVS environment, having > dynamic bridges. Each VM is tied with one bridge via vhu and a VM > add/del causes bridge add/del. We found individual bridge deletion, > while deleting VM was impacting overall OVS traffic, causing temp > traffic outage even for other unrelated VMs. The packets are dropped > with datapath_drop_lock_error coverage counter. We found that when > deleting a bridge, we disable upcall for some time and flush entire > dpctl flows. As we donot have per bridge dpctl flow, so any packets > coming during this period of time will miss dpctl flow and go for > upcall, which being disabled now will be dropped. > > Fix is to bypass "ofproto->ofproto_class->flush(ofproto)", once > this callback holds the lock and dp flows flushed, leads to complete > traffic outage. As per my analysis no need to do udpif_flush() to > flush all datapath flows instead needed flows will be auto-deleted > for required bridge with port delete or destroy APIs already called > further in codeflow and bridge add/delete paths. > > Signed-off-by: Vipul Ashri <vipul.ashri@ericsson.com> > --- This code leaves (what I think is) a very werid race condition in place as the dp rules will still be present, so a port ID recycle would cause unexpected and bad side effects, depending on the DP used. You mention that you are only using this for your usage of userspace datapath, but this change would impact all datapaths, so I don't think it can be merged as-is. > ofproto/ofproto.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 3df64efb9..559f92b04 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -1680,16 +1680,11 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule) > } > > static void > -ofproto_flush__(struct ofproto *ofproto, bool del) > +ofproto_flush__(struct ofproto *ofproto) > OVS_EXCLUDED(ofproto_mutex) > { > struct oftable *table; > > - /* This will flush all datapath flows. */ > - if (del && ofproto->ofproto_class->flush) { > - ofproto->ofproto_class->flush(ofproto); > - } > - > /* XXX: There is a small race window here, where new datapath flows can be > * created by upcall handlers based on the existing flow table. We can not > * call ofproto class flush while holding 'ofproto_mutex' to prevent this, > @@ -1839,7 +1834,7 @@ ofproto_destroy(struct ofproto *p, bool del) > return; > } > > - ofproto_flush__(p, del); > + ofproto_flush__(p); > HMAP_FOR_EACH_SAFE (ofport, hmap_node, &p->ports) { > ofport_destroy(ofport, del); > } > @@ -2420,7 +2415,7 @@ void > ofproto_flush_flows(struct ofproto *ofproto) > { > COVERAGE_INC(ofproto_flush); > - ofproto_flush__(ofproto, false); > + ofproto_flush__(ofproto); > connmgr_flushed(ofproto->connmgr); > } >
> This code leaves (what I think is) a very werid race condition in place > as the dp rules will still be present, so a port ID recycle would cause > unexpected and bad side effects, depending on the DP used. As this is Bridge add/del sequence, I doubt if port ID recycle is actually Possible or not. Although I have tested most of bridge deletion in combination of running traffic and never see any issue or abnormal behaviour. > You mention that you are only using this for your usage of userspace > datapath, but this change would impact all datapaths, so I don't think > it can be merged as-is. I think you are right here, and we can skip flushing only for user space Datapath type. Let me raise another patch having this comment addressed.
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 3df64efb9..559f92b04 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1680,16 +1680,11 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule) } static void -ofproto_flush__(struct ofproto *ofproto, bool del) +ofproto_flush__(struct ofproto *ofproto) OVS_EXCLUDED(ofproto_mutex) { struct oftable *table; - /* This will flush all datapath flows. */ - if (del && ofproto->ofproto_class->flush) { - ofproto->ofproto_class->flush(ofproto); - } - /* XXX: There is a small race window here, where new datapath flows can be * created by upcall handlers based on the existing flow table. We can not * call ofproto class flush while holding 'ofproto_mutex' to prevent this, @@ -1839,7 +1834,7 @@ ofproto_destroy(struct ofproto *p, bool del) return; } - ofproto_flush__(p, del); + ofproto_flush__(p); HMAP_FOR_EACH_SAFE (ofport, hmap_node, &p->ports) { ofport_destroy(ofport, del); } @@ -2420,7 +2415,7 @@ void ofproto_flush_flows(struct ofproto *ofproto) { COVERAGE_INC(ofproto_flush); - ofproto_flush__(ofproto, false); + ofproto_flush__(ofproto); connmgr_flushed(ofproto->connmgr); }
RCA: This issue was reported in multi-bridge OVS environment, having dynamic bridges. Each VM is tied with one bridge via vhu and a VM add/del causes bridge add/del. We found individual bridge deletion, while deleting VM was impacting overall OVS traffic, causing temp traffic outage even for other unrelated VMs. The packets are dropped with datapath_drop_lock_error coverage counter. We found that when deleting a bridge, we disable upcall for some time and flush entire dpctl flows. As we donot have per bridge dpctl flow, so any packets coming during this period of time will miss dpctl flow and go for upcall, which being disabled now will be dropped. Fix is to bypass "ofproto->ofproto_class->flush(ofproto)", once this callback holds the lock and dp flows flushed, leads to complete traffic outage. As per my analysis no need to do udpif_flush() to flush all datapath flows instead needed flows will be auto-deleted for required bridge with port delete or destroy APIs already called further in codeflow and bridge add/delete paths. Signed-off-by: Vipul Ashri <vipul.ashri@ericsson.com> --- ofproto/ofproto.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)