| Message ID | 20170711183252.GL29918@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
This patch can slove the problem. But I think wc->masks.tp_src should not be masked in userspace as igmp type is not supported in datapath. Otherwise, flow_wildcards_has_extra() in revalidate_ukey__ always return true for igmp megaflow, and igmp flow is never kept in datapath. Thanks, Huanle On Wednesday, July 12, 2017, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Jun 22, 2017 at 01:04:48AM +0800, Huanle Han wrote: > > In "Normal" action, igmp report packet is expected to processed in slow > > path. > > However, the igmp_type(flow->tp_src) is not supported to be masked in > > datapath. > > Then ovs-vswitchd revalidate the flow with igmp_type(flow->tp_src) == 0. > It > > leads to a "multicast traffic, flooding" action and overwrites the > > "userspace()" in datapath. > > > > This is code segment from function "xlate_normal": > > > > if (is_igmp(flow, wc)) { > > memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > > if (mcast_snooping_is_membership(flow->tp_src) || > > mcast_snooping_is_query(flow->tp_src)) { <<<<<<<<< > > flow->tp_src is 0 > > if (ctx->xin->allow_side_effects && ctx->xin->packet) { > > update_mcast_snooping_table(ctx, flow, vlan, > > in_xbundle, > > ctx->xin->packet); > > } > > /* > > * IGMP packets need to take the slow path, in order to > be > > * processed for mdb updates. That will prevent expires > > * firing off even after hosts have sent reports. > > */ > > ctx->xout->slow |= SLOW_ACTION; > > } > > ....... > > } > > > > To reproduce this bug, you need to ovs-appctl upcall/disable-megaflows > > Thanks for the bug report. > > Does the following patch fix the problem? > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 089c7f170d18..5030dafd9f42 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2720,6 +2720,13 @@ xlate_normal(struct xlate_ctx *ctx) > struct mcast_group *grp = NULL; > > if (is_igmp(flow, wc)) { > + /* > + * IGMP packets need to take the slow path, in order to be > + * processed for mdb updates. That will prevent expires > + * firing off even after hosts have sent reports. > + */ > + ctx->xout->slow |= SLOW_ACTION; > + > memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > if (mcast_snooping_is_membership(flow->tp_src) || > mcast_snooping_is_query(flow->tp_src)) { > @@ -2727,12 +2734,6 @@ xlate_normal(struct xlate_ctx *ctx) > update_mcast_snooping_table(ctx, flow, vlan, > in_xbundle, > ctx->xin->packet); > } > - /* > - * IGMP packets need to take the slow path, in order to be > - * processed for mdb updates. That will prevent expires > - * firing off even after hosts have sent reports. > - */ > - ctx->xout->slow |= SLOW_ACTION; > } > > if (mcast_snooping_is_membership(flow->tp_src)) { >
Thanks for verifying the fix. I applied this to master. I think you're right that there's a remaining mismatch here. If you have time to figure out a full and proper solution, then please do so; I'd appreciate it. On Sat, Jul 15, 2017 at 11:11:40AM +0800, Huanle Han wrote: > This patch can slove the problem. > > But I think wc->masks.tp_src should not be masked in userspace as igmp type > is not supported in datapath. Otherwise, flow_wildcards_has_extra() in > revalidate_ukey__ always return true for igmp megaflow, and igmp flow is > never kept in datapath. > > Thanks, > Huanle > > On Wednesday, July 12, 2017, Ben Pfaff <blp@ovn.org> wrote: > > > On Thu, Jun 22, 2017 at 01:04:48AM +0800, Huanle Han wrote: > > > In "Normal" action, igmp report packet is expected to processed in slow > > > path. > > > However, the igmp_type(flow->tp_src) is not supported to be masked in > > > datapath. > > > Then ovs-vswitchd revalidate the flow with igmp_type(flow->tp_src) == 0. > > It > > > leads to a "multicast traffic, flooding" action and overwrites the > > > "userspace()" in datapath. > > > > > > This is code segment from function "xlate_normal": > > > > > > if (is_igmp(flow, wc)) { > > > memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > > > if (mcast_snooping_is_membership(flow->tp_src) || > > > mcast_snooping_is_query(flow->tp_src)) { <<<<<<<<< > > > flow->tp_src is 0 > > > if (ctx->xin->allow_side_effects && ctx->xin->packet) { > > > update_mcast_snooping_table(ctx, flow, vlan, > > > in_xbundle, > > > ctx->xin->packet); > > > } > > > /* > > > * IGMP packets need to take the slow path, in order to > > be > > > * processed for mdb updates. That will prevent expires > > > * firing off even after hosts have sent reports. > > > */ > > > ctx->xout->slow |= SLOW_ACTION; > > > } > > > ....... > > > } > > > > > > To reproduce this bug, you need to ovs-appctl upcall/disable-megaflows > > > > Thanks for the bug report. > > > > Does the following patch fix the problem? > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 089c7f170d18..5030dafd9f42 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -2720,6 +2720,13 @@ xlate_normal(struct xlate_ctx *ctx) > > struct mcast_group *grp = NULL; > > > > if (is_igmp(flow, wc)) { > > + /* > > + * IGMP packets need to take the slow path, in order to be > > + * processed for mdb updates. That will prevent expires > > + * firing off even after hosts have sent reports. > > + */ > > + ctx->xout->slow |= SLOW_ACTION; > > + > > memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > > if (mcast_snooping_is_membership(flow->tp_src) || > > mcast_snooping_is_query(flow->tp_src)) { > > @@ -2727,12 +2734,6 @@ xlate_normal(struct xlate_ctx *ctx) > > update_mcast_snooping_table(ctx, flow, vlan, > > in_xbundle, > > ctx->xin->packet); > > } > > - /* > > - * IGMP packets need to take the slow path, in order to be > > - * processed for mdb updates. That will prevent expires > > - * firing off even after hosts have sent reports. > > - */ > > - ctx->xout->slow |= SLOW_ACTION; > > } > > > > if (mcast_snooping_is_membership(flow->tp_src)) { > >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 089c7f170d18..5030dafd9f42 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2720,6 +2720,13 @@ xlate_normal(struct xlate_ctx *ctx) struct mcast_group *grp = NULL; if (is_igmp(flow, wc)) { + /* + * IGMP packets need to take the slow path, in order to be + * processed for mdb updates. That will prevent expires + * firing off even after hosts have sent reports. + */ + ctx->xout->slow |= SLOW_ACTION; + memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); if (mcast_snooping_is_membership(flow->tp_src) || mcast_snooping_is_query(flow->tp_src)) { @@ -2727,12 +2734,6 @@ xlate_normal(struct xlate_ctx *ctx) update_mcast_snooping_table(ctx, flow, vlan, in_xbundle, ctx->xin->packet); } - /* - * IGMP packets need to take the slow path, in order to be - * processed for mdb updates. That will prevent expires - * firing off even after hosts have sent reports. - */ - ctx->xout->slow |= SLOW_ACTION; } if (mcast_snooping_is_membership(flow->tp_src)) {