Thanks Ethan, for the comments,
On Fri, Jul 12, 2013 at 2:15 PM, Ethan Jackson <et...@nicira.com> wrote:
> 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.
This makes sense. Modifying the bfd_poll() will make life easier for sure.
> 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.
>
Thanks for this reminder!
> 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.
>
Thanks for pointing these out.
> 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.
Makes sense, I'll adjust accordingly
> In bfd_rx_packets() why would bfd->netdev be null?
>
Yes, it is an unnecessary check.
> In bfd.h instead of including netdev.h you can simply declare struct
> netdev. cfm.h has an example.
>
Thanks pointing this out.
On Fri, Jul 12, 2013 at 2:15 PM, Ethan Jackson <et...@nicira.com> wrote:
> 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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
