[ovs-dev] [tunnels2 4/5] datapath: Add multicast tunnel support.

Ben Pfaff blp at nicira.com
Mon Oct 24 12:28:35 PDT 2011


On Fri, Oct 21, 2011 at 06:04:29PM -0700, Jesse Gross wrote:
> On Fri, Oct 21, 2011 at 4:24 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Thu, Oct 20, 2011 at 05:11:33PM -0700, Jesse Gross wrote:
> >> On Mon, Oct 17, 2011 at 3:14 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> >> > index 8edff06..6fde389 100644
> >> > --- a/datapath/tunnel.c
> >> > +++ b/datapath/tunnel.c
> >> > ??int tnl_set_addr(struct vport *vport, const unsigned char *addr)
> >> > ??{
> >> > ?? ?? ?? ??struct tnl_vport *tnl_vport = tnl_vport_priv(vport);
> >> > - ?? ?? ?? struct tnl_mutable_config *mutable;
> >> > + ?? ?? ?? struct tnl_mutable_config *old_mutable, *mutable;
> >> >
> >> > - ?? ?? ?? mutable = kmemdup(rtnl_dereference(tnl_vport->mutable),
> >> > - ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? sizeof(struct tnl_mutable_config), GFP_KERNEL);
> >> > + ?? ?? ?? old_mutable = rtnl_dereference(tnl_vport->mutable);
> >> > + ?? ?? ?? mutable = kmemdup(old_mutable, sizeof(struct tnl_mutable_config), GFP_KERNEL);
> >> > ?? ?? ?? ??if (!mutable)
> >> > ?? ?? ?? ?? ?? ?? ?? ??return -ENOMEM;
> >> >
> >> > + ?? ?? ?? mutable->mlink = mutable->mlink;
> >>
> >> I think this line probably isn't going to do a whole lot...
> >
> > Oops. ??The one part that I don't test...
> >
> > Changed to:
> > ?? ?? ?? ??mutable->mlink = old_mutable->mlink;
> 
> Actually, I think this line is completely unnecessary because it
> follows kmemdup().

You're right, of course.  I must have tunnel vision (pun intended).
Deleted.

> >> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> >> > index d579b87..d09ade4 100644
> >> > --- a/vswitchd/vswitch.xml
> >> > +++ b/vswitchd/vswitch.xml
> >> > ?? ?? ?? <column name="options" key="local_ip">
> >> > - ?? ?? ?? ??Optional. ??The destination IP that received packets must
> >> > - ?? ?? ?? ??match. ??Default is to match all addresses.
> >> > + ?? ?? ?? ??Optional. ??The destination IP that received packets must match.
> >> > + ?? ?? ?? ??Default is to match all addresses. ??Must be omitted when <ref
> >> > + ?? ?? ?? ??column="options" key="remote_ip"/> is a multicast address.
> >> > ?? ?? ?? </column>
> >>
> >> I think we probably should do some validation on this - otherwise
> >> ports that specify a local address and a multicast address silently
> >> won't match.
> >
> > OK, how's this:
> 
> Looks good.

Thanks.

> >> Maybe we should also document that routing table changes for multicast
> >> tunnels require deleting and recreating them.
> >
> > It's an embarrassing limitation. ??For now:
> 
> OK, that's fine.

OK.  Looking at it again, I think that it overstates the case.  I
think that routing table changes will be effective *except* that the
multicast group won't be registered for receive on the new network
device.  I'm not quite sure how to word that, though, so maybe the
current text is OK anyway.

> >> I think in the gre_err() code path, we should ignore ICMP messages
> >> sent to multicast addresses.
> >
> > OK, like this?
> 
> Yes, looks good as well.
> 
> So other than that one line looks good:
> Acked-by: Jesse Gross <jesse at nicira.com>

Thanks.  I'll do a quick re-test and push it.



More information about the dev mailing list