On Tue, Jun 09, 2015 at 06:19:35PM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Jun 09, 2015 at 03:03:03PM -0300, Flavio Leitner wrote:
> > On Mon, Jun 08, 2015 at 01:05:41PM -0300, Thadeu Lima de Souza Cascardo
> > wrote:
> > > Support IGMPv3 messages with multiple records. Make sure all IGMPv3
> > > messages go through slow path, since they may carry multiple multicast
> > > addresses, unlike IGMPv2.
> > >
> > > Tests done:
> > >
> > > * multiple addresses in IGMPv3 report are inserted in mdb;
> > > * address is removed from IGMPv3 if record is INCLUDE_MODE;
> > > * reports sent on a burst with same flow all go to userspace;
> > > * IGMPv3 reports go to mrouters, i.e., ports that have issued a query.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <casca...@redhat.com>
> > > ---
> > > lib/mcast-snooping.c | 52
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > lib/mcast-snooping.h | 5 +++++
> > > lib/packets.h | 28 ++++++++++++++++++++++++
> > > ofproto/ofproto-dpif-xlate.c | 27 +++++++++++++++++++----
> > > 4 files changed, 108 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> > > index c3ffd6b..6b89a6c 100644
> > > --- a/lib/mcast-snooping.c
> > > +++ b/lib/mcast-snooping.c
> > > @@ -69,6 +69,7 @@ mcast_snooping_is_membership(ovs_be16 igmp_type)
> > > switch (ntohs(igmp_type)) {
> > > case IGMP_HOST_MEMBERSHIP_REPORT:
> > > case IGMPV2_HOST_MEMBERSHIP_REPORT:
> > > + case IGMPV3_HOST_MEMBERSHIP_REPORT:
> > > case IGMP_HOST_LEAVE_MESSAGE:
> > > return true;
> > > }
> > > @@ -416,6 +417,57 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
> > > ovs_be32 ip4,
> > > return learned;
> > > }
> > >
> > > +int
> > > +mcast_snooping_add_report(struct mcast_snooping *ms,
> > > + const struct dp_packet *p,
> > > + uint16_t vlan, void *port)
> > > +{
> > > + ovs_be32 ip4;
> > > + size_t offset;
> > > + const struct igmpv3_header *igmpv3;
> > > + const struct igmpv3_record *record;
> > > + int count = 0;
> > > + int ngrp;
> > > +
> > > + offset = p->l4_ofs;
> >
> > The above could be like this:
> > offset = dp_packet_l4(p) - dp_packet_data(p)
> > to avoid accessing internals of dp_packet.
> >
>
> That's how it originally was written, but I thought that using it like
> this would protect the code against l4_ofs being UINT16_MAX, and
> dp_packet_l4 returning NULL.
>
> Any thoughs on that? Should I just check for dp_packet_l4 returning NULL?
The UINT16_MAX is reserved and means not set.
see dp_packet_reset_offsets()
[...]
> > > @@ -2270,8 +2281,16 @@ xlate_normal(struct xlate_ctx *ctx)
> > > if (mcast_snooping_is_membership(flow->tp_src) ||
> > > mcast_snooping_is_query(flow->tp_src)) {
> > > update_mcast_snooping_table(ctx->xbridge, flow, vlan,
> > > - in_xbundle);
> > > + in_xbundle,
> > > ctx->xin->packet);
> > > + /*
> > > + * Since there may be multiple addresses in the
> > > + * packet, flow keys can't be used. Setting slow
> > > + * prevents the flow from being installed in the DP.
> > > + */
> > > + if (ntohs(flow->tp_src) ==
> > > IGMPV3_HOST_MEMBERSHIP_REPORT) {
> > > + ctx->xout->slow |= SLOW_ACTION;
> > > }
> >
> > Hm, I think the slow path enforcement is valid for all IGMP versions
> > because even for IGMPv1 or v2, a report can't pass through the DP
> > directly without refreshing the mdb. It works today because the
> > flow will expire way before the IGMP expiration or next querier
> > probe.
> >
> > fbl
>
> That makes sense. In that case, I think I could send a different patch
> for that, to be applied before IGMPv3 support.
Agree.
Thanks,
fbl
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
