[ovs-dev] [PATCH] bfd: Implement BFD decay.

Alex Wang alexw at nicira.com
Sat Jul 13 01:57:09 UTC 2013


Hey Ethan,

Sorry, I didn't add the version number. Should be [PATCH 1/2 V2]

One clarification:
"""
+    bfd->decay_detect_time = (bfd->decay_min_rx < 2000 ?
+                              2000 : bfd->decay_min_rx) + time_msec();
"""
The decay_detect_time should be greater than 2000, since
"facet_push_stats()"
is called every 2 seconds. If the "decay_detect_time" is less than 2 sec,
there can
be flips between decay and non-decay.

>From what I experimented on ejja and ejjb, this patch works fine. Also I
think I can
finish the bfd decay tests soon and check if  there is any hidden bugs.

Hope to hear comments from you.

Kind Regards,
Alex Wang,


On Fri, Jul 12, 2013 at 7:03 PM, Alex Wang <alexw at nicira.com> wrote:

> When there is no incoming data traffic at the interface for a period,
> BFD decay allows the bfd session to increase the min_rx. This is
> helpful in that some interfaces usually idle for long time. And cpu
> consumption can be reduced by processing fewer bfd control packets.
>
> Signed-off-by: Alex Wang <alexw at nicira.com>
> ---
>
> v1 -> v2:
> - remove bfd:decay_enable option, only use bfd:decay_min_rx.
> - add bfd_set_netdev() function.
> - reset decay_min_rx when itself or min_rx is reconfigured.
> - use bfd_poll() to update the decay changes.
> - refine the code as suggested by Ethan.
>
> ---
>  lib/bfd.c              |   97
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  lib/bfd.h              |    5 ++-
>  ofproto/ofproto-dpif.c |    7 +++-
>  vswitchd/vswitch.xml   |   10 +++++
>  4 files changed, 113 insertions(+), 6 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index aa1a3f7..e5b04ec 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -24,6 +24,7 @@
>  #include "hash.h"
>  #include "hmap.h"
>  #include "list.h"
> +#include "netdev.h"
>  #include "netlink.h"
>  #include "odp-util.h"
>  #include "ofpbuf.h"
> @@ -152,6 +153,9 @@ struct bfd {
>      bool cpath_down;              /* Concatenated Path Down. */
>      uint8_t mult;                 /* bfd.DetectMult. */
>
> +    struct netdev *netdev;
> +    uint64_t rx_packets;          /* Packets received by 'netdev'. */
> +
>      enum state state;             /* bfd.SessionState. */
>      enum state rmt_state;         /* bfd.RemoteSessionState. */
>
> @@ -182,6 +186,10 @@ struct bfd {
>
>      int ref_cnt;
>      int forwarding_override;      /* Manual override of 'forwarding'
> status. */
> +
> +    /* BFD decay related variables. */
> +    int decay_min_rx;
> +    long long int decay_detect_time; /* Decay detection time. */
>  };
>
>  static bool bfd_in_poll(const struct bfd *);
> @@ -191,6 +199,8 @@ static const char *bfd_state_str(enum state);
>  static long long int bfd_min_tx(const struct bfd *);
>  static long long int bfd_tx_interval(const struct bfd *);
>  static long long int bfd_rx_interval(const struct bfd *);
> +static uint64_t bfd_rx_packets(const struct bfd *);
> +static void bfd_decay(struct bfd *);
>  static void bfd_set_next_tx(struct bfd *);
>  static void bfd_set_state(struct bfd *, enum state, enum diag);
>  static uint32_t generate_discriminator(void);
> @@ -242,12 +252,13 @@ bfd_get_status(const struct bfd *bfd, struct smap
> *smap)
>   * handle for the session, or NULL if BFD is not enabled according to
> 'cfg'.
>   * Also returns NULL if cfg is NULL. */
>  struct bfd *
> -bfd_configure(struct bfd *bfd, const char *name,
> -              const struct smap *cfg)
> +bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
> +              struct netdev *netdev)
>  {
>      static uint16_t udp_src = 0;
>      static bool init = false;
>
> +    int decay_min_rx;
>      long long int min_tx, min_rx;
>      bool cpath_down;
>
> @@ -276,6 +287,9 @@ bfd_configure(struct bfd *bfd, const char *name,
>          bfd->min_tx = 1000;
>          bfd->mult = 3;
>          bfd->ref_cnt = 1;
> +        bfd->netdev = netdev_ref(netdev);
> +        bfd->decay_detect_time = 0;
> +        bfd->rx_packets = bfd_rx_packets(bfd);
>
>          /* RFC 5881 section 4
>           * The source port MUST be in the range 49152 through 65535.  The
> same
> @@ -295,6 +309,7 @@ bfd_configure(struct bfd *bfd, const char *name,
>              || (!bfd_in_poll(bfd) && bfd->cfg_min_tx < bfd->min_tx)) {
>              bfd->min_tx = bfd->cfg_min_tx;
>          }
> +        bfd->decay_min_rx = 0;
>          bfd_poll(bfd);
>      }
>
> @@ -309,6 +324,19 @@ bfd_configure(struct bfd *bfd, const char *name,
>          bfd_poll(bfd);
>      }
>
> +    decay_min_rx = smap_get_int(cfg, "decay_min_rx", 0);
> +    if (bfd->decay_min_rx != decay_min_rx ) {
> +        if (decay_min_rx > 0 && decay_min_rx < bfd->cfg_min_rx) {
> +            VLOG_WARN("%s: decay_min_rx cannot be less than %lld ms",
> +                      bfd->name, bfd->cfg_min_rx);
> +            bfd->decay_min_rx = 0;
> +        } else {
> +            bfd->decay_min_rx = decay_min_rx;
> +        }
> +        bfd->min_rx = bfd->cfg_min_rx;
> +        bfd_poll(bfd);
> +    }
> +
>      cpath_down = smap_get_bool(cfg, "cpath_down", false);
>      if (bfd->cpath_down != cpath_down) {
>          bfd->cpath_down = cpath_down;
> @@ -340,6 +368,7 @@ bfd_unref(struct bfd *bfd)
>              hmap_remove(&all_bfds, &bfd->node);
>              free(bfd->name);
>              free(bfd);
> +            netdev_close(bfd->netdev);
>          }
>      }
>  }
> @@ -360,11 +389,23 @@ bfd_wait(const struct bfd *bfd)
>  void
>  bfd_run(struct bfd *bfd)
>  {
> -    if (bfd->state > STATE_DOWN && time_msec() >= bfd->detect_time) {
> +    long long int now = time_msec();
> +
> +    if (bfd->state > STATE_DOWN && now >= bfd->detect_time) {
>          bfd_set_state(bfd, STATE_DOWN, DIAG_EXPIRED);
>      }
>
> +    if (bfd->state == STATE_UP && bfd->decay_min_rx > 0
> +        && now >= bfd->decay_detect_time) {
> +        bfd_decay(bfd);
> +    }
> +
>      if (bfd->min_tx != bfd->cfg_min_tx || bfd->min_rx != bfd->cfg_min_rx)
> {
> +        /* Do not poll if already decayed to decay_min_rx. */
> +        if (bfd->state == STATE_UP && bfd->poll_min_rx ==
> bfd->decay_min_rx
> +            && bfd->cfg_min_tx == bfd->min_tx) {
> +            return;
> +        }
>          bfd_poll(bfd);
>      }
>  }
> @@ -616,6 +657,17 @@ bfd_process_packet(struct bfd *bfd, const struct flow
> *flow,
>      }
>      /* XXX: RFC 5880 Section 6.8.6 Demand mode related calculations here.
> */
>  }
> +
> +/* Must be called when the netdev owned by 'bfd' should change. */
> +void
> +bfd_set_netdev(struct bfd *bfd, const struct netdev *netdev)
> +{
> +    if (bfd->netdev != netdev) {
> +        netdev_close(bfd->netdev);
> +        bfd->netdev = netdev_ref(netdev);
> +    }
> +}
> +
>
>  /* Helpers. */
>  static bool
> @@ -630,7 +682,8 @@ bfd_poll(struct bfd *bfd)
>      if (bfd->state > STATE_DOWN && !bfd_in_poll(bfd)
>          && !(bfd->flags & FLAG_FINAL)) {
>          bfd->poll_min_tx = bfd->cfg_min_tx;
> -        bfd->poll_min_rx = bfd->cfg_min_rx;
> +        bfd->poll_min_rx = bfd->min_rx == bfd->decay_min_rx
> +                           ? bfd->decay_min_rx : bfd->cfg_min_rx;
>          bfd->flags |= FLAG_POLL;
>          bfd->next_tx = 0;
>          VLOG_INFO_RL(&rl, "%s: Initiating poll sequence", bfd->name);
> @@ -804,6 +857,42 @@ bfd_set_state(struct bfd *bfd, enum state state, enum
> diag diag)
>      }
>  }
>
> +static uint64_t
> +bfd_rx_packets(const struct bfd *bfd)
> +{
> +    struct netdev_stats stats;
> +
> +    if (!netdev_get_stats(bfd->netdev, &stats)) {
> +        return stats.rx_packets;
> +    } else {
> +        return 0;
> +    }
> +}
> +
> +static void
> +bfd_decay(struct bfd *bfd)
> +{
> +    uint64_t rx_packets = bfd_rx_packets(bfd);
> +    int64_t diff;
> +
> +    diff = rx_packets - bfd->rx_packets;
> +    bfd->rx_packets = rx_packets;
> +    bfd->decay_detect_time = (bfd->decay_min_rx < 2000 ?
> +                              2000 : bfd->decay_min_rx) + time_msec();
> +
> +    if (diff <= (bfd->decay_min_rx / bfd->min_rx + 5)) {
> +        /* Decay when there is no obvious data traffic. */
> +        if (bfd->min_rx != bfd->decay_min_rx) {
> +            bfd->min_rx = bfd->decay_min_rx;
> +        }
> +    } else {
> +        /* Restore the min_rx. */
> +        if (bfd->min_rx != bfd->cfg_min_rx) {
> +            bfd->min_rx = bfd->cfg_min_rx;
> +        }
> +    }
> +}
> +
>  static uint32_t
>  generate_discriminator(void)
>  {
> diff --git a/lib/bfd.h b/lib/bfd.h
> index ab854d8..db652e1 100644
> --- a/lib/bfd.h
> +++ b/lib/bfd.h
> @@ -24,6 +24,7 @@
>  struct bfd;
>  struct flow;
>  struct flow_wildcards;
> +struct netdev;
>  struct ofpbuf;
>  struct smap;
>
> @@ -39,11 +40,13 @@ void bfd_process_packet(struct bfd *, const struct
> flow *,
>                          const struct ofpbuf *);
>
>  struct bfd *bfd_configure(struct bfd *, const char *name,
> -                          const struct smap *smap);
> +                          const struct smap *smap,
> +                          struct netdev *netdev);
>  struct bfd *bfd_ref(const struct bfd *);
>  void bfd_unref(struct bfd *);
>
>  bool bfd_forwarding(const struct bfd *);
>  void bfd_get_status(const struct bfd *, struct smap *);
> +void bfd_set_netdev(struct bfd *, const struct netdev *);
>
>  #endif /* bfd.h */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 67e6c7a..63e4299 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1748,6 +1748,10 @@ port_modified(struct ofport *port_)
>          cfm_set_netdev(port->cfm, port->up.netdev);
>      }
>
> +    if (port->bfd) {
> +        bfd_set_netdev(port->bfd, port->up.netdev);
> +    }
> +
>      if (port->is_tunnel && tnl_port_reconfigure(port, port->up.netdev,
>                                                  port->odp_port)) {
>          ofproto_dpif_cast(port->up.ofproto)->backer->need_revalidate =
> @@ -1882,7 +1886,8 @@ set_bfd(struct ofport *ofport_, const struct smap
> *cfg)
>      struct bfd *old;
>
>      old = ofport->bfd;
> -    ofport->bfd = bfd_configure(old, netdev_get_name(ofport->up.netdev),
> cfg);
> +    ofport->bfd = bfd_configure(old, netdev_get_name(ofport->up.netdev),
> +                                cfg, ofport->up.netdev);
>      if (ofport->bfd != old) {
>          ofproto->backer->need_revalidate = REV_RECONFIGURE;
>      }
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 3385912..7d2d601 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1880,6 +1880,16 @@
>            specified.  Defaults to <code>100</code>.
>        </column>
>
> +      <column name="bfd" key="decay_min_rx" type='{"type": "integer"}'>
> +          <code>decay_min_rx</code> is used to set the
> <code>min_rx</code>,
> +          when there is no obvious incoming data traffic at the interface.
> +          It cannot be less than the <code>min_rx</code>. The decay
> feature
> +          is disable by setting the <code>decay_min_rx</code> to 0. And
> the
> +          feature is reset everytime the itself or <code>min_rx</code> is
> +          reconfigured.
> +      </column>
> +
> +
>        <column name="bfd" key="cpath_down" type='{"type": "boolean"}'>
>            Concatenated path down may be used when the local system should
> not
>            have traffic forwarded to it for some reason other than a
> connectivty
> --
> 1.7.9.5
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130712/bbdf6f58/attachment-0003.html>


More information about the dev mailing list