[ovs-dev] [nxm 41/42] Add support for matching Ethernet multicast frames.

Ben Pfaff blp at nicira.com
Thu Nov 11 18:51:18 UTC 2010


On Wed, Nov 10, 2010 at 11:30:35PM -0800, Justin Pettit wrote:
> On Oct 28, 2010, at 10:28 AM, Ben Pfaff wrote:
> 
> > @@ -291,6 +291,9 @@ flow_from_match(const struct ofp_match *match, int flow_format,
> >             flow->tun_id = htonl(ntohll(cookie) >> 32);
> >         }
> >     }
> > +    if (wildcards & OFPFW_DL_DST) {
> > +        wildcards |= FWW_ETH_MCAST;
> > +    }
> 
> It may be nice to point out that the OpenFlow 1.0 ofp_match structure
> doesn't have a concept of masking Ethernet destination addresses, but
> our internal way of supporting it requires us to patch these places
> up.

OK, done.

> > +/* Set to 1 if bit 0 (the multicast bit) of the flow's dl_dst is wildcarded.
> > + *
> > + * (We reinterpret OFPFW_DL_DST as excluding bit 0.) */
> 
> I know it's probably obvious, but I think it would be clearer to
> indicate that (FWW_ETH_MCAST | OFPFW_DL_DST) needs to be set to
> wildcard all destination MAC addresses.

OK.

> > +#define FWW_ETH_MCAST (1u << 30)
> 
> I feel like we're on our way to a stack/hash collision.  :-)

Yes, this becomes increasingly unmanageable as we add more fields, so in
the "generic tci masks" series that I posted yesterday I cleanly
separated the internal OVS wildcards from the OpenFlow 1.0 wildcards.

> > +static void
> > +nxm_put_eth_dst(struct ofpbuf *b,
> > +                uint32_t wc, const uint8_t value[ETH_ADDR_LEN])
> > +{
> > +    switch (wc & (OFPFW_DL_DST | FWW_ETH_MCAST)) {
> > ...
> > +    case 0:
> > +        nxm_put_header(b, NXM_OF_ETH_DST);
> > +        ofpbuf_put(b, value, ETH_ADDR_LEN);
> > +        break;
> 
> The function nxm_put_eth() does what's in that case already.  After
> this, the only caller of that function is for setting the source
> Ethernet address.  It would be nice to either use that function here
> or retire it, since there's not much there.

OK, I used it here.

> > --- a/tests/ovs-ofctl.at
> > +++ b/tests/ovs-ofctl.at
> > @@ -36,6 +36,10 @@ NXM_OF_IN_PORT(fffe)
> > 
> > # eth dst
> > NXM_OF_ETH_DST(0002e30f80a4)
> > +NXM_OF_ETH_DST_W(010000000000/010000000000)
> > +NXM_OF_ETH_DST_W(000000000000/010000000000)
> > +NXM_OF_ETH_DST_W(0002e30f80a4/ffffffffffff)
> > +NXM_OF_ETH_DST_W(0002e30f80a4/feffffffffff)
> > 
> > # eth src
> > NXM_OF_ETH_SRC(020898456ddb)
> > @@ -137,6 +141,10 @@ NXM_OF_IN_PORT(fffe)
> > 
> > # eth dst
> > NXM_OF_ETH_DST(0002e30f80a4)
> > +NXM_OF_ETH_DST_W(010000000000/010000000000)
> > +NXM_OF_ETH_DST_W(000000000000/010000000000)
> > +NXM_OF_ETH_DST(0002e30f80a4)
> > +NXM_OF_ETH_DST_W(0002e30f80a4/feffffffffff)
> > 
> > # eth src
> > NXM_OF_ETH_SRC(020898456ddb)
> 
> Do you think it's worth having some tests that check that masked
> values get properly set to zero in the resulting output?  It seems
> like it would be good to enforce some of the invariants defined in
> flow.h.

That's a good idea, thanks.  I've added some.




More information about the dev mailing list