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.
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
X-CudaMail-Whitelist-To: dev@openvswitch.org
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
