Thanks for writing this up, it's pretty close.
In bfd_check_rx() the assertion makes me uncomfortable. I'd prefer we
warn and reset bfd->rx_packets instead. Im worried that we could hit
the assertion transiently during a configuration change.
I'd prefer we don't use the bfd->detect_time for this. It has a very
specific meaning which I'd rather not overload. Let's add a new timer
bfd->rx_detect_time used specifically for this feature?
Could the bfd->state <= DOWN && bfd->forwarding_if_rx check be moved
into the bfd_check_rx() function?
Actually come to think of it, we shouldn't need to check if bfd->state
<=DOWN at all. Consider a tunnel which is up and happily receiving
data traffic. If we suddenly have congestion and lose our BFD
packets, we'd have to transition into a DOWN state before this
forwarding_if_rx feature would kick in.
A unit test would be nice.
Ethan
On Thu, Jul 11, 2013 at 6:39 PM, Alex Wang <al...@nicira.com> wrote:
> This commit adds a new boolean option "forwarding_if_rx" to bfd.
>
> When forwarding_if_rx is true and BFD state is down, the forwarding
> field in "ovs-appctl bfd/show" output will still be true as long as
> there are incoming packets received. This is for indicating the
> tunnel liveness when the tunnel is congested and consecutive BFD
> control packets are lost. Or when BFD is enabled only on one side
> of the tunnel.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
> ---
> lib/bfd.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
> vswitchd/vswitch.xml | 8 ++++++++
> 2 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 0d4805a..a16a154 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -186,6 +186,13 @@ struct bfd {
> int ref_cnt;
> int forwarding_override; /* Manual override of 'forwarding' status.
> */
>
> + /* When forward_if_rx is true and bfd->state is STATE_DOWN, the
> + * bfd_forwarding() will return true as long as there are incoming
> + * packets received. Note, forwarding_override still has
> + * higher priority. */
> + bool forwarding_if_rx;
> + bool has_rx;
> +
> /* BFD decay related variables. */
> bool decay_enabled; /* Flag that enables bfd decay. */
> int decay_min_rx;
> @@ -202,6 +209,7 @@ 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_check_rx(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);
> @@ -226,10 +234,11 @@ bfd_forwarding(const struct bfd *bfd)
> return bfd->forwarding_override == 1;
> }
>
> - return bfd->state == STATE_UP
> - && bfd->rmt_diag != DIAG_PATH_DOWN
> - && bfd->rmt_diag != DIAG_CPATH_DOWN
> - && bfd->rmt_diag != DIAG_RCPATH_DOWN;
> + return (bfd->forwarding_if_rx && bfd->state <= STATE_DOWN
> + ? bfd->has_rx : bfd->state == STATE_UP)
> + && bfd->rmt_diag != DIAG_PATH_DOWN
> + && bfd->rmt_diag != DIAG_CPATH_DOWN
> + && bfd->rmt_diag != DIAG_RCPATH_DOWN;
> }
>
> /* Returns a 'smap' of key value pairs representing the status of 'bfd'
> @@ -261,7 +270,7 @@ bfd_configure(struct bfd *bfd, const char *name,
> static bool init = false;
>
> long long int min_tx, min_rx;
> - bool cpath_down;
> + bool cpath_down, forwarding_if_rx;
>
> if (!init) {
> unixctl_command_register("bfd/show", "[interface]", 0, 1,
> @@ -291,6 +300,8 @@ bfd_configure(struct bfd *bfd, const char *name,
> bfd->netdev = netdev;
> bfd->decay_detect_time = 0;
> bfd->rx_packets = bfd_rx_packets(bfd);
> + bfd->forwarding_if_rx = false;
> + bfd->has_rx = false;
>
> /* RFC 5881 section 4
> * The source port MUST be in the range 49152 through 65535. The
> same
> @@ -341,6 +352,14 @@ bfd_configure(struct bfd *bfd, const char *name,
> }
> bfd_poll(bfd);
> }
> +
> + forwarding_if_rx = smap_get_bool(cfg, "forwarding_if_rx", false);
> + if (bfd->forwarding_if_rx != forwarding_if_rx) {
> + bfd->forwarding_if_rx = forwarding_if_rx;
> + if (bfd->state <= STATE_DOWN && bfd->forwarding_if_rx) {
> + bfd_check_rx(bfd);
> + }
> + }
> return bfd;
> }
>
> @@ -390,6 +409,11 @@ bfd_run(struct bfd *bfd)
> bfd_set_state(bfd, STATE_DOWN, DIAG_EXPIRED);
> }
>
> + if (bfd->state <= STATE_DOWN && bfd->forwarding_if_rx
> + && now >= bfd->detect_time) {
> + bfd_check_rx(bfd);
> + }
> +
> if (bfd->state == STATE_UP && bfd->decay_enabled
> && now >= bfd->decay_detect_time) {
> bfd_decay(bfd);
> @@ -887,6 +911,20 @@ bfd_decay(struct bfd *bfd)
> }
> }
>
> +static void
> +bfd_check_rx(struct bfd *bfd)
> +{
> + uint64_t rx_packets = bfd_rx_packets(bfd);
> + int64_t diff;
> +
> + diff = rx_packets - bfd->rx_packets;
> + ovs_assert(diff >= 0);
> +
> + bfd->rx_packets = rx_packets;
> + bfd->has_rx = (diff > 0);
> + bfd->detect_time = bfd_rx_interval(bfd) * bfd->mult + time_msec();
> +}
> +
> static uint32_t
> generate_discriminator(void)
> {
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index f888f4b..8c1170d 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1896,6 +1896,14 @@
> </code>.
> </column>
>
> + <column name="bfd" key="forwarding_if_rx" type='{"type": "boolean"}'>
> + When <code>forwarding_if_rx</code> is true and BFD state is down,
> + the <code>forwarding</code> field in <code>"ovs-appctl bfd/show"
> + </code> output will still be true as long as there are incoming
> + packets received. This option is for indicating the tunnel liveness
> + when the tunnel becomes congested and consecutive BFD control
> + packets are lost.
> + </column>
>
> <column name="bfd" key="cpath_down" type='{"type": "boolean"}'>
> Concatenated path down may be used when the local system should not
> --
> 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
