Thanks for getting this together. Comments below.
Major stuff:
When anything changes about the configuration of the decay feature,
the decay_detect_time needs to be reset and we should switch back to
the original min_rx. To be safe, I'd also reset it when the min_rx
changes.
I wouldn't bother with a default decay_min_rx value. If someone fails
to set the appropriate value (something >= min_rx) I would simply warn
and disable the feature.
Changing the min_rx is actually quite a bit more complicated than what
you've implemented here. It's insufficient to just set the value,
instead we have to tell the other end of the BFD session we're going
to do so, before we actually pull the trigger. We do this by by
initiating a poll sequence. And setting the bfd->poll_min_rx the the
new value. I think we can pretty cleanly achieve the correct behavior
by (1) initiating a poll in bfd_run() if we should decay, but the
min_rx does not equal the decay_min_rx. And (2) in bfd_poll() setting
poll_min_rx to decay_min_rx if decay is enabled and we should decay.
In bfd_run() the final if statement should run whether or not decay is
enabled. It will have to become a bit more sophisticated, but
assuming my suggestion in the previous patch works, it should be
straight forward.
With the above two comments implemented the bfd_decay function should
become quite a bit simpler. For example, we shouldn't be manually
mucking with the detect_time. The current min_rx changing code should
handle that.
The port_modified() function in ofproto-dpif.c needs to update the
"struct bfd"s netdev like it does for cfm. Otherwise you'll run into
some subtle bugs.
Minor stuff:
The subject line of the commit message should have a period. It
should also change "Implements" to "Implement".
Please change the commit message to refer to "interfaces" instead of
"tunnels". BFD can run on anything.
This prexisted your patch, but in bfd_configure() the 'cfg' argument
should be on the previous line.
Instead of directly holding a pointer to 'netdev', please take a
reference of it using netdev_ref(). See ofproto-dpif-xlate for an
example.
We can probably get rid of decay_enabled flag altogher and just use
the decay_detect_time. If it's 0, we'll say the feature is disabled.
In bfd_rx_packets() why would bfd->netdev be null?
In bfd.h instead of including netdev.h you can simply declare struct
netdev. cfm.h has an example.
Ethan
On Thu, Jul 11, 2013 at 6:39 PM, Alex Wang <al...@nicira.com> wrote:
> When there is no incoming data traffic in the tunnel for a period,
> BFD decay allows the bfd session to increase the min_rx. This is
> helpful in that usually some tunnels are rarely used. And cpu
> consumption can be reduced by processing fewer bfd control packets.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
> ---
> lib/bfd.c | 89
> ++++++++++++++++++++++++++++++++++++++++++++++--
> lib/bfd.h | 5 ++-
> ofproto/ofproto-dpif.c | 3 +-
> vswitchd/vswitch.xml | 17 +++++++++
> 4 files changed, 109 insertions(+), 5 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index aa1a3f7..0d4805a 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -152,6 +152,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 +185,11 @@ struct bfd {
>
> int ref_cnt;
> int forwarding_override; /* Manual override of 'forwarding' status.
> */
> +
> + /* BFD decay related variables. */
> + bool decay_enabled; /* Flag that enables bfd decay. */
> + int decay_min_rx;
> + long long int decay_detect_time;
> };
>
> static bool bfd_in_poll(const struct bfd *);
> @@ -191,6 +199,9 @@ 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_min_rx_set_default(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);
> @@ -243,7 +254,8 @@ bfd_get_status(const struct bfd *bfd, struct smap *smap)
> * Also returns NULL if cfg is NULL. */
> struct bfd *
> bfd_configure(struct bfd *bfd, const char *name,
> - const struct smap *cfg)
> + const struct smap *cfg,
> + struct netdev *netdev)
> {
> static uint16_t udp_src = 0;
> static bool init = false;
> @@ -276,6 +288,9 @@ bfd_configure(struct bfd *bfd, const char *name,
> bfd->min_tx = 1000;
> bfd->mult = 3;
> bfd->ref_cnt = 1;
> + bfd->netdev = 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
> @@ -309,6 +324,15 @@ bfd_configure(struct bfd *bfd, const char *name,
> bfd_poll(bfd);
> }
>
> + bfd->decay_enabled = smap_get_bool(cfg, "decay_enable", false);
> + bfd->decay_min_rx = smap_get_int(cfg, "decay_min_rx", -1);
> + /* Set the default decay_min_rx if decay is enabled and decay_min_rx
> + is not specified. */
> + if (bfd->decay_enabled && bfd->decay_min_rx < 0) {
> + bfd_decay_min_rx_set_default(bfd);
> + bfd_poll(bfd);
> + }
> +
> cpath_down = smap_get_bool(cfg, "cpath_down", false);
> if (bfd->cpath_down != cpath_down) {
> bfd->cpath_down = cpath_down;
> @@ -360,11 +384,19 @@ 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->min_tx != bfd->cfg_min_tx || bfd->min_rx != bfd->cfg_min_rx) {
> + if (bfd->state == STATE_UP && bfd->decay_enabled
> + && now >= bfd->decay_detect_time) {
> + bfd_decay(bfd);
> + }
> +
> + if (!bfd->decay_enabled &&
> + (bfd->min_tx != bfd->cfg_min_tx || bfd->min_rx != bfd->cfg_min_rx)) {
> bfd_poll(bfd);
> }
> }
> @@ -804,6 +836,57 @@ 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 (bfd->netdev && !netdev_get_stats(bfd->netdev, &stats)) {
> + return stats.rx_packets;
> + } else {
> + return 0;
> + }
> +}
> +
> +static void
> +bfd_decay_min_rx_set_default(struct bfd *bfd)
> +{
> +
> + bfd->decay_min_rx = bfd->mult * bfd->mult * MAX(bfd->cfg_min_rx,
> + bfd->rmt_min_tx);
> + /* Always restore the min_rx. */
> + bfd->min_rx = bfd->cfg_min_rx;
> +}
> +
> +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 + 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;
> + /* Increase the detect time to avoid
> + * "Control Detection Time Expired". */
> + bfd->detect_time = bfd->decay_detect_time;
> + }
> + } else {
> + /* Restore the min_rx. */
> + if (bfd->min_rx != bfd->cfg_min_rx) {
> + bfd->min_rx = bfd->cfg_min_rx;
> + /* Increase the detect time to avoid
> + * "Control Detection Time Expired". */
> + bfd->detect_time = bfd->decay_detect_time;
> + }
> + }
> +}
> +
> static uint32_t
> generate_discriminator(void)
> {
> diff --git a/lib/bfd.h b/lib/bfd.h
> index ab854d8..a9b6d43 100644
> --- a/lib/bfd.h
> +++ b/lib/bfd.h
> @@ -21,6 +21,8 @@
> #include <stdbool.h>
> #include <inttypes.h>
>
> +#include "netdev.h"
> +
> struct bfd;
> struct flow;
> struct flow_wildcards;
> @@ -39,7 +41,8 @@ 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 *);
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 67e6c7a..d475379 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1882,7 +1882,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..f888f4b 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1880,6 +1880,23 @@
> specified. Defaults to <code>100</code>.
> </column>
>
> + <column name="bfd" key="decay_enable" type='{"type": "boolean"}'>
> + Allow the BFD session decay the <code>min_rx</code> to <code>
> + decay_min_rx</code> when there is no obvious incoming data traffic
> + in the tunnel. This is helpful in that usually some tunnels are
> + rarely used. And decaying the min_rx can reduce the number of
> + BFD control packets processed, as well as the cpu consumption.
> + </column>
> +
> + <column name="bfd" key="decay_min_rx" type='{"type": "integer"}'>
> + When <code>decay_enable</code> is true, the BFD session will wait
> + <code>decay_min_rx</code> amount of time. If there is no obvious
> + incoming data traffic in the tunnel during this time interal,
> + BFD session will set <code>min_rx</code> to <code>decay_min_rx
> + </code>.
> + </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
>
> _______________________________________________
> 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
