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?
> > + igmpv3 = dp_packet_at(p, offset, IGMPV3_HEADER_LEN);
> > + if (!igmpv3) {
> > + return 0;
> > + }
> > + ngrp = ntohs(igmpv3->ngrp);
> > + offset += IGMPV3_HEADER_LEN;
> > + while (ngrp--) {
> > + bool ret;
> > + record = dp_packet_at(p, offset, sizeof(struct igmpv3_record));
> > + if (!record) {
> > + break;
> > + }
> > + /* Only consider known record types. */
> > + if (record->type < IGMPV3_MODE_IS_INCLUDE
> > + || record->type > IGMPV3_BLOCK_OLD_SOURCES) {
> > + continue;
> > + }
> > + ip4 = record->maddr;
> > + /*
> > + * If record is INCLUDE MODE and there are no sources, it's
> > equivalent
> > + * to a LEAVE.
> > + * */
> > + if (ntohs(record->nsrcs) == 0
> > + && (record->type == IGMPV3_MODE_IS_INCLUDE
> > + || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
> > + ret = mcast_snooping_leave_group(ms, ip4, vlan, port);
> > + } else {
> > + ret = mcast_snooping_add_group(ms, ip4, vlan, port);
> > + }
> > + if (ret) {
> > + count++;
> > + }
> > + offset += sizeof(*record)
> > + + ntohs(record->nsrcs) * sizeof(ovs_be32) +
> > record->aux_len;
> > + }
> > + return count;
> > +}
> > +
> > bool
> > mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
> > uint16_t vlan, void *port)
> > diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
> > index 979b2aa..f4bc8fb 100644
> > --- a/lib/mcast-snooping.h
> > +++ b/lib/mcast-snooping.h
> > @@ -20,6 +20,7 @@
> > #define MCAST_SNOOPING_H 1
> >
> > #include <time.h>
> > +#include "dp-packet.h"
> > #include "hmap.h"
> > #include "list.h"
> > #include "ovs-atomic.h"
> > @@ -181,6 +182,10 @@ mcast_snooping_lookup(const struct mcast_snooping *ms,
> > ovs_be32 dip,
> > bool mcast_snooping_add_group(struct mcast_snooping *ms, ovs_be32 ip4,
> > uint16_t vlan, void *port)
> > OVS_REQ_WRLOCK(ms->rwlock);
> > +int mcast_snooping_add_report(struct mcast_snooping *ms,
> > + const struct dp_packet *p,
> > + uint16_t vlan, void *port)
> > + OVS_REQ_WRLOCK(ms->rwlock);
> > bool mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
> > uint16_t vlan, void *port)
> > OVS_REQ_WRLOCK(ms->rwlock);
> > diff --git a/lib/packets.h b/lib/packets.h
> > index b146a50..c64240c 100644
> > --- a/lib/packets.h
> > +++ b/lib/packets.h
> > @@ -533,12 +533,40 @@ struct igmp_header {
> > };
> > BUILD_ASSERT_DECL(IGMP_HEADER_LEN == sizeof(struct igmp_header));
> >
> > +#define IGMPV3_HEADER_LEN 8
> > +OVS_PACKED(
> > +struct igmpv3_header {
> > + uint8_t type;
> > + uint8_t rsvr1;
> > + ovs_be16 csum;
> > + ovs_be16 rsvr2;
> > + ovs_be16 ngrp;
> > +});
> > +BUILD_ASSERT_DECL(IGMPV3_HEADER_LEN == sizeof(struct igmpv3_header));
> > +
> > +#define IGMPV3_RECORD_LEN 8
> > +OVS_PACKED(
> > +struct igmpv3_record {
> > + uint8_t type;
> > + uint8_t aux_len;
> > + ovs_be16 nsrcs;
> > + ovs_be32 maddr;
> > +});
> > +BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> > +
> > #define IGMP_HOST_MEMBERSHIP_QUERY 0x11 /* From RFC1112 */
> > #define IGMP_HOST_MEMBERSHIP_REPORT 0x12 /* Ditto */
> > #define IGMPV2_HOST_MEMBERSHIP_REPORT 0x16 /* V2 version of 0x12 */
> > #define IGMP_HOST_LEAVE_MESSAGE 0x17
> > #define IGMPV3_HOST_MEMBERSHIP_REPORT 0x22 /* V3 version of 0x12 */
> >
> > +#define IGMPV3_MODE_IS_INCLUDE 1
> > +#define IGMPV3_MODE_IS_EXCLUDE 2
> > +#define IGMPV3_CHANGE_TO_INCLUDE_MODE 3
> > +#define IGMPV3_CHANGE_TO_EXCLUDE_MODE 4
> > +#define IGMPV3_ALLOW_NEW_SOURCES 5
> > +#define IGMPV3_BLOCK_OLD_SOURCES 6
> > +
> > #define SCTP_HEADER_LEN 12
> > struct sctp_header {
> > ovs_be16 sctp_src;
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 71b8bef..73d1b03 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -1994,10 +1994,12 @@ update_mcast_snooping_table__(const struct xbridge
> > *xbridge,
> > const struct flow *flow,
> > struct mcast_snooping *ms,
> > ovs_be32 ip4, int vlan,
> > - struct xbundle *in_xbundle)
> > + struct xbundle *in_xbundle,
> > + const struct dp_packet *packet)
> > OVS_REQ_WRLOCK(ms->rwlock)
> > {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(60, 30);
> > + int count;
> >
> > switch (ntohs(flow->tp_src)) {
> > case IGMP_HOST_MEMBERSHIP_REPORT:
> > @@ -2024,6 +2026,14 @@ update_mcast_snooping_table__(const struct xbridge
> > *xbridge,
> > in_xbundle->name, vlan);
> > }
> > break;
> > + case IGMPV3_HOST_MEMBERSHIP_REPORT:
> > + if ((count = mcast_snooping_add_report(ms, packet, vlan,
> > + in_xbundle->ofbundle))) {
> > + VLOG_DBG_RL(&rl, "bridge %s: multicast snooping processed %d "
> > + "addresses on port %s in VLAN %d",
> > + xbridge->name, count, in_xbundle->name, vlan);
> > + }
> > + break;
> > }
> > }
> >
> > @@ -2032,7 +2042,8 @@ update_mcast_snooping_table__(const struct xbridge
> > *xbridge,
> > static void
> > update_mcast_snooping_table(const struct xbridge *xbridge,
> > const struct flow *flow, int vlan,
> > - struct xbundle *in_xbundle)
> > + struct xbundle *in_xbundle,
> > + const struct dp_packet *packet)
> > {
> > struct mcast_snooping *ms = xbridge->ms;
> > struct xlate_cfg *xcfg;
> > @@ -2057,7 +2068,7 @@ update_mcast_snooping_table(const struct xbridge
> > *xbridge,
> >
> > if (!mcast_xbundle || mcast_xbundle != in_xbundle) {
> > update_mcast_snooping_table__(xbridge, flow, ms,
> > flow->igmp_group_ip4,
> > - vlan, in_xbundle);
> > + vlan, in_xbundle, packet);
> > }
> > ovs_rwlock_unlock(&ms->rwlock);
> > }
> > @@ -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.
Cascardo.
>
> > + }
> > }
> >
> > if (mcast_snooping_is_membership(flow->tp_src)) {
> > --
> > 2.4.2
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
