On Mon, Aug 5, 2013 at 12:25 PM, Ethan Jackson <et...@nicira.com> wrote:
> This looks good to me. My only comment is that I'd prefer we store
> BFD_ETH_DST as an array of uint8_t in packets.h like we do for
> eth_addr_stp and eth_addr_lacp.
>
> Okay. I made the change and applied it to master and 1.12
> Acked-by: Ethan Jackson <et...@nicira.com>
>
>
> On Mon, Aug 5, 2013 at 12:47 AM, Gurucharan Shetty <shet...@nicira.com>
> wrote:
> > The current situation is that whenever any packet enters the
> > userspace, bfd_should_process_flow() looks at the UDP destination
> > port to figure out whether that is a BFD packet. This means that
> > UDP destination port cannot be wildcarded for all the other flows
> > too.
> >
> > To optimize BFD for megaflows, we introduce a new
> > 'bfd:bfd_dst_mac' field in the database. Whenever this field is set
> > by a controller, it is assumed that all the BFD packets to/from
> > this interface will have the destination mac address set as the one
> > specified in the bfd:bfd_dst_mac field. If this field is set, we
> > first look at the destination mac address of a packet and if it
> > does not match the mac address set in bfd:bfd_dst_mac, we do not
> > process that packet as bfd. If the field does match, we go ahead
> > and look at the UDP destination port too.
> >
> > Also, change the default BFD destination mac address to
> > "00:23:20:00:00:01".
> >
> > Feature #18850.
> > Signed-off-by: Gurucharan Shetty <gshe...@nicira.com>
> > ---
> > lib/bfd.c | 27 +++++++++++++++++++++++++--
> > lib/bfd.h | 1 +
> > vswitchd/vswitch.xml | 9 +++++++++
> > 3 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/bfd.c b/lib/bfd.c
> > index f2d4b9c..668c0a1 100644
> > --- a/lib/bfd.c
> > +++ b/lib/bfd.c
> > @@ -161,6 +161,9 @@ struct bfd {
> >
> > uint32_t rmt_disc; /* bfd.RemoteDiscr. */
> >
> > + uint8_t eth_dst[ETH_ADDR_LEN];/* Ethernet destination address. */
> > + bool eth_dst_set; /* 'eth_dst' set through database. */
> > +
> > uint16_t udp_src; /* UDP source port. */
> >
> > /* All timers in milliseconds. */
> > @@ -259,6 +262,8 @@ bfd_configure(struct bfd *bfd, const char *name,
> const struct smap *cfg)
> >
> > long long int min_tx, min_rx;
> > bool cpath_down;
> > + const char *hwaddr;
> > + uint8_t ea[ETH_ADDR_LEN];
> >
> > if (ovsthread_once_start(&once)) {
> > unixctl_command_register("bfd/show", "[interface]", 0, 1,
> > @@ -296,6 +301,9 @@ bfd_configure(struct bfd *bfd, const char *name,
> const struct smap *cfg)
> > bfd->udp_src = (bfd->udp_src % 16384) + 49152;
> >
> > bfd_set_state(bfd, STATE_DOWN, DIAG_NONE);
> > +
> > + eth_addr_from_string(BFD_ETH_DST, ea);
> > + memcpy(bfd->eth_dst, ea, ETH_ADDR_LEN);
> > }
> >
> > atomic_store(&bfd->check_tnl_key,
> > @@ -330,6 +338,17 @@ bfd_configure(struct bfd *bfd, const char *name,
> const struct smap *cfg)
> > }
> > bfd_poll(bfd);
> > }
> > +
> > + hwaddr = smap_get(cfg, "bfd_dst_mac");
> > + if (hwaddr && eth_addr_from_string(hwaddr, ea) &&
> !eth_addr_is_zero(ea)) {
> > + memcpy(bfd->eth_dst, ea, ETH_ADDR_LEN);
> > + bfd->eth_dst_set = true;
> > + } else if (bfd->eth_dst_set) {
> > + eth_addr_from_string(BFD_ETH_DST, ea);
> > + memcpy(bfd->eth_dst, ea, ETH_ADDR_LEN);
> > + bfd->eth_dst_set = false;
> > + }
> > +
> > ovs_mutex_unlock(&mutex);
> > return bfd;
> > }
> > @@ -430,8 +449,8 @@ bfd_put_packet(struct bfd *bfd, struct ofpbuf *p,
> >
> > ofpbuf_reserve(p, 2); /* Properly align after the ethernet header.
> */
> > eth = ofpbuf_put_uninit(p, sizeof *eth);
> > - memcpy(eth->eth_dst, eth_addr_broadcast, ETH_ADDR_LEN);
> > memcpy(eth->eth_src, eth_src, ETH_ADDR_LEN);
> > + memcpy(eth->eth_dst, bfd->eth_dst, ETH_ADDR_LEN);
> > eth->eth_type = htons(ETH_TYPE_IP);
> >
> > ip = ofpbuf_put_zeros(p, sizeof *ip);
> > @@ -483,6 +502,10 @@ bfd_should_process_flow(const struct bfd *bfd,
> const struct flow *flow,
> > struct flow_wildcards *wc)
> > {
> > bool check_tnl_key;
> > + memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> > + if (bfd->eth_dst_set && memcmp(bfd->eth_dst, flow->dl_dst,
> ETH_ADDR_LEN)) {
> > + return false;
> > + }
> >
> > memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
> > memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
> > @@ -493,7 +516,7 @@ bfd_should_process_flow(const struct bfd *bfd, const
> struct flow *flow,
> > }
> > return (flow->dl_type == htons(ETH_TYPE_IP)
> > && flow->nw_proto == IPPROTO_UDP
> > - && flow->tp_dst == htons(3784)
> > + && flow->tp_dst == htons(BFD_DEST_PORT)
> > && (check_tnl_key || flow->tunnel.tun_id == htonll(0)));
> > }
> >
> > diff --git a/lib/bfd.h b/lib/bfd.h
> > index 67d012e..cf6a46f 100644
> > --- a/lib/bfd.h
> > +++ b/lib/bfd.h
> > @@ -16,6 +16,7 @@
> > #define BFD_H 1
> >
> > #define BFD_PACKET_LEN 24
> > +#define BFD_ETH_DST "00:23:20:00:00:01"
> > #define BFD_DEST_PORT 3784
> >
> > #include <stdbool.h>
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 957b02c..b89d58c 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -1896,6 +1896,15 @@
> > <code>false</code>.
> > </column>
> >
> > + <column name="bfd" key="bfd_dst_mac">
> > + An Ethernet address in the form
> > +
>
> <var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>
> > + to set the destination mac address of the bfd packet. If this
> > + field is set, it is assumed that all the bfd packets destined
> to this
> > + interface also has the same destination mac address. If not
> set, a
> > + default value of <code>00:23:20:00:00:01</code> is used.
> > + </column>
> > +
> > <column name="bfd_status" key="state"
> > type='{"type": "string",
> > "enum": ["set", ["admin_down", "down", "init", "up"]]}'>
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
