Re: [ovs-dev] [branch-1.11] ofproto-dpif: Fix memory leak in handle_flow_miss().
On Aug 19, 2013 11:35 PM, "Justin Pettit" wrote:
>
> Great catch. Also, very cool with sleuthing with your malloc histogram
(as discussed off-list).
Thanks.
Did you look around to make sure that I didn't somehow overlook a free call
somewhere? This patch "feels right" but obviously it needs to be more than
a feeling.
> Do you think it's worth mentioning in the comment describing
xlate_actions()?
Good idea.
> Acked-by: Justin Pettit
>
> --Justin
>
>
> On Aug 19, 2013, at 10:46 PM, Ben Pfaff wrote:
>
> > Every xlate_actions() needs a corresponding xlate_out_uninit(), but the
> > call in handle_flow_miss() lacked one. struct xlate_out has a built-in
> > 256-byte actions stub, so the bug only showed up for lots of actions.
> >
> > Bug #19198.
> > Reported-by: Ronald Lee
> > Signed-off-by: Ben Pfaff
> > ---
> > This probably needs to be ported to master, but this was reported as
> > an urgent bug against branch-1.11, so I've fixed it there first.
> >
> > ofproto/ofproto-dpif.c |2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 0f4c42c..dc89727 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -3898,10 +3898,12 @@ handle_flow_miss(struct flow_miss *miss, struct
flow_miss_op *ops,
> > if (miss->key_fitness == ODP_FIT_TOO_LITTLE
> > || !flow_miss_should_make_facet(miss, &xout.wc)) {
> > handle_flow_miss_without_facet(rule, &xout, miss, ops,
n_ops);
> > +xlate_out_uninit(&xout);
> > return;
> > }
> >
> > facet = facet_create(miss, rule, &xout, stats);
> > +xlate_out_uninit(&xout);
> > stats = NULL;
> > }
> > handle_flow_miss_with_facet(miss, facet, now, stats, ops, n_ops);
> > --
> > 1.7.10.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [branch-1.11] ofproto-dpif: Fix memory leak in handle_flow_miss().
On Aug 20, 2013, at 12:01 AM, Ben Pfaff wrote: > > On Aug 19, 2013 11:35 PM, "Justin Pettit" wrote: > > > > Great catch. Also, very cool with sleuthing with your malloc histogram (as > > discussed off-list). > > Thanks. > > Did you look around to make sure that I didn't somehow overlook a free call > somewhere? This patch "feels right" but obviously it needs to be more than a > feeling. I think it's correct. The only references of that xout instance are given to handle_flow_miss_without_facet() and facet_create(), which make local copies of the data and don't retain references to that instance. I also had gone through the code to see if we missed any other frees and didn't see any. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] bfd: Implement BFD decay.
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 may usually be idle for a long time.
And cpu consumption can be reduced by processing fewer bfd control
packets.
Signed-off-by: Alex Wang
---
lib/bfd.c | 134 ++-
lib/bfd.h |5 +-
ofproto/ofproto-dpif.c |7 +-
tests/bfd.at | 279 ++--
vswitchd/vswitch.xml | 10 ++
5 files changed, 419 insertions(+), 16 deletions(-)
diff --git a/lib/bfd.c b/lib/bfd.c
index 6f86f26..43ef759 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -28,6 +28,7 @@
#include "hash.h"
#include "hmap.h"
#include "list.h"
+#include "netdev.h"
#include "netlink.h"
#include "odp-util.h"
#include "ofpbuf.h"
@@ -151,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. */
@@ -186,6 +190,14 @@ struct bfd {
atomic_bool check_tnl_key;/* Verify tunnel key of inbound packets? */
atomic_int ref_cnt;
+
+/* BFD decay related variables. */
+bool in_decay;/* True when bfd is in decay. */
+int decay_min_rx; /* min_rx is set to decay_min_rx when */
+ /* in decay. */
+int decay_rx_ctrl;/* Count bfd packets received within decay */
+ /* detect interval. */
+long long int decay_detect_time; /* Decay detection time. */
};
static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
@@ -208,6 +220,9 @@ static void bfd_set_state(struct bfd *, enum state, enum
diag)
static uint32_t generate_discriminator(void) OVS_REQUIRES(mutex);
static void bfd_put_details(struct ds *, const struct bfd *)
OVS_REQUIRES(mutex);
+static uint64_t bfd_rx_packets(const struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_try_decay(struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_decay_update(struct bfd *) OVS_REQUIRES(mutex);
static void bfd_unixctl_show(struct unixctl_conn *, int argc,
const char *argv[], void *aux OVS_UNUSED);
static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
@@ -255,14 +270,16 @@ 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)
-OVS_EXCLUDED(mutex)
+bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
+ struct netdev *netdev) OVS_EXCLUDED(mutex)
{
static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
static atomic_uint16_t udp_src = ATOMIC_VAR_INIT(0);
+int decay_min_rx;
long long int min_tx, min_rx;
bool need_poll = false;
+bool cfg_min_rx_changed = false;
bool cpath_down;
const char *hwaddr;
uint8_t ea[ETH_ADDR_LEN];
@@ -293,6 +310,8 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg)
bfd->min_tx = 1000;
bfd->mult = 3;
atomic_init(&bfd->ref_cnt, 1);
+bfd->netdev = netdev_ref(netdev);
+bfd->in_decay = false;
/* RFC 5881 section 4
* The source port MUST be in the range 49152 through 65535. The same
@@ -328,6 +347,22 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg)
|| (!bfd_in_poll(bfd) && bfd->cfg_min_rx > bfd->min_rx)) {
bfd->min_rx = bfd->cfg_min_rx;
}
+cfg_min_rx_changed = true;
+need_poll = true;
+}
+
+decay_min_rx = smap_get_int(cfg, "decay_min_rx", 0);
+if (bfd->decay_min_rx != decay_min_rx || cfg_min_rx_changed) {
+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;
+}
+/* Resets decay. */
+bfd->in_decay = false;
+bfd_decay_update(bfd);
need_poll = true;
}
@@ -379,6 +414,7 @@ bfd_unref(struct bfd *bfd) OVS_EXCLUDED(mutex)
if (orig == 1) {
ovs_mutex_lock(&mutex);
hmap_remove(all_bfds, &bfd->node);
+netdev_close(bfd->netdev);
free(bfd->name);
free(bfd);
ovs_mutex_unlock(&mutex);
@@ -404,12 +440,27 @@ bfd_wait(const struct bfd *bfd) OVS_EXCLUDED(mutex)
void
bfd_run(struct bfd *bfd) OVS_EXCLUDED(mutex)
{
+long long int
[ovs-dev] [PATCH 2/2] bfd: Implements forwarding_if_rx
This commit adds a new boolean option "forwarding_if_rx" to bfd.
When forwarding_if_rx is true, 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 link liveness when
the link 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
---
lib/bfd.c| 69 +---
tests/bfd.at | 141 ++
vswitchd/vswitch.xml |7 +++
3 files changed, 209 insertions(+), 8 deletions(-)
diff --git a/lib/bfd.c b/lib/bfd.c
index 43ef759..b8d242b 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -191,12 +191,19 @@ struct bfd {
atomic_bool check_tnl_key;/* Verify tunnel key of inbound packets? */
atomic_int ref_cnt;
+/* When forward_if_rx is true, 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;
+long long int forwarding_if_rx_detect_time;
+
/* BFD decay related variables. */
bool in_decay;/* True when bfd is in decay. */
int decay_min_rx; /* min_rx is set to decay_min_rx when */
/* in decay. */
-int decay_rx_ctrl;/* Count bfd packets received within decay */
+uint64_t decay_rx_ctrl; /* Count bfd packets received within decay */
/* detect interval. */
+uint64_t decay_rx_packets;/* Packets received by 'netdev'. */
long long int decay_detect_time; /* Decay detection time. */
};
@@ -223,6 +230,8 @@ static void bfd_put_details(struct ds *, const struct bfd *)
static uint64_t bfd_rx_packets(const struct bfd *) OVS_REQUIRES(mutex);
static void bfd_try_decay(struct bfd *) OVS_REQUIRES(mutex);
static void bfd_decay_update(struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_check_rx(struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_forwarding_if_rx_update(struct bfd *) OVS_REQUIRES(mutex);
static void bfd_unixctl_show(struct unixctl_conn *, int argc,
const char *argv[], void *aux OVS_UNUSED);
static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
@@ -280,7 +289,7 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg,
long long int min_tx, min_rx;
bool need_poll = false;
bool cfg_min_rx_changed = false;
-bool cpath_down;
+bool cpath_down, forwarding_if_rx;
const char *hwaddr;
uint8_t ea[ETH_ADDR_LEN];
@@ -311,6 +320,7 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg,
bfd->mult = 3;
atomic_init(&bfd->ref_cnt, 1);
bfd->netdev = netdev_ref(netdev);
+bfd->rx_packets = bfd_rx_packets(bfd);
bfd->in_decay = false;
/* RFC 5881 section 4
@@ -384,6 +394,16 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg,
bfd->eth_dst_set = false;
}
+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_UP && bfd->forwarding_if_rx) {
+bfd_forwarding_if_rx_update(bfd);
+} else {
+bfd->forwarding_if_rx_detect_time = 0;
+}
+}
+
if (need_poll) {
bfd_poll(bfd);
}
@@ -458,6 +478,9 @@ bfd_run(struct bfd *bfd) OVS_EXCLUDED(mutex)
bfd_try_decay(bfd);
}
+/* Always checks the reception of any packet. */
+bfd_check_rx(bfd);
+
if (bfd->min_tx != bfd->cfg_min_tx
|| (bfd->min_rx != bfd->cfg_min_rx && bfd->min_rx != bfd->decay_min_rx)
|| bfd->in_decay != old_in_decay) {
@@ -753,6 +776,7 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev *netdev)
if (bfd->decay_min_rx) {
bfd_decay_update(bfd);
}
+bfd->rx_packets = bfd_rx_packets(bfd);
}
ovs_mutex_unlock(&mutex);
}
@@ -765,10 +789,12 @@ bfd_forwarding__(const struct bfd *bfd)
OVS_REQUIRES(mutex)
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->forwarding_if_rx_detect_time > time_msec()
+ : bfd->state == STATE_UP)
+ && bfd->rmt_diag != DIAG_PATH_DOWN
+ && bfd->rmt_diag != DIAG_CPATH_DOWN
+ && bfd->rmt_diag != DIAG_RCPATH_DOWN;
}
/* Helpers. */
@@ -999,7 +1025,7 @@ bfd_try_decay(struct bfd *bfd) OVS_REQUIRES(mutex)
* asynchronously to the bfd_rx_packets() function, the 'diff' value
* can be jittered. Thusly, we double the decay_rx_ctrl t
[ovs-dev] [PATCH v3] Introduce odp_flow_key_to_mask() API
From: Guolin Yang
With megaflow support, there is API to convert mask to nlattr key based
format. This change introduces API to do the reverse conversion. We
leverage the existing odp_flow_key_to_flow() API to resue the code.
Signed-off-by: Guolin Yang
---
lib/odp-util.c | 295 +---
lib/odp-util.h |2 +
2 files changed, 221 insertions(+), 76 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index a09042e..4373bbd 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2680,20 +2680,34 @@ check_expectations(uint64_t present_attrs, int
out_of_range_attr,
static bool
parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
uint64_t present_attrs, uint64_t *expected_attrs,
-struct flow *flow)
+struct flow *flow, struct flow *src_flow)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+bool is_mask = flow != src_flow;
if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
flow->dl_type = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]);
-if (ntohs(flow->dl_type) < 1536) {
+if (!is_mask && ntohs(flow->dl_type) < ETH_TYPE_MIN) {
VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key",
ntohs(flow->dl_type));
return false;
}
+if (is_mask && ntohs(src_flow->dl_type) < ETH_TYPE_MIN &&
+flow->dl_type != 0x) {
+return false;
+}
*expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
} else {
-flow->dl_type = htons(FLOW_DL_TYPE_NONE);
+if (!is_mask) {
+flow->dl_type = htons(FLOW_DL_TYPE_NONE);
+} else if (ntohs(src_flow->dl_type) < ETH_TYPE_MIN) {
+/*
+ * see comments odp_flow_key_from_flow__
+ */
+VLOG_ERR_RL(&rl, "Alway expect mask for non ethernet II "
+"frame");
+return false;
+}
}
return true;
}
@@ -2702,20 +2716,43 @@ static enum odp_key_fitness
parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
uint64_t present_attrs, int out_of_range_attr,
uint64_t expected_attrs, struct flow *flow,
- const struct nlattr *key, size_t key_len)
+ const struct nlattr *key, size_t key_len,
+ struct flow *src_flow)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-
-if (eth_type_mpls(flow->dl_type)) {
-expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
-
-if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS))) {
-return ODP_FIT_TOO_LITTLE;
+bool is_mask = src_flow != flow;
+const uint8_t *checkStart = NULL;
+size_t checkLen = 0;
+enum ovs_key_attr expected_bit = 0xff;
+
+if (eth_type_mpls(src_flow->dl_type)) {
+ if (!is_mask) {
+ expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
+
+ if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS))) {
+ return ODP_FIT_TOO_LITTLE;
+ }
+ flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
+ flow->mpls_depth++;
+} else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS)) {
+flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
+
+if (flow->mpls_lse != 0 && flow->dl_type != 0x) {
+return ODP_FIT_ERROR;
+}
+expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
+if (flow->mpls_lse) {
+/*
+ * do we need to set the following? XXX
+ */
+flow->mpls_depth = 0x;
+}
+}
+goto done;
+} else if (src_flow->dl_type == htons(ETH_TYPE_IP)) {
+if (!is_mask) {
+expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4;
}
-flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
-flow->mpls_depth++;
-} else if (flow->dl_type == htons(ETH_TYPE_IP)) {
-expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4;
if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV4)) {
const struct ovs_key_ipv4 *ipv4_key;
@@ -2725,12 +2762,19 @@ parse_l2_5_onward(const struct nlattr
*attrs[OVS_KEY_ATTR_MAX + 1],
flow->nw_proto = ipv4_key->ipv4_proto;
flow->nw_tos = ipv4_key->ipv4_tos;
flow->nw_ttl = ipv4_key->ipv4_ttl;
-if (!odp_to_ovs_frag(ipv4_key->ipv4_frag, flow)) {
+if (is_mask) {
+ flow->nw_frag = ipv4_key->ipv4_frag;
+checkStart = (const uint8_t *)ipv4_key;
+checkLen = sizeof (struct ovs_key_ipv4);
+expected_bit = OVS_KEY_ATTR_IPV4;
+} else if (!odp_to_ovs_frag(ipv4_key->ipv4_frag, flow)) {
Re: [ovs-dev] [Patch net-next] openvswitch: check CONFIG_OPENVSWITCH_GRE in makefile
On Tue, Aug 20, 2013 at 2:20 AM, Cong Wang wrote: > From: Cong Wang > > Cc: Jesse Gross > Cc: Pravin B Shelar > Signed-off-by: Cong Wang Applied, thanks Cong. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [clang-atomics 4/4] ovs-thread: Work around apparent Clang bug.
Without this patch, I get Clang warnings that I don't understand for each
of the non-static functions in dirs.c:
warning: control reaches end of non-void function
I get the same warning for this test program with "clang -c -Wall":
struct foo {
const char *y;
_Atomic(int) x;
};
const char *
bar(const struct foo *fp)
{
return fp->y;
}
const char *f(void)
{
static struct foo f = {"xyzzy", 0};
return bar(&f);
}
If I change {"xyzzy", 0} to just {"xyzzy"}, the warning goes away, and the
same is true for the current patch.
Signed-off-by: Ben Pfaff
---
lib/ovs-thread.h |7 +++
1 file changed, 7 insertions(+)
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index fb76954..dc56d83 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -460,11 +460,18 @@ struct ovsthread_once {
struct ovs_mutex mutex;
};
+#if !OVS_ATOMIC_CLANG_IMPL
#define OVSTHREAD_ONCE_INITIALIZER \
{ \
ATOMIC_VAR_INIT(false), \
OVS_ADAPTIVE_MUTEX_INITIALIZER, \
}
+#else /* OVS_ATOMIC_CLANG_IMPL */
+/* Clang 3.4-1~exp1 has some odd ideas about initializers: if we use the
+ * conventional initializer above, then it complains that "warning: control
+ * reaches end of non-void function" in each of the functions in lib/dirs.c. */
+#define OVSTHREAD_ONCE_INITIALIZER { .mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER }
+#endif
static inline bool ovsthread_once_start(struct ovsthread_once *once)
OVS_TRY_LOCK(true, once->mutex);
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [clang-atomics 1/4] ovs-atomic: Fix typo in comment.
Signed-off-by: Ben Pfaff --- lib/ovs-atomic.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ovs-atomic.h b/lib/ovs-atomic.h index 3fc9dcb..3467f11 100644 --- a/lib/ovs-atomic.h +++ b/lib/ovs-atomic.h @@ -21,7 +21,7 @@ * * This library implements atomic operations with an API based on the one * defined in C11. It includes multiple implementations for compilers and - * libraries with varying degrees of built-in support for C11, including an + * libraries with varying degrees of built-in support for C11, including a * fallback implementation for systems that have pthreads but no other support * for atomics. * -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [clang-atomics 3/4] ovs-atomic: Add native Clang implementation.
With this implementation I get warnings with Clang on GNU/Linux when the
previous patch is not applied. This ought to make it easier to avoid
introducing new problems in the future even without building on FreeBSD.
Signed-off-by: Ben Pfaff
---
lib/automake.mk |2 +
lib/compiler.h|3 ++
lib/ovs-atomic-clang.h| 115 +
lib/ovs-atomic-flag-gcc4.7+.h | 52 +++
lib/ovs-atomic-gcc4.7+.h | 34 +---
lib/ovs-atomic.h |2 +
6 files changed, 176 insertions(+), 32 deletions(-)
create mode 100644 lib/ovs-atomic-clang.h
create mode 100644 lib/ovs-atomic-flag-gcc4.7+.h
diff --git a/lib/automake.mk b/lib/automake.mk
index f936897..ffbfc43 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -125,6 +125,8 @@ lib_libopenvswitch_a_SOURCES = \
lib/ofpbuf.c \
lib/ofpbuf.h \
lib/ovs-atomic-c11.h \
+ lib/ovs-atomic-clang.h \
+ lib/ovs-atomic-flag-gcc4.7+.h \
lib/ovs-atomic-gcc4+.c \
lib/ovs-atomic-gcc4+.h \
lib/ovs-atomic-gcc4.7+.h \
diff --git a/lib/compiler.h b/lib/compiler.h
index 519b832..43eb8eb 100644
--- a/lib/compiler.h
+++ b/lib/compiler.h
@@ -20,6 +20,9 @@
#ifndef __has_feature
#define __has_feature(x) 0
#endif
+#ifndef __has_extension
+ #define __has_extension(x) 0
+#endif
#if __GNUC__ && !__CHECKER__
#define NO_RETURN __attribute__((__noreturn__))
diff --git a/lib/ovs-atomic-clang.h b/lib/ovs-atomic-clang.h
new file mode 100644
index 000..dbec956
--- /dev/null
+++ b/lib/ovs-atomic-clang.h
@@ -0,0 +1,115 @@
+/*
+ * Copyright (c) 2013 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/* This header implements atomic operation primitives on Clang. */
+#ifndef IN_OVS_ATOMIC_H
+#error "This header should only be included indirectly via ovs-atomic.h."
+#endif
+
+#define OVS_ATOMIC_CLANG_IMPL 1
+
+/* Standard atomic types. */
+typedef _Atomic(_Bool) atomic_bool;
+
+typedef _Atomic(char) atomic_char;
+typedef _Atomic(signed char) atomic_schar;
+typedef _Atomic(unsigned char) atomic_uchar;
+
+typedef _Atomic(short) atomic_short;
+typedef _Atomic(unsigned short) atomic_ushort;
+
+typedef _Atomic(int) atomic_int;
+typedef _Atomic(unsigned int) atomic_uint;
+
+typedef _Atomic(long) atomic_long;
+typedef _Atomic(unsigned long) atomic_ulong;
+
+typedef _Atomic(long long) atomic_llong;
+typedef _Atomic(unsigned long long) atomic_ullong;
+
+typedef _Atomic(size_t) atomic_size_t;
+typedef _Atomic(ptrdiff_t) atomic_ptrdiff_t;
+
+typedef _Atomic(intmax_t) atomic_intmax_t;
+typedef _Atomic(uintmax_t) atomic_uintmax_t;
+
+typedef _Atomic(intptr_t) atomic_intptr_t;
+typedef _Atomic(uintptr_t) atomic_uintptr_t;
+
+/* Nonstandard atomic types. */
+typedef _Atomic(uint8_t) atomic_uint8_t;
+typedef _Atomic(uint16_t) atomic_uint16_t;
+typedef _Atomic(uint32_t) atomic_uint32_t;
+typedef _Atomic(uint64_t) atomic_uint64_t;
+
+typedef _Atomic(int8_t)atomic_int8_t;
+typedef _Atomic(int16_t) atomic_int16_t;
+typedef _Atomic(int32_t) atomic_int32_t;
+typedef _Atomic(int64_t) atomic_int64_t;
+
+#define ATOMIC_VAR_INIT(VALUE) (VALUE)
+
+#define atomic_init(OBJECT, VALUE) __c11_atomic_init(OBJECT, VALUE)
+
+/* Clang hard-codes these exact values internally but does not appear to
+ * export any names for them. */
+typedef enum {
+memory_order_relaxed = 0,
+memory_order_consume = 1,
+memory_order_acquire = 2,
+memory_order_release = 3,
+memory_order_acq_rel = 4,
+memory_order_seq_cst = 5
+} memory_order;
+
+#define atomic_thread_fence(ORDER) __c11_atomic_thread_fence(ORDER)
+#define atomic_signal_fence(ORDER) __c11_atomic_signal_fence(ORDER)
+
+#define atomic_store(DST, SRC) \
+atomic_store_explicit(DST, SRC, memory_order_seq_cst)
+#define atomic_store_explicit(DST, SRC, ORDER) \
+__c11_atomic_store(DST, SRC, ORDER)
+
+
+#define atomic_read(SRC, DST) \
+atomic_read_explicit(SRC, DST, memory_order_seq_cst)
+#define atomic_read_explicit(SRC, DST, ORDER) \
+(*(DST) = __c11_atomic_load(SRC, ORDER), \
+ (void) 0)
+
+#define atomic_add(RMW, ARG, ORIG) \
+atomic_add_explicit(RMW, ARG, ORIG, memory_order_seq_cst)
+#define atomic_sub(RMW, ARG, ORIG) \
+atomic_sub_explicit(RMW, ARG, ORIG, memory_order_seq_cst)
+#define atomic_or(RMW, ARG, ORIG) \
+atomic_or_explicit(RMW, ARG, ORIG, memory_order_seq_cst)
+#define atomic_xor(RMW, ARG, ORIG) \
+at
[ovs-dev] [clang-atomics 2/4] ovs-atomic: atomic_load() must take a non-const argument.
C11 says that atomic_load() requires a non-const argument, and Clang
enforces that. This fixes warnings with FreeBSD that uses
the Clang extensions.
Reported-by: Ed Maste
Signed-off-by: Ben Pfaff
---
lib/bfd.c|4 +++-
lib/cfm.c| 14 --
lib/ovs-thread.h |2 +-
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/lib/bfd.c b/lib/bfd.c
index 6f86f26..fb3b682 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -502,10 +502,12 @@ bfd_put_packet(struct bfd *bfd, struct ofpbuf *p,
}
bool
-bfd_should_process_flow(const struct bfd *bfd, const struct flow *flow,
+bfd_should_process_flow(const struct bfd *bfd_, const struct flow *flow,
struct flow_wildcards *wc)
{
+struct bfd *bfd = CONST_CAST(struct bfd *, bfd_);
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;
diff --git a/lib/cfm.c b/lib/cfm.c
index a238c67..297894e 100644
--- a/lib/cfm.c
+++ b/lib/cfm.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -168,7 +168,7 @@ cfm_rx_packets(const struct cfm *cfm) OVS_REQUIRES(mutex)
}
static const uint8_t *
-cfm_ccm_addr(const struct cfm *cfm)
+cfm_ccm_addr(struct cfm *cfm)
{
bool extended;
atomic_read(&cfm->extended, &extended);
@@ -648,9 +648,10 @@ cfm_set_netdev(struct cfm *cfm, const struct netdev
*netdev)
/* Returns true if 'cfm' should process packets from 'flow'. Sets
* fields in 'wc' that were used to make the determination. */
bool
-cfm_should_process_flow(const struct cfm *cfm, const struct flow *flow,
+cfm_should_process_flow(const struct cfm *cfm_, const struct flow *flow,
struct flow_wildcards *wc)
{
+struct cfm *cfm = CONST_CAST(struct cfm *, cfm_);
bool check_tnl_key;
atomic_read(&cfm->check_tnl_key, &check_tnl_key);
@@ -839,8 +840,9 @@ cfm_get_health(const struct cfm *cfm) OVS_EXCLUDED(mutex)
* 'cfm' is operationally down, or -1 if 'cfm' has no operational state
* (because it isn't in extended mode). */
int
-cfm_get_opup(const struct cfm *cfm) OVS_EXCLUDED(mutex)
+cfm_get_opup(const struct cfm *cfm_) OVS_EXCLUDED(mutex)
{
+struct cfm *cfm = CONST_CAST(struct cfm *, cfm_);
bool extended;
int opup;
@@ -879,7 +881,7 @@ cfm_find(const char *name) OVS_REQUIRES(mutex)
}
static void
-cfm_print_details(struct ds *ds, const struct cfm *cfm) OVS_REQUIRES(mutex)
+cfm_print_details(struct ds *ds, struct cfm *cfm) OVS_REQUIRES(mutex)
{
struct remote_mp *rmp;
bool extended;
@@ -926,7 +928,7 @@ cfm_unixctl_show(struct unixctl_conn *conn, int argc, const
char *argv[],
void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
{
struct ds ds = DS_EMPTY_INITIALIZER;
-const struct cfm *cfm;
+struct cfm *cfm;
ovs_mutex_lock(&mutex);
if (argc > 1) {
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index b7bc5d1..fb76954 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -475,7 +475,7 @@ bool ovsthread_once_start__(struct ovsthread_once *once)
OVS_TRY_LOCK(false, once->mutex);
static inline bool
-ovsthread_once_is_done__(const struct ovsthread_once *once)
+ovsthread_once_is_done__(struct ovsthread_once *once)
{
bool done;
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [branch-1.11] ofproto-dpif: Fix memory leak in handle_flow_miss().
On Tue, Aug 20, 2013 at 12:25:37AM -0700, Justin Pettit wrote: > On Aug 20, 2013, at 12:01 AM, Ben Pfaff wrote: > > > > > On Aug 19, 2013 11:35 PM, "Justin Pettit" wrote: > > > > > > Great catch. Also, very cool with sleuthing with your malloc histogram > > > (as discussed off-list). > > > > Thanks. > > > > Did you look around to make sure that I didn't somehow overlook a > > free call somewhere? This patch "feels right" but obviously it > > needs to be more than a feeling. > > I think it's correct. The only references of that xout instance are > given to handle_flow_miss_without_facet() and facet_create(), which > make local copies of the data and don't retain references to that > instance. > > I also had gone through the code to see if we missed any other frees > and didn't see any. Thanks. I applied this to branch-1.11. "master" didn't have the same problem so I only applied the comment update there. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Clang compile failure with C11 atomics
On Mon, Aug 19, 2013 at 05:31:36AM -0400, Ed Maste wrote:
> Commit 1514b275 introduces the use of atomic_read_explicit for
> once-only initializers. When using C11 atomics this becomes
> atomic_load_explicit, and the first argument needs to be non-const --
> with FreeBSD HEAD's in-tree clang 3.3 the build fails for me with:
>
> In file included from lib/bfd.c:34:
> ./lib/ovs-thread.h:482:5: error: first argument to atomic operation must be a
> pointer to non-const _Atomic type ('const atomic_bool *' (aka
> 'const _Atomic(bool) *') invalid)
> atomic_read_explicit(&once->done, &done, memory_order_relaxed);
> ^~~~
>
> One workaround is to sprinkle CONST_CASTs around, in ovs-thread.h and
> lib/bfd. - e.g.:
>
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -479,7 +479,8 @@ ovsthread_once_is_done__(const struct ovsthread_once
> *once)
> {
> bool done;
>
> -atomic_read_explicit(&once->done, &done, memory_order_relaxed);
> +atomic_read_explicit(CONST_CAST(atomic_bool *, &once->done), &done,
> + memory_order_relaxed);
> return done;
> }
>
> Ben, what do you think?
Oops. Thanks for the bug report.
I sent out a series that should fix the problem and make it more
difficult for it to recur:
http://openvswitch.org/pipermail/dev/2013-August/030888.html
http://openvswitch.org/pipermail/dev/2013-August/030889.html
http://openvswitch.org/pipermail/dev/2013-August/030890.html
The last patch is really curious and makes me wonder if I really
misunderstand something:
http://openvswitch.org/pipermail/dev/2013-August/030891.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Enable smooth transition between CFM and BFD.
When user switches between using CFM and BFD, there will be a short
down time before the new protocol goes up. This can unintentionally
change the traffic pattern of the bundled ports. To prevent this,
it is proposed that user can enable both CFM and BFD before transition,
wait for the new protocol to go up, and then disable the old protocol.
To make this scheme work, this commit modifies the port_run() in
ofproto-dpif.c, so that when both CFM and BFD are used, if either shows
correct status, the port is considered usable in the bundle.
Signed-off-by: Alex Wang
---
ofproto/ofproto-dpif.c | 12 +--
tests/ofproto-dpif.at | 81
2 files changed, 90 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4690215..9fe91c1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2952,6 +2952,8 @@ port_run(struct ofport_dpif *ofport)
long long int carrier_seq = netdev_get_carrier_resets(ofport->up.netdev);
bool carrier_changed = carrier_seq != ofport->carrier_seq;
bool enable = netdev_get_carrier(ofport->up.netdev);
+bool cfm_enable = false;
+bool bfd_enable = false;
ofport->carrier_seq = carrier_seq;
@@ -2961,16 +2963,20 @@ port_run(struct ofport_dpif *ofport)
int cfm_opup = cfm_get_opup(ofport->cfm);
cfm_run(ofport->cfm);
-enable = enable && !cfm_get_fault(ofport->cfm);
+cfm_enable = !cfm_get_fault(ofport->cfm);
if (cfm_opup >= 0) {
-enable = enable && cfm_opup;
+cfm_enable = cfm_enable && cfm_opup;
}
}
if (ofport->bfd) {
bfd_run(ofport->bfd);
-enable = enable && bfd_forwarding(ofport->bfd);
+bfd_enable = bfd_forwarding(ofport->bfd);
+}
+
+if (ofport->bfd || ofport->cfm) {
+enable = enable && (cfm_enable || bfd_enable);
}
if (ofport->bundle) {
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b093998..4772416 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2689,3 +2689,84 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: 5
])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+# Tests the bundling with various bfd and cfm configurations.
+AT_SETUP([ofproto - bundle with variable bfd/cfm config])
+OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy -- \
+add-bond br0 br0bond p0 p2 bond-mode=active-backup -- \
+add-bond br1 br1bond p1 p3 bond-mode=active-backup -- \
+set Interface p1 type=patch options:peer=p0
ofport_request=2 -- \
+set Interface p3 type=patch options:peer=p2
ofport_request=4 -- \
+set Interface p0 type=patch options:peer=p1
ofport_request=1 -- \
+set Interface p2 type=patch options:peer=p3
ofport_request=3 -- \
+set Interface p0 bfd:enable=true bfd:min_tx=300
bfd:min_rx=300 -- \
+set Interface p0 cfm_mpid=1 -- \
+set Interface p1 bfd:enable=true bfd:min_tx=500
bfd:min_rx=500])
+
+ovs-appctl time/stop
+# advance the clock to stablize everything.
+for i in `seq 0 49`; do ovs-appctl time/warp 100; done
+# cfm/show should show 'recv' fault.
+AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
+ fault: recv
+])
+# bfd/show should show 'up'.
+AT_CHECK([ovs-appctl bfd/show | sed -n '/^.*Session State:.*/p'], [0], [dnl
+ Local Session State: up
+ Remote Session State: up
+ Local Session State: up
+ Remote Session State: up
+])
+# bond/show should show 'may-enable: true' for all slaves.
+AT_CHECK([ovs-appctl bond/show | sed -n '/^.*may_enable:.*/p'], [0], [dnl
+ may_enable: true
+ may_enable: true
+ may_enable: true
+ may_enable: true
+])
+
+# now disable the bfd on p1.
+AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=false])
+# advance the clock to stablize everything.
+for i in `seq 0 49`; do ovs-appctl time/warp 100; done
+# cfm/show should show 'recv' fault.
+AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
+ fault: recv
+])
+# bfd/show should show 'down'.
+AT_CHECK([ovs-appctl bfd/show | sed -n '/^.*Session State:.*/p'], [0], [dnl
+ Local Session State: down
+ Remote Session State: down
+])
+# bond/show should show 'may-enable: false' for p0.
+AT_CHECK([ovs-appctl bond/show | sed -n '/^.*may_enable:.*/p'], [0], [dnl
+ may_enable: false
+ may_enable: true
+ may_enable: true
+ may_enable: true
+])
+
+# now enable the bfd on p1 and disable bfd on p0.
+AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=true])
+AT_CHECK([ovs-vsctl set Interface p0 bfd:enable=false])
+# advance the clock to stablize everything.
+for i in `seq 0 49`; do ovs-appctl time/warp 100; done
+# cfm/show should show 'recv' fault.
+AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
[ovs-dev] [PATCH v2] Use "error-checking" mutexes in place of other kinds wherever possible.
We've seen a number of deadlocks in the tree since thread safety was
introduced. So far, all of these are self-deadlocks, that is, a single
thread acquiring a lock and then attempting to re-acquire the same lock
recursively. When this has happened, the process simply hung, and it was
somewhat difficult to find the cause.
POSIX "error-checking" mutexes check for this specific problem (and
others). This commit switches from other types of mutexes to
error-checking mutexes everywhere that we can, that is, everywhere that
we're not using recursive mutexes. This ought to help find problems more
quickly in the future.
There might be performance advantages to other kinds of mutexes in some
cases. However, the existing mutex type choices were just guesses, so I'd
rather go for easy detection of errors until we know that other mutex
types actually perform better in specific cases. Also, I did a quick
microbenchmark of glibc mutex types on my host and found that the
error checking mutexes weren't any slower than the other types, at least
when the mutex is uncontended.
Signed-off-by: Ben Pfaff
---
v1->v2: Instead of passing a 'type' parameter to ovs_mutex_init(),
break off a separate function for recursive mutexes.
include/sparse/pthread.h |3 ---
lib/dpif-linux.c |2 +-
lib/fatal-signal.c|2 +-
lib/lacp.c|4 ++--
lib/netdev-bsd.c |6 +++---
lib/netdev-dummy.c|2 +-
lib/netdev-linux.c|2 +-
lib/netdev-vport.c|2 +-
lib/netlink-socket.c |2 +-
lib/ovs-atomic-gcc4+.c|2 +-
lib/ovs-thread.c | 16 ++--
lib/ovs-thread.h | 38 ++
lib/seq.c |2 +-
lib/stp.c |4 ++--
lib/uuid.c|2 +-
lib/vlog.c|2 +-
lib/vlog.h|2 +-
ofproto/ofproto-dpif-sflow.c |2 +-
ofproto/ofproto-dpif-upcall.c |8
ofproto/ofproto-dpif.c|8
ofproto/ofproto.c |4 ++--
vswitchd/system-stats.c |2 +-
22 files changed, 50 insertions(+), 67 deletions(-)
diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index 40c5ca3..e5b2a08 100644
--- a/include/sparse/pthread.h
+++ b/include/sparse/pthread.h
@@ -30,8 +30,5 @@
#undef PTHREAD_RWLOCK_INITIALIZER
#define PTHREAD_RWLOCK_INITIALIZER {}
-#undef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
-#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP {}
-
#undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 1b97410..a95ece1 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -248,7 +248,7 @@ open_dpif(const struct dpif_linux_dp *dp, struct dpif
**dpifp)
dpif = xzalloc(sizeof *dpif);
dpif->port_notifier = NULL;
-ovs_mutex_init(&dpif->upcall_lock, PTHREAD_MUTEX_DEFAULT);
+ovs_mutex_init(&dpif->upcall_lock);
dpif->epoll_fd = -1;
dpif_init(&dpif->dpif, &dpif_linux_class, dp->name,
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index b4c5ee6..e980f4b 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -77,7 +77,7 @@ fatal_signal_init(void)
assert_single_threaded();
inited = true;
-ovs_mutex_init(&mutex, PTHREAD_MUTEX_RECURSIVE);
+ovs_mutex_init_recursive(&mutex);
xpipe_nonblocking(signal_fds);
for (i = 0; i < ARRAY_SIZE(fatal_signals); i++) {
diff --git a/lib/lacp.c b/lib/lacp.c
index 8269874..5421e2a 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2011, 2012 Nicira, Inc.
+/* Copyright (c) 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -207,7 +207,7 @@ lacp_create(void) OVS_EXCLUDED(mutex)
struct lacp *lacp;
if (ovsthread_once_start(&once)) {
-ovs_mutex_init(&mutex, PTHREAD_MUTEX_RECURSIVE);
+ovs_mutex_init_recursive(&mutex);
ovsthread_once_done(&once);
}
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 180ce7f..7c19483 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011 Gaetano Catalli.
+ * Copyright (c) 2011, 2013 Gaetano Catalli.
* Copyright (c) 2013 YAMAMOTO Takashi.
*
* Licensed under the Apache License, Version 2.0 (the "License");
@@ -293,7 +293,7 @@ netdev_bsd_construct_system(struct netdev *netdev_)
return error;
}
-ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+ovs_mutex_init(&netdev->mutex);
netdev->change_seq = 1;
netdev->tap_fd = -1;
netdev->kernel_name = xstrdup(netdev_->name);
@@ -327,7 +327,7 @@ netdev_bsd_construct_tap(struct netdev *netdev_)
/* Create a tap device by opening
Re: [ovs-dev] [PATCH] Use "error-checking" mutexes in place of other kinds wherever possible.
I defined a separate ovs_mutex_init_recursive(). I sent out a v2 in case you want to look: http://openvswitch.org/pipermail/dev/2013-August/030895.html On Sat, Aug 17, 2013 at 05:21:41PM +0800, Ethan Jackson wrote: > Oops yes you're right. It might be worth defining a separate function for > that, but its your call. The patch is good as is. > > Ethan (iPhone) > > On Aug 17, 2013, at 13:08, Ben Pfaff wrote: > > > > > On Aug 16, 2013 9:47 PM, "Ethan Jackson" wrote: > > > Personally I'd prefer we change ovs_mutex_init() to not take a mutex > > > type as an argument and simply use the error checking mutex always. I > > > think it's going to be a long time before we actually need to > > > configure this on a per mutex basis, and I don't think we know what > > > abstraction we'll need at that point today. > > > > Don't we need the ability to initialize a recursive mutex? It didn't occur > > to me to drop the parameter but that's the only current reason to keep it. > > > > Thanks, > > > > Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto: Start ofport allocation from the previous max after restart.
On Mon, Aug 19, 2013 at 04:59:16PM -0700, Gurucharan Shetty wrote: > We currently do not recycle ofport numbers from interfaces that are recently > deleted by maintaining the maximum allocated ofport value and > allocating new ofport numbers greater than the previous maximum. > But after a restart of ovs-vswitchd, we start again from ofport value of '1'. > This means that after a restart, we can immeditaley recycle the 'ofport' > value of the most recently deleted interface. > > With this commit, during ovs-vswitchd initial configuration, we figure > out the previously allocated max ofport value. New interfaces get ofport > value that is greater than this. > > Signed-off-by: Gurucharan Shetty Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread 09/15] ofproto-dpif: Lock the expirable list.
On Sun, Aug 18, 2013 at 02:56:55PM +1000, Simon Horman wrote: > On Thu, Aug 08, 2013 at 02:37:48PM -0700, Ben Pfaff wrote: > > On Thu, Aug 08, 2013 at 02:32:08PM -0700, Ethan Jackson wrote: > > > > Simon presented numbers that showed it to be a valuable optimization > > > > in some cases, otherwise I'd just say get rid of it. > > > > > > If that's the only reason we have it, I vote we ditch it. It's a new > > > world with multithreading, I'd like to have a clean slate in the > > > direction of thread safety and add optimizations back if they help in > > > this new paradigm > > > > That's the only reason we have it. If you want to ditch it be my > > guest. > > I accept that multi-threaded ovs-vswtichd is a whole new world > and that dropping optimisations that previously made sense is logical. > And I guess that the best thing is for the situation that lead to > this optimisation to be re-profiled once the multi-threading work is more > complete. > > For reference, my recollection is that this optimisation came > about because it was observed that inserting 100k non-expirable flows > would result in ovs-vswtichd consuming about 100% (of one) CPU running > through the list of flows to see if any of them were ready to be expired > although none of them ever would be. With the optimisation in place > CPU utilisation was reduced to roughly 0% as the list that was traversed > became empty and the system was otherwise idle. I think it's perfectly reasonable to reintroduce the optimization. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Use "error-checking" mutexes in place of other kinds wherever possible.
I skimmed it, looks fine to me. Ethan On Tue, Aug 20, 2013 at 11:25 AM, Ben Pfaff wrote: > I defined a separate ovs_mutex_init_recursive(). I sent out a v2 in > case you want to look: > http://openvswitch.org/pipermail/dev/2013-August/030895.html > > On Sat, Aug 17, 2013 at 05:21:41PM +0800, Ethan Jackson wrote: >> Oops yes you're right. It might be worth defining a separate function for >> that, but its your call. The patch is good as is. >> >> Ethan (iPhone) >> >> On Aug 17, 2013, at 13:08, Ben Pfaff wrote: >> >> > >> > On Aug 16, 2013 9:47 PM, "Ethan Jackson" wrote: >> > > Personally I'd prefer we change ovs_mutex_init() to not take a mutex >> > > type as an argument and simply use the error checking mutex always. I >> > > think it's going to be a long time before we actually need to >> > > configure this on a per mutex basis, and I don't think we know what >> > > abstraction we'll need at that point today. >> > >> > Don't we need the ability to initialize a recursive mutex? It didn't occur >> > to me to drop the parameter but that's the only current reason to keep it. >> > >> > Thanks, >> > >> > Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread 09/15] ofproto-dpif: Lock the expirable list.
I never removed it, just complained about it. Ethan On Tue, Aug 20, 2013 at 12:47 PM, Ben Pfaff wrote: > On Sun, Aug 18, 2013 at 02:56:55PM +1000, Simon Horman wrote: >> On Thu, Aug 08, 2013 at 02:37:48PM -0700, Ben Pfaff wrote: >> > On Thu, Aug 08, 2013 at 02:32:08PM -0700, Ethan Jackson wrote: >> > > > Simon presented numbers that showed it to be a valuable optimization >> > > > in some cases, otherwise I'd just say get rid of it. >> > > >> > > If that's the only reason we have it, I vote we ditch it. It's a new >> > > world with multithreading, I'd like to have a clean slate in the >> > > direction of thread safety and add optimizations back if they help in >> > > this new paradigm >> > >> > That's the only reason we have it. If you want to ditch it be my >> > guest. >> >> I accept that multi-threaded ovs-vswtichd is a whole new world >> and that dropping optimisations that previously made sense is logical. >> And I guess that the best thing is for the situation that lead to >> this optimisation to be re-profiled once the multi-threading work is more >> complete. >> >> For reference, my recollection is that this optimisation came >> about because it was observed that inserting 100k non-expirable flows >> would result in ovs-vswtichd consuming about 100% (of one) CPU running >> through the list of flows to see if any of them were ready to be expired >> although none of them ever would be. With the optimisation in place >> CPU utilisation was reduced to roughly 0% as the list that was traversed >> became empty and the system was otherwise idle. > > I think it's perfectly reasonable to reintroduce the optimization. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
On Mon, Aug 12, 2013 at 11:34:57AM -0700, Romain Lenglet wrote:
> Implement a per-exporter flow cache with active timeout expiration.
> Add columns "cache_active_timeout" and "cache_max_flows" into table
> "IPFIX" to configure each cache.
>
> Add per-flow elements "octetDeltaSumOfSquares",
> "minimumIpTotalLength", and "maximumIpTotalLength" to replace
> "ethernetTotalLength". Add per-flow element "flowEndReason" to
> indicate whether a flow has expired because of an active timeout, the
> cache size limit being reached, or the exporter being stopped.
>
> Signed-off-by: Romain Lenglet
Thanks for the patch. Sorry that it's taken so long to look at this.
"clang" reports:
../ofproto/ofproto-dpif-ipfix.c:1456:58: error: variable
'next_timeout_msec' is uninitialized when used here [-Werror,-Wuninitialized]
&& (!has_next_timeout || temp_timeout_msec <
next_timeout_msec)) {
^
../ofproto/ofproto-dpif-ipfix.c:1448:36: note: initialize the variable
'next_timeout_msec' to silence this warning
long long int next_timeout_msec, temp_timeout_msec;
^
= 0
You could make it happy, and simplify the code, by initializing
next_timeout_msec to LLONG_MAX. Or you could call
poll_timer_wait_until(), which is cheap, each place you update
next_timeout_msec.
Please include just after "ofproto-dpif-ipfix.h" rather
than just before. That way, we can be sure that ofproto-dpif-ipfix.h
remains self-contained:
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 8e8e7a2..c37da5f 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -15,15 +15,18 @@
> */
>
> #include
> +#include
> #include "ofproto-dpif-ipfix.h"
The reason for this change isn't obvious. But we cannot use
__kernel_time_t because it is not a portable type (it is Linux kernel
specific):
> @@ -41,7 +44,11 @@
> struct dpif_ipfix_exporter {
> struct collectors *collectors;
> uint32_t seq_number;
> -time_t last_template_set_time;
> +__kernel_time_t last_template_set_time;
Perhaps on the same topic, this is the first time I've run into
timercmp(). It appears to be a nonstandard BSDism. Is there a reason
to use "struct timeval" instead of the "long long int as msecs from
the epoch" that we usually use in OVS? The latter is nice, when you
can use it, because arithmetic and relational operators are meaningful
and simple.
I see that this patch adds multiple consecutive blank lines in a few
places. We don't usually do that.
In ipfix_cache_next_timeout_msec(), usually I put a space just after
LIST_FOR_EACH (because I want it to look like a statement, not a
function call).
I see that ipfix_cache_entry_init() uses ofpbuf_use_stub() and then
later asserts that the buffer was not reallocated. If that is the
behavior that you want, then you can use ofpbuf_use_stack(), which
will assert-fail immediately when reallocation is attempted.
70 minutes is a curious number to choose as the maximum for
cache_active_timeout. Is this the maximum value allowed by IPFIX?
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread 09/15] ofproto-dpif: Lock the expirable list.
Ah. Is it OK for us to keep complaining about you removing it though? On Tue, Aug 20, 2013 at 01:10:06PM -0700, Ethan Jackson wrote: > I never removed it, just complained about it. > > Ethan > > On Tue, Aug 20, 2013 at 12:47 PM, Ben Pfaff wrote: > > On Sun, Aug 18, 2013 at 02:56:55PM +1000, Simon Horman wrote: > >> On Thu, Aug 08, 2013 at 02:37:48PM -0700, Ben Pfaff wrote: > >> > On Thu, Aug 08, 2013 at 02:32:08PM -0700, Ethan Jackson wrote: > >> > > > Simon presented numbers that showed it to be a valuable optimization > >> > > > in some cases, otherwise I'd just say get rid of it. > >> > > > >> > > If that's the only reason we have it, I vote we ditch it. It's a new > >> > > world with multithreading, I'd like to have a clean slate in the > >> > > direction of thread safety and add optimizations back if they help in > >> > > this new paradigm > >> > > >> > That's the only reason we have it. If you want to ditch it be my > >> > guest. > >> > >> I accept that multi-threaded ovs-vswtichd is a whole new world > >> and that dropping optimisations that previously made sense is logical. > >> And I guess that the best thing is for the situation that lead to > >> this optimisation to be re-profiled once the multi-threading work is more > >> complete. > >> > >> For reference, my recollection is that this optimisation came > >> about because it was observed that inserting 100k non-expirable flows > >> would result in ovs-vswtichd consuming about 100% (of one) CPU running > >> through the list of flows to see if any of them were ready to be expired > >> although none of them ever would be. With the optimisation in place > >> CPU utilisation was reduced to roughly 0% as the list that was traversed > >> became empty and the system was otherwise idle. > > > > I think it's perfectly reasonable to reintroduce the optimization. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Use "error-checking" mutexes in place of other kinds wherever possible.
Thanks. Applied to master. On Tue, Aug 20, 2013 at 01:09:42PM -0700, Ethan Jackson wrote: > I skimmed it, looks fine to me. > > Ethan > > On Tue, Aug 20, 2013 at 11:25 AM, Ben Pfaff wrote: > > I defined a separate ovs_mutex_init_recursive(). I sent out a v2 in > > case you want to look: > > http://openvswitch.org/pipermail/dev/2013-August/030895.html > > > > On Sat, Aug 17, 2013 at 05:21:41PM +0800, Ethan Jackson wrote: > >> Oops yes you're right. It might be worth defining a separate function for > >> that, but its your call. The patch is good as is. > >> > >> Ethan (iPhone) > >> > >> On Aug 17, 2013, at 13:08, Ben Pfaff wrote: > >> > >> > > >> > On Aug 16, 2013 9:47 PM, "Ethan Jackson" wrote: > >> > > Personally I'd prefer we change ovs_mutex_init() to not take a mutex > >> > > type as an argument and simply use the error checking mutex always. I > >> > > think it's going to be a long time before we actually need to > >> > > configure this on a per mutex basis, and I don't think we know what > >> > > abstraction we'll need at that point today. > >> > > >> > Don't we need the ability to initialize a recursive mutex? It didn't > >> > occur to me to drop the parameter but that's the only current reason to > >> > keep it. > >> > > >> > Thanks, > >> > > >> > Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
On Mon, Aug 19, 2013 at 12:32:06PM -0700, Alex Wang wrote:
> Just noticed, when compiling with sparse, it issues the warnings like:
>
> """
> lib/netdev-linux.c:76:1: warning: symbol 'counter_netdev_set_policing' was
> not declared. Should it be static?
> lib/netdev-linux.c:77:1: warning: symbol 'counter_netdev_arp_lookup' was
> not declared. Should it be static?
> lib/netdev-linux.c:78:1: warning: symbol 'counter_netdev_get_ifindex' was
> not declared. Should it be static?
> """
Ouch, thanks, I folded in this incremental to fix that:
diff --git a/lib/coverage.h b/lib/coverage.h
index 73b027a..3d1a115 100644
--- a/lib/coverage.h
+++ b/lib/coverage.h
@@ -54,6 +54,7 @@ struct coverage_counter {
{ \
*counter_##COUNTER##_get() += n;\
} \
+extern struct coverage_counter counter_##COUNTER; \
struct coverage_counter counter_##COUNTER \
= { #COUNTER, COUNTER##_count, 0 }; \
extern struct coverage_counter *counter_ptr_##COUNTER; \
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
On Mon, Aug 19, 2013 at 08:08:37AM -0700, Alex Wang wrote:
> This looks good to me,
>
> The only issue may be that some comments need to be changed:
>
> -/* Sorts coverage counters in descending order by count, within equal
> counts
> - * alphabetically by name. */
> +/* Sorts coverage counters in descending order by total count,
> + * within equal total counts alphabetically by name. */
> static int
> compare_coverage_counters(const void *a_, const void *b_)
> {
> @@ -108,7 +108,7 @@ coverage_hash(void)
> uint32_t hash = 0;
> int n_groups, i;
>
> -/* Sort coverage counters into groups with equal counts. */
> +/* Sort coverage counters into groups with equal total counts. */
> c = xmalloc(n_coverage_counters * sizeof *c);
> ovs_mutex_lock(&coverage_mutex);
> for (i = 0; i < n_coverage_counters; i++) {
That is a little clearer, thanks.
I'll apply this in a minute.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OF1.1+ Groups
On Tue, Aug 13, 2013 at 11:15:58AM -0700, Casey Barker wrote: > I've got a lot more code I can upstream, but I haven't seen any reviews for > the first patch. Any comments? (Or did I submit it incorrectly?) You sent it right but I've been buried in high-priority bugs for a while. Sorry. I didn't see anything wrong with it on a skim but it doesn't seem to represent what I understood as your interests previously. That is, you mentioned that you were concerned about the ofproto-provider interface. I see that you said that ofpbucket is a component of that interface, which makes sense, but really I'd like to see what the interface is before I make any judgments. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OF1.1+ Groups
On Sun, Aug 18, 2013 at 02:35:05PM +1000, Simon Horman wrote: > On Sun, Aug 04, 2013 at 07:45:38PM -0700, Ben Pfaff wrote: > > On Mon, Aug 05, 2013 at 11:38:57AM +0900, Simon Horman wrote: > > > Hi, > > > > > > I would like to announce my intention to work on OF1.1+ Groups support for > > > Open vSwtich with a particular focus on supporting the fast failover group > > > type. > > > > > > I do not wish to tread on any toes so if anyone is already working on this > > > please let me know. > > > > > > Assuming that is not the case my high-level plan is as follows: > > > > > > 1. Test and as necessary fix > > >"[groups RFC 2/2] Implement OpenFLow 1.1+ "groups" protocol."[1] > > >by Neil Zhu. > > > > I'm working on this myself and made some progress. I'll try to post a > > new version this week. > > Hi Ben, > > I am wondering if you managed to make any progress on this. I was busy with a pile of high-priority bugs and reviews, but now I'm trying to catch up. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
May I ask, what rule is this?
Should we always declare global variable before defining it? or just when
it is enclosed by macros?
Also, when the "!USE_LINKER_SECTIONS", we should modify the coverage.c the
same way. I'll send out a patch,
On Tue, Aug 20, 2013 at 1:44 PM, Ben Pfaff wrote:
> On Mon, Aug 19, 2013 at 12:32:06PM -0700, Alex Wang wrote:
> > Just noticed, when compiling with sparse, it issues the warnings like:
> >
> > """
> > lib/netdev-linux.c:76:1: warning: symbol 'counter_netdev_set_policing'
> was
> > not declared. Should it be static?
> > lib/netdev-linux.c:77:1: warning: symbol 'counter_netdev_arp_lookup' was
> > not declared. Should it be static?
> > lib/netdev-linux.c:78:1: warning: symbol 'counter_netdev_get_ifindex' was
> > not declared. Should it be static?
> > """
>
> Ouch, thanks, I folded in this incremental to fix that:
>
> diff --git a/lib/coverage.h b/lib/coverage.h
> index 73b027a..3d1a115 100644
> --- a/lib/coverage.h
> +++ b/lib/coverage.h
> @@ -54,6 +54,7 @@ struct coverage_counter {
> { \
> *counter_##COUNTER##_get() += n;\
> } \
> +extern struct coverage_counter counter_##COUNTER; \
> struct coverage_counter counter_##COUNTER \
> = { #COUNTER, COUNTER##_count, 0 }; \
> extern struct coverage_counter *counter_ptr_##COUNTER; \
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [clang-atomics 4/4] ovs-thread: Work around apparent Clang bug.
On 20 August 2013 14:10, Ben Pfaff wrote: > Without this patch, I get Clang warnings that I don't understand for each > of the non-static functions in dirs.c: > > warning: control reaches end of non-void function This is a clang bug; in your example the call to bar() is omitted by clang, and the warning is thus actually true. I encountered the bug while looking at other issues in FreeBSD's stdatomic.h and my colleague distilled a simple test case and submitted a bug report[1] for it yesterday. As of this morning it's fixed in clang svn[2]. We're going to bring the fix into FreeBSD's in-tree clang. [1] http://llvm.org/bugs/show_bug.cgi?id=16931 [2] http://llvm.org/viewvc/llvm-project?view=revision&revision=188718 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [clang-atomics 2/4] ovs-atomic: atomic_load() must take a non-const argument.
On 20 August 2013 14:10, Ben Pfaff wrote: > C11 says that atomic_load() requires a non-const argument, and Clang > enforces that. This fixes warnings with FreeBSD that uses > the Clang extensions. Acked-by: Ed Maste ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Sync vxlan tunneling code with upstream ovs-vxlan.
Upstream vxlan implementation was changed according to few comments. Following patch brings back those changes to out of tree ovs module. Signed-off-by: Pravin B Shelar --- datapath/linux/compat/include/net/vxlan.h | 38 +++--- datapath/linux/compat/vxlan.c | 195 - datapath/vport-vxlan.c| 36 +++--- 3 files changed, 112 insertions(+), 157 deletions(-) diff --git a/datapath/linux/compat/include/net/vxlan.h b/datapath/linux/compat/include/net/vxlan.h index 102bc0c..5fc8d54 100644 --- a/datapath/linux/compat/include/net/vxlan.h +++ b/datapath/linux/compat/include/net/vxlan.h @@ -5,35 +5,31 @@ #include #include +#define VNI_HASH_BITS 10 +#define VNI_HASH_SIZE (1sk) == port) + return vs; + } + return NULL; +} + /* Callback from net/ipv4/udp.c to receive packets */ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb) { - struct vxlan_handler *vh; struct vxlan_sock *vs; struct vxlanhdr *vxh; @@ -128,10 +141,8 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb) if (!vs) goto drop; - list_for_each_entry_rcu(vh, &vs->handler_list, node) { - if (vh->rcv(vh, skb, vxh->vx_vni) == PACKET_RCVD) - return 0; - } + vs->rcv(vs, skb, vxh->vx_vni); + return 0; drop: /* Consume bad packet */ @@ -211,7 +222,7 @@ static int handle_offloads(struct sk_buff *skb) return 0; } -int vxlan_xmit_skb(struct net *net, struct vxlan_handler *vh, +int vxlan_xmit_skb(struct net *net, struct vxlan_sock *vs, struct rtable *rt, struct sk_buff *skb, __be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df, __be16 src_port, __be16 dst_port, __be32 vni) @@ -249,7 +260,7 @@ int vxlan_xmit_skb(struct net *net, struct vxlan_handler *vh, uh->len = htons(skb->len); uh->check = 0; - vxlan_set_owner(vh->vs->sock->sk, skb); + vxlan_set_owner(vs->sock->sk, skb); err = handle_offloads(skb); if (err) @@ -259,8 +270,19 @@ int vxlan_xmit_skb(struct net *net, struct vxlan_handler *vh, IPPROTO_UDP, tos, ttl, df); } -static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port) +static void vxlan_del_work(struct work_struct *work) { + struct vxlan_sock *vs = container_of(work, struct vxlan_sock, del_work); + + sk_release_kernel(vs->sock->sk); + kfree_rcu(vs, rcu); + vxlan_cleanup_module(); +} + +static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port, + vxlan_rcv_t *rcv, void *data) +{ + struct vxlan_net *vn = net_generic(net, vxlan_net_id); struct vxlan_sock *vs; struct sock *sk; struct sockaddr_in vxlan_addr = { @@ -269,10 +291,18 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port) .sin_port = port, }; int rc; + unsigned int h; vs = kmalloc(sizeof(*vs), GFP_KERNEL); - if (!vs) + if (!vs) { + pr_debug("memory alocation failure\n"); return ERR_PTR(-ENOMEM); + } + + for (h = 0; h < VNI_HASH_SIZE; ++h) + INIT_HLIST_HEAD(&vs->vni_list[h]); + + INIT_WORK(&vs->del_work, vxlan_del_work); /* Create UDP socket for encapsulation receive. */ rc = sock_create_kern(AF_INET, SOCK_DGRAM, IPPROTO_UDP, &vs->sock); @@ -287,81 +317,37 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port) sk_change_net(sk, net); rc = kernel_bind(vs->sock, (struct sockaddr *) &vxlan_addr, -sizeof(vxlan_addr)); + sizeof(vxlan_addr)); if (rc < 0) { pr_debug("bind for UDP socket %pI4:%u (%d)\n", -&vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc); + &vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc); sk_release_kernel(sk); kfree(vs); return ERR_PTR(rc); } + atomic_set(&vs->refcnt, 1); + vs->rcv = rcv; + vs->data = data; /* Disable multicast loopback */ inet_sk(sk)->mc_loop = 0; - INIT_LIST_HEAD(&vs->handler_list); + spin_lock(&vn->sock_lock); hlist_add_head_rcu(&vs->hlist, vs_head(net, port)); + spin_unlock(&vn->sock_lock); /* Mark socket as an encapsulation socket. */ udp_sk(sk)->encap_type = 1; udp_sk(sk)->encap_rcv = vxlan_udp_encap_recv; udp_encap_enable(); - return vs; } -static void rcu_free_vs_callback(struct rcu_head *rcu) -{ - struct vxlan_sock *vs = container_of(rcu, struct vxlan_sock, rcu); - - kfree(vs); -} - -static void vxlan_socket_de
Re: [ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
Hi Ben,
Thanks for your review!
My comments inline.
I'll send a revised patch shortly for review.
- Original Message -
> From: "Ben Pfaff"
> To: "Romain Lenglet"
> Cc: dev@openvswitch.org
> Sent: Tuesday, August 20, 2013 1:25:16 PM
> Subject: Re: [PATCH] ipfix: implement flow caching and aggregation in exporter
>
> On Mon, Aug 12, 2013 at 11:34:57AM -0700, Romain Lenglet wrote:
> > Implement a per-exporter flow cache with active timeout expiration.
> > Add columns "cache_active_timeout" and "cache_max_flows" into table
> > "IPFIX" to configure each cache.
> >
> > Add per-flow elements "octetDeltaSumOfSquares",
> > "minimumIpTotalLength", and "maximumIpTotalLength" to replace
> > "ethernetTotalLength". Â Add per-flow element "flowEndReason" to
> > indicate whether a flow has expired because of an active timeout, the
> > cache size limit being reached, or the exporter being stopped.
> >
> > Signed-off-by: Romain Lenglet
>
> Thanks for the patch. Â Sorry that it's taken so long to look at this.
>
> "clang" reports:
>
> Â Â ../ofproto/ofproto-dpif-ipfix.c:1456:58: error: variable
> Â Â 'next_timeout_msec' is uninitialized when used here
> Â Â [-Werror,-Wuninitialized]
> Â Â Â Â Â Â Â Â && (!has_next_timeout || temp_timeout_msec <
> Â Â Â Â Â Â Â Â next_timeout_msec)) {
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ^
> Â Â ../ofproto/ofproto-dpif-ipfix.c:1448:36: note: initialize the variable
> Â Â 'next_timeout_msec' to silence this warning
> Â Â Â Â long long int next_timeout_msec, temp_timeout_msec;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ^
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â = 0
>
> You could make it happy, and simplify the code, by initializing
> next_timeout_msec to LLONG_MAX. Â Or you could call
> poll_timer_wait_until(), which is cheap, each place you update
> next_timeout_msec.
Done. Â I'm going with the latter solution, it's nicer.
> Please include just after "ofproto-dpif-ipfix.h" rather
> than just before. Â That way, we can be sure that ofproto-dpif-ipfix.h
> remains self-contained:
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index 8e8e7a2..c37da5f 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -15,15 +15,18 @@
> > Â */
> > Â
> > Â #include
> > +#include
> > Â #include "ofproto-dpif-ipfix.h"
Done.
> The reason for this change isn't obvious. Â But we cannot use
> __kernel_time_t because it is not a portable type (it is Linux kernel
> specific):
> > @@ -41,7 +44,11 @@
> > Â struct dpif_ipfix_exporter {
> > Â Â Â struct collectors *collectors;
> > Â Â Â uint32_t seq_number;
> > - Â Â time_t last_template_set_time;
> > + Â Â __kernel_time_t last_template_set_time;
Done, reverted this change.
> Perhaps on the same topic, this is the first time I've run into
> timercmp(). Â It appears to be a nonstandard BSDism. Â Is there a reason
> to use "struct timeval" instead of the "long long int as msecs from
> the epoch" that we usually use in OVS? Â The latter is nice, when you
> can use it, because arithmetic and relational operators are meaningful
> and simple.
I send microsecond-granularity timestamps in the IPFIX flow records, and
only timeval provides values at that granularity.
Even though it doesn't matter much right now since timestamping is done
in userspace and is imprecise, I'd like to be able to support precise
(e.g. hardware, or at least kernelspace) timestamping later.
> I see that this patch adds multiple consecutive blank lines in a few
> places. Â We don't usually do that.
Done.
> In ipfix_cache_next_timeout_msec(), usually I put a space just after
> LIST_FOR_EACH (because I want it to look like a statement, not a
> function call).
Done.
> I see that ipfix_cache_entry_init() uses ofpbuf_use_stub() and then
> later asserts that the buffer was not reallocated. Â If that is the
> behavior that you want, then you can use ofpbuf_use_stack(), which
> will assert-fail immediately when reallocation is attempted.
Done. Yes, that's the behavior I want and I switched to using
ofpbuf_use_stack(). Thanks for the suggestion.
I've also redefined the flow_key_msg_part buffer member to be a
uint64_t array to ensure proper alignment.
> 70 minutes is a curious number to choose as the maximum for
> cache_active_timeout. Â Is this the maximum value allowed by IPFIX?
This is because I send timestamps as unsigned 32-bit deltas in
microseconds. This corresponds to ~71.5 minutes.
That's a limit in the standard:
http://tools.ietf.org/html/rfc5102#section-5.9
That constraint on the timestamps directly translates into a
constraint on the cache timeouts.
--
Romain Lenglet
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] bfd: Implement BFD decay.
Merged. I changed the variable name from ctrl to ctl to be consistent
with the bfd spec's convention.
Ethan
On Tue, Aug 20, 2013 at 8:41 AM, Alex Wang 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 may usually be idle for a long time.
> And cpu consumption can be reduced by processing fewer bfd control
> packets.
>
> Signed-off-by: Alex Wang
> ---
> lib/bfd.c | 134 ++-
> lib/bfd.h |5 +-
> ofproto/ofproto-dpif.c |7 +-
> tests/bfd.at | 279
> ++--
> vswitchd/vswitch.xml | 10 ++
> 5 files changed, 419 insertions(+), 16 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 6f86f26..43ef759 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -28,6 +28,7 @@
> #include "hash.h"
> #include "hmap.h"
> #include "list.h"
> +#include "netdev.h"
> #include "netlink.h"
> #include "odp-util.h"
> #include "ofpbuf.h"
> @@ -151,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. */
>
> @@ -186,6 +190,14 @@ struct bfd {
>
> atomic_bool check_tnl_key;/* Verify tunnel key of inbound packets? */
> atomic_int ref_cnt;
> +
> +/* BFD decay related variables. */
> +bool in_decay;/* True when bfd is in decay. */
> +int decay_min_rx; /* min_rx is set to decay_min_rx when */
> + /* in decay. */
> +int decay_rx_ctrl;/* Count bfd packets received within decay
> */
> + /* detect interval. */
> +long long int decay_detect_time; /* Decay detection time. */
> };
>
> static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
> @@ -208,6 +220,9 @@ static void bfd_set_state(struct bfd *, enum state, enum
> diag)
> static uint32_t generate_discriminator(void) OVS_REQUIRES(mutex);
> static void bfd_put_details(struct ds *, const struct bfd *)
> OVS_REQUIRES(mutex);
> +static uint64_t bfd_rx_packets(const struct bfd *) OVS_REQUIRES(mutex);
> +static void bfd_try_decay(struct bfd *) OVS_REQUIRES(mutex);
> +static void bfd_decay_update(struct bfd *) OVS_REQUIRES(mutex);
> static void bfd_unixctl_show(struct unixctl_conn *, int argc,
> const char *argv[], void *aux OVS_UNUSED);
> static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
> @@ -255,14 +270,16 @@ 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)
> -OVS_EXCLUDED(mutex)
> +bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
> + struct netdev *netdev) OVS_EXCLUDED(mutex)
> {
> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> static atomic_uint16_t udp_src = ATOMIC_VAR_INIT(0);
>
> +int decay_min_rx;
> long long int min_tx, min_rx;
> bool need_poll = false;
> +bool cfg_min_rx_changed = false;
> bool cpath_down;
> const char *hwaddr;
> uint8_t ea[ETH_ADDR_LEN];
> @@ -293,6 +310,8 @@ bfd_configure(struct bfd *bfd, const char *name, const
> struct smap *cfg)
> bfd->min_tx = 1000;
> bfd->mult = 3;
> atomic_init(&bfd->ref_cnt, 1);
> +bfd->netdev = netdev_ref(netdev);
> +bfd->in_decay = false;
>
> /* RFC 5881 section 4
> * The source port MUST be in the range 49152 through 65535. The
> same
> @@ -328,6 +347,22 @@ bfd_configure(struct bfd *bfd, const char *name, const
> struct smap *cfg)
> || (!bfd_in_poll(bfd) && bfd->cfg_min_rx > bfd->min_rx)) {
> bfd->min_rx = bfd->cfg_min_rx;
> }
> +cfg_min_rx_changed = true;
> +need_poll = true;
> +}
> +
> +decay_min_rx = smap_get_int(cfg, "decay_min_rx", 0);
> +if (bfd->decay_min_rx != decay_min_rx || cfg_min_rx_changed) {
> +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;
> +}
> +/* Resets decay. */
> +bfd->in_decay = false;
> +bfd_decay_update(bfd);
> need_poll = true;
> }
>
> @@ -379,6 +414,7 @@ bfd_unref(struct bfd *bfd) OVS_EXCLUDED(mutex)
>
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
I found the answer from this thread:
http://openvswitch.org/pipermail/dev/2011-May/008608.html
On Tue, Aug 20, 2013 at 2:02 PM, Alex Wang wrote:
> May I ask, what rule is this?
>
> Should we always declare global variable before defining it? or just when
> it is enclosed by macros?
>
> Also, when the "!USE_LINKER_SECTIONS", we should modify the coverage.c the
> same way. I'll send out a patch,
>
>
> On Tue, Aug 20, 2013 at 1:44 PM, Ben Pfaff wrote:
>
>> On Mon, Aug 19, 2013 at 12:32:06PM -0700, Alex Wang wrote:
>> > Just noticed, when compiling with sparse, it issues the warnings like:
>> >
>> > """
>> > lib/netdev-linux.c:76:1: warning: symbol 'counter_netdev_set_policing'
>> was
>> > not declared. Should it be static?
>> > lib/netdev-linux.c:77:1: warning: symbol 'counter_netdev_arp_lookup' was
>> > not declared. Should it be static?
>> > lib/netdev-linux.c:78:1: warning: symbol 'counter_netdev_get_ifindex'
>> was
>> > not declared. Should it be static?
>> > """
>>
>> Ouch, thanks, I folded in this incremental to fix that:
>>
>> diff --git a/lib/coverage.h b/lib/coverage.h
>> index 73b027a..3d1a115 100644
>> --- a/lib/coverage.h
>> +++ b/lib/coverage.h
>> @@ -54,6 +54,7 @@ struct coverage_counter {
>> { \
>> *counter_##COUNTER##_get() += n;\
>> } \
>> +extern struct coverage_counter counter_##COUNTER; \
>> struct coverage_counter counter_##COUNTER \
>> = { #COUNTER, COUNTER##_count, 0 }; \
>> extern struct coverage_counter *counter_ptr_##COUNTER; \
>>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] sparse: Suppress sparse warnings for global variables.
sparse warns if a non-static variable with external linkage has an
initializer at first declaration. This commit suppresses the
warnings issued when adding custom section is not supported by
compiler.
Signed-off-by: Alex Wang
---
lib/coverage.c |2 ++
lib/vlog.c |1 +
2 files changed, 3 insertions(+)
diff --git a/lib/coverage.c b/lib/coverage.c
index 82ce85b..23e2997 100644
--- a/lib/coverage.c
+++ b/lib/coverage.c
@@ -46,11 +46,13 @@ extern struct coverage_counter *__stop_coverage[];
*countp = 0;\
return count; \
} \
+extern struct coverage_counter counter_##COUNTER; \
struct coverage_counter counter_##COUNTER \
= { #COUNTER, COUNTER##_count, 0 };
#include "coverage.def"
#undef COVERAGE_COUNTER
+extern struct coverage_counter *coverage_counters[];
struct coverage_counter *coverage_counters[] = {
#define COVERAGE_COUNTER(NAME) &counter_##NAME,
#include "coverage.def"
diff --git a/lib/vlog.c b/lib/vlog.c
index 061250a..a267112 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -74,6 +74,7 @@ extern struct vlog_module *__stop_vlog_modules[];
#include "vlog-modules.def"
#undef VLOG_MODULE
+extern struct vlog_module *vlog_modules[];
struct vlog_module *vlog_modules[] = {
#define VLOG_MODULE(NAME) &VLM_##NAME,
#include "vlog-modules.def"
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
Implement a per-exporter flow cache with active timeout expiration.
Add columns "cache_active_timeout" and "cache_max_flows" into table
"IPFIX" to configure each cache.
Add per-flow elements "octetDeltaSumOfSquares",
"minimumIpTotalLength", and "maximumIpTotalLength" to replace
"ethernetTotalLength". Add per-flow element "flowEndReason" to
indicate whether a flow has expired because of an active timeout, the
cache size limit being reached, or the exporter being stopped.
Signed-off-by: Romain Lenglet
---
ofproto/ofproto-dpif-ipfix.c | 746 +-
ofproto/ofproto-dpif-ipfix.h |3 +
ofproto/ofproto-dpif.c | 23 +-
ofproto/ofproto.h|4 +
utilities/ovs-vsctl.8.in |5 +-
vswitchd/bridge.c| 10 +
vswitchd/vswitch.ovsschema | 12 +-
vswitchd/vswitch.xml | 12 +
8 files changed, 716 insertions(+), 99 deletions(-)
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 8e8e7a2..c3061c9 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -16,14 +16,17 @@
#include
#include "ofproto-dpif-ipfix.h"
+#include
#include "byte-order.h"
#include "collectors.h"
#include "flow.h"
#include "hash.h"
#include "hmap.h"
+#include "list.h"
#include "ofpbuf.h"
#include "ofproto.h"
#include "packets.h"
+#include "poll-loop.h"
#include "sset.h"
#include "util.h"
#include "timeval.h"
@@ -42,6 +45,10 @@ struct dpif_ipfix_exporter {
struct collectors *collectors;
uint32_t seq_number;
time_t last_template_set_time;
+struct hmap cache_flow_key_map; /* ipfix_flow_cache_entry. */
+struct list cache_flow_start_timestamp_list; /* ipfix_flow_cache_entry. */
+uint32_t cache_active_timeout; /* In seconds. */
+uint32_t cache_max_flows;
};
struct dpif_ipfix_bridge_exporter {
@@ -62,7 +69,7 @@ struct dpif_ipfix_flow_exporter_map_node {
struct dpif_ipfix {
struct dpif_ipfix_bridge_exporter bridge_exporter;
-struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_nodes. */
+struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_node. */
atomic_int ref_cnt;
};
@@ -143,32 +150,30 @@ struct ipfix_template_field_specifier {
});
BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 4);
-/* Part of data record for common metadata and Ethernet entities. */
+/* Part of data record flow key for common metadata and Ethernet entities. */
OVS_PACKED(
-struct ipfix_data_record_common {
+struct ipfix_data_record_flow_key_common {
ovs_be32 observation_point_id; /* OBSERVATION_POINT_ID */
-ovs_be64 packet_delta_count; /* PACKET_DELTA_COUNT */
-ovs_be64 layer2_octet_delta_count; /* LAYER2_OCTET_DELTA_COUNT */
uint8_t source_mac_address[6]; /* SOURCE_MAC_ADDRESS */
uint8_t destination_mac_address[6]; /* DESTINATION_MAC_ADDRESS */
ovs_be16 ethernet_type; /* ETHERNET_TYPE */
-ovs_be16 ethernet_total_length; /* ETHERNET_TOTAL_LENGTH */
uint8_t ethernet_header_length; /* ETHERNET_HEADER_LENGTH */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_common) == 37);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 19);
-/* Part of data record for VLAN entities. */
+/* Part of data record flow key for VLAN entities. */
OVS_PACKED(
-struct ipfix_data_record_vlan {
+struct ipfix_data_record_flow_key_vlan {
ovs_be16 vlan_id; /* VLAN_ID */
ovs_be16 dot1q_vlan_id; /* DOT1Q_VLAN_ID */
uint8_t dot1q_priority; /* DOT1Q_PRIORITY */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_vlan) == 5);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_vlan) == 5);
-/* Part of data record for IP entities. */
+/* Part of data record flow key for IP entities. */
+/* XXX: Replace IP_TTL with MINIMUM_TTL and MAXIMUM_TTL? */
OVS_PACKED(
-struct ipfix_data_record_ip {
+struct ipfix_data_record_flow_key_ip {
uint8_t ip_version; /* IP_VERSION */
uint8_t ip_ttl; /* IP_TTL */
uint8_t protocol_identifier; /* PROTOCOL_IDENTIFIER */
@@ -176,32 +181,117 @@ struct ipfix_data_record_ip {
uint8_t ip_precedence; /* IP_PRECEDENCE */
uint8_t ip_class_of_service; /* IP_CLASS_OF_SERVICE */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_ip) == 6);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_ip) == 6);
-/* Part of data record for IPv4 entities. */
+/* Part of data record flow key for IPv4 entities. */
OVS_PACKED(
-struct ipfix_data_record_ipv4 {
+struct ipfix_data_record_flow_key_ipv4 {
ovs_be32 source_ipv4_address; /* SOURCE_IPV4_ADDRESS */
ovs_be32 destination_ipv4_address; /* DESTINATION_IPV4_ADDRESS */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_ipv4) == 8);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_ipv4) == 8);
-/* Part of data record for IPv4 entities. */
+/* Part of data record flow key for IPv6 entities. */
OVS_PACKED(
-struct ipfix_dat
Re: [ovs-dev] [PATCH 2/2] bfd: Implements forwarding_if_rx
Does this patch work while decay mode is on? Won't the fact that we
share rx_packets between the two features, and that it's set so
frequently by the forwarding_if_rx feature, break the decay mode
feature's assumption that it's the only one setting it? I'm surprised
this doesn't break the unit tests. Perhaps we should add one that
runs decay mode and forwarding_if_rx in conjunctin?
> +/* When forward_if_rx is true, the bfd_forwarding() will
s/the bfd_forwarding()/bfd_forwarding()
> + * return true as long as there are incoming packets received.
> + * Note, forwarding_override still has higher priority. */
> +bool forwarding_if_rx;
> +long long int forwarding_if_rx_detect_time;
> +
> /* BFD decay related variables. */
> bool in_decay;/* True when bfd is in decay. */
> int decay_min_rx; /* min_rx is set to decay_min_rx when */
>/* in decay. */
> -int decay_rx_ctrl;/* Count bfd packets received within decay
> */
> +uint64_t decay_rx_ctrl; /* Count bfd packets received within decay
> */
>/* detect interval. */
> +uint64_t decay_rx_packets;/* Packets received by 'netdev'. */
decay_rx_packets is unused. Also, if changing the type of
decay_rx_ctl is necessary, it should probably be in a separate patch.
> @@ -753,6 +776,7 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
> *netdev)
> if (bfd->decay_min_rx) {
> bfd_decay_update(bfd);
> }
> +bfd->rx_packets = bfd_rx_packets(bfd);
we should probably reset the detect time here as well.
> +if (diff < 0) {
> +VLOG_WARN("rx_packets count is smaller than last time.");
It's probably worth rate limiting this log message. I suspect if it
happens it might happen frequently.
> +
> + When forwarding_if_rx is true the forwarding
> + field in "ovs-appctl bfd/show" 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.
> +
I think the forwarding field in the database bfd_status column is far
more important than that in the appctl output which is mostly for
developers to debug things. I'd rewrite this and the commit message
to mention that instead. Also both here and in the comment message, I
would talk about "interfaces" instead of "tunnels". BFD can be run
on any kind of interface, so there's no reason to restrict it in the
documentation.
Ethan
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
On Tue, Aug 20, 2013 at 02:34:40PM -0700, Romain Lenglet wrote: > > From: "Ben Pfaff" > > Perhaps on the same topic, this is the first time I've run into > > timercmp(). ??It appears to be a nonstandard BSDism. ??Is there a reason > > to use "struct timeval" instead of the "long long int as msecs from > > the epoch" that we usually use in OVS? ??The latter is nice, when you > > can use it, because arithmetic and relational operators are meaningful > > and simple. > > I send microsecond-granularity timestamps in the IPFIX flow records, and > only timeval provides values at that granularity. > Even though it doesn't matter much right now since timestamping is done > in userspace and is imprecise, I'd like to be able to support precise > (e.g. hardware, or at least kernelspace) timestamping later. What's the motivation, that is, why does anyone care when a packet arrived to a precision beyond 1 ms? Anyway, if we really need this, I can think of a couple of possibilities. One is to define a timeval_compare_3way() in timeval.[ch] that returns a strcmp()-like return value. Another is to use a long long int count of usecs instead of msecs. > > 70 minutes is a curious number to choose as the maximum for > > cache_active_timeout. ??Is this the maximum value allowed by IPFIX? > > This is because I send timestamps as unsigned 32-bit deltas in > microseconds. This corresponds to ~71.5 minutes. > That's a limit in the standard: > http://tools.ietf.org/html/rfc5102#section-5.9 > > That constraint on the timestamps directly translates into a > constraint on the cache timeouts. That makes sense. Thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [clang-atomics 4/4] ovs-thread: Work around apparent Clang bug.
On Tue, Aug 20, 2013 at 05:05:26PM -0400, Ed Maste wrote: > On 20 August 2013 14:10, Ben Pfaff wrote: > > Without this patch, I get Clang warnings that I don't understand for each > > of the non-static functions in dirs.c: > > > > warning: control reaches end of non-void function > > This is a clang bug; in your example the call to bar() is omitted by > clang, and the warning is thus actually true. I encountered the bug > while looking at other issues in FreeBSD's stdatomic.h and my > colleague distilled a simple test case and submitted a bug report[1] > for it yesterday. As of this morning it's fixed in clang svn[2]. Thanks, that's good to know. I'm using a Clang nightly build, so I'll upgrade tomorrow and see if it fixes the problem for me, and I'll plan to drop this patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Enable smooth transition between CFM and BFD.
Merged thanks.
Like an idiot I forgot to put my signed-off-by so I'll put it here.
Signed-off-by: Ethan Jackson
Ethan
On Tue, Aug 20, 2013 at 11:45 AM, Alex Wang wrote:
> When user switches between using CFM and BFD, there will be a short
> down time before the new protocol goes up. This can unintentionally
> change the traffic pattern of the bundled ports. To prevent this,
> it is proposed that user can enable both CFM and BFD before transition,
> wait for the new protocol to go up, and then disable the old protocol.
>
> To make this scheme work, this commit modifies the port_run() in
> ofproto-dpif.c, so that when both CFM and BFD are used, if either shows
> correct status, the port is considered usable in the bundle.
>
> Signed-off-by: Alex Wang
> ---
> ofproto/ofproto-dpif.c | 12 +--
> tests/ofproto-dpif.at | 81
>
> 2 files changed, 90 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4690215..9fe91c1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2952,6 +2952,8 @@ port_run(struct ofport_dpif *ofport)
> long long int carrier_seq = netdev_get_carrier_resets(ofport->up.netdev);
> bool carrier_changed = carrier_seq != ofport->carrier_seq;
> bool enable = netdev_get_carrier(ofport->up.netdev);
> +bool cfm_enable = false;
> +bool bfd_enable = false;
>
> ofport->carrier_seq = carrier_seq;
>
> @@ -2961,16 +2963,20 @@ port_run(struct ofport_dpif *ofport)
> int cfm_opup = cfm_get_opup(ofport->cfm);
>
> cfm_run(ofport->cfm);
> -enable = enable && !cfm_get_fault(ofport->cfm);
> +cfm_enable = !cfm_get_fault(ofport->cfm);
>
> if (cfm_opup >= 0) {
> -enable = enable && cfm_opup;
> +cfm_enable = cfm_enable && cfm_opup;
> }
> }
>
> if (ofport->bfd) {
> bfd_run(ofport->bfd);
> -enable = enable && bfd_forwarding(ofport->bfd);
> +bfd_enable = bfd_forwarding(ofport->bfd);
> +}
> +
> +if (ofport->bfd || ofport->cfm) {
> +enable = enable && (cfm_enable || bfd_enable);
> }
>
> if (ofport->bundle) {
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b093998..4772416 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -2689,3 +2689,84 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: 5
> ])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +# Tests the bundling with various bfd and cfm configurations.
> +AT_SETUP([ofproto - bundle with variable bfd/cfm config])
> +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy -- \
> +add-bond br0 br0bond p0 p2 bond-mode=active-backup -- \
> +add-bond br1 br1bond p1 p3 bond-mode=active-backup -- \
> +set Interface p1 type=patch options:peer=p0
> ofport_request=2 -- \
> +set Interface p3 type=patch options:peer=p2
> ofport_request=4 -- \
> +set Interface p0 type=patch options:peer=p1
> ofport_request=1 -- \
> +set Interface p2 type=patch options:peer=p3
> ofport_request=3 -- \
> +set Interface p0 bfd:enable=true bfd:min_tx=300
> bfd:min_rx=300 -- \
> +set Interface p0 cfm_mpid=1 -- \
> +set Interface p1 bfd:enable=true bfd:min_tx=500
> bfd:min_rx=500])
> +
> +ovs-appctl time/stop
> +# advance the clock to stablize everything.
> +for i in `seq 0 49`; do ovs-appctl time/warp 100; done
> +# cfm/show should show 'recv' fault.
> +AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
> + fault: recv
> +])
> +# bfd/show should show 'up'.
> +AT_CHECK([ovs-appctl bfd/show | sed -n '/^.*Session State:.*/p'], [0], [dnl
> + Local Session State: up
> + Remote Session State: up
> + Local Session State: up
> + Remote Session State: up
> +])
> +# bond/show should show 'may-enable: true' for all slaves.
> +AT_CHECK([ovs-appctl bond/show | sed -n '/^.*may_enable:.*/p'], [0], [dnl
> + may_enable: true
> + may_enable: true
> + may_enable: true
> + may_enable: true
> +])
> +
> +# now disable the bfd on p1.
> +AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=false])
> +# advance the clock to stablize everything.
> +for i in `seq 0 49`; do ovs-appctl time/warp 100; done
> +# cfm/show should show 'recv' fault.
> +AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
> + fault: recv
> +])
> +# bfd/show should show 'down'.
> +AT_CHECK([ovs-appctl bfd/show | sed -n '/^.*Session State:.*/p'], [0], [dnl
> + Local Session State: down
> + Remote Session State: down
> +])
> +# bond/show should show 'may-enable: false' for p0.
> +AT_CHECK([ovs-appctl bond/show | sed -n '/^.*may_enable:.*/p'], [0], [dnl
> + may_enable: false
> + may_enable: true
> + may_enable: tru
Re: [ovs-dev] [PATCH] ofproto-dpif: Enable smooth transition between CFM and BFD.
Thanks a lot! ;D
On Tue, Aug 20, 2013 at 3:12 PM, Ethan Jackson wrote:
> Merged thanks.
>
> Like an idiot I forgot to put my signed-off-by so I'll put it here.
>
> Signed-off-by: Ethan Jackson
>
> Ethan
>
> On Tue, Aug 20, 2013 at 11:45 AM, Alex Wang wrote:
> > When user switches between using CFM and BFD, there will be a short
> > down time before the new protocol goes up. This can unintentionally
> > change the traffic pattern of the bundled ports. To prevent this,
> > it is proposed that user can enable both CFM and BFD before transition,
> > wait for the new protocol to go up, and then disable the old protocol.
> >
> > To make this scheme work, this commit modifies the port_run() in
> > ofproto-dpif.c, so that when both CFM and BFD are used, if either shows
> > correct status, the port is considered usable in the bundle.
> >
> > Signed-off-by: Alex Wang
> > ---
> > ofproto/ofproto-dpif.c | 12 +--
> > tests/ofproto-dpif.at | 81
>
> > 2 files changed, 90 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 4690215..9fe91c1 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -2952,6 +2952,8 @@ port_run(struct ofport_dpif *ofport)
> > long long int carrier_seq =
> netdev_get_carrier_resets(ofport->up.netdev);
> > bool carrier_changed = carrier_seq != ofport->carrier_seq;
> > bool enable = netdev_get_carrier(ofport->up.netdev);
> > +bool cfm_enable = false;
> > +bool bfd_enable = false;
> >
> > ofport->carrier_seq = carrier_seq;
> >
> > @@ -2961,16 +2963,20 @@ port_run(struct ofport_dpif *ofport)
> > int cfm_opup = cfm_get_opup(ofport->cfm);
> >
> > cfm_run(ofport->cfm);
> > -enable = enable && !cfm_get_fault(ofport->cfm);
> > +cfm_enable = !cfm_get_fault(ofport->cfm);
> >
> > if (cfm_opup >= 0) {
> > -enable = enable && cfm_opup;
> > +cfm_enable = cfm_enable && cfm_opup;
> > }
> > }
> >
> > if (ofport->bfd) {
> > bfd_run(ofport->bfd);
> > -enable = enable && bfd_forwarding(ofport->bfd);
> > +bfd_enable = bfd_forwarding(ofport->bfd);
> > +}
> > +
> > +if (ofport->bfd || ofport->cfm) {
> > +enable = enable && (cfm_enable || bfd_enable);
> > }
> >
> > if (ofport->bundle) {
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index b093998..4772416 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -2689,3 +2689,84 @@ AT_CHECK([tail -1 stdout], [0], [Datapath
> actions: 5
> > ])
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> > +
> > +# Tests the bundling with various bfd and cfm configurations.
> > +AT_SETUP([ofproto - bundle with variable bfd/cfm config])
> > +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy --
> \
> > +add-bond br0 br0bond p0 p2 bond-mode=active-backup
> -- \
> > +add-bond br1 br1bond p1 p3 bond-mode=active-backup
> -- \
> > +set Interface p1 type=patch options:peer=p0
> ofport_request=2 -- \
> > +set Interface p3 type=patch options:peer=p2
> ofport_request=4 -- \
> > +set Interface p0 type=patch options:peer=p1
> ofport_request=1 -- \
> > +set Interface p2 type=patch options:peer=p3
> ofport_request=3 -- \
> > +set Interface p0 bfd:enable=true bfd:min_tx=300
> bfd:min_rx=300 -- \
> > +set Interface p0 cfm_mpid=1 -- \
> > +set Interface p1 bfd:enable=true bfd:min_tx=500
> bfd:min_rx=500])
> > +
> > +ovs-appctl time/stop
> > +# advance the clock to stablize everything.
> > +for i in `seq 0 49`; do ovs-appctl time/warp 100; done
> > +# cfm/show should show 'recv' fault.
> > +AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
> > + fault: recv
> > +])
> > +# bfd/show should show 'up'.
> > +AT_CHECK([ovs-appctl bfd/show | sed -n '/^.*Session State:.*/p'], [0],
> [dnl
> > + Local Session State: up
> > + Remote Session State: up
> > + Local Session State: up
> > + Remote Session State: up
> > +])
> > +# bond/show should show 'may-enable: true' for all slaves.
> > +AT_CHECK([ovs-appctl bond/show | sed -n '/^.*may_enable:.*/p'], [0],
> [dnl
> > + may_enable: true
> > + may_enable: true
> > + may_enable: true
> > + may_enable: true
> > +])
> > +
> > +# now disable the bfd on p1.
> > +AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=false])
> > +# advance the clock to stablize everything.
> > +for i in `seq 0 49`; do ovs-appctl time/warp 100; done
> > +# cfm/show should show 'recv' fault.
> > +AT_CHECK([ovs-appctl cfm/show | sed -n '/^.*fault:.*/p'], [0], [dnl
> > + fault: recv
> > +])
> > +# bfd/show should show 'down'.
> > +AT_CHECK([ovs-appctl bfd/show | sed -n '/^.*Session State
Re: [ovs-dev] [clang-atomics 2/4] ovs-atomic: atomic_load() must take a non-const argument.
On Tue, Aug 20, 2013 at 05:09:43PM -0400, Ed Maste wrote: > On 20 August 2013 14:10, Ben Pfaff wrote: > > C11 says that atomic_load() requires a non-const argument, and Clang > > enforces that. This fixes warnings with FreeBSD that uses > > the Clang extensions. > > Acked-by: Ed Maste Applied, thanks for the review. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OF1.1+ Groups
OK, thanks. I'll send a follow-on with the provider header change that uses the buckets later this week. (Likewise buried in high-priority bugs...) On Tue, Aug 20, 2013 at 1:51 PM, Ben Pfaff wrote: > On Tue, Aug 13, 2013 at 11:15:58AM -0700, Casey Barker wrote: > > I've got a lot more code I can upstream, but I haven't seen any reviews > for > > the first patch. Any comments? (Or did I submit it incorrectly?) > > You sent it right but I've been buried in high-priority bugs for a > while. Sorry. > > I didn't see anything wrong with it on a skim but it doesn't seem to > represent what I understood as your interests previously. That is, > you mentioned that you were concerned about the ofproto-provider > interface. I see that you said that ofpbucket is a component of that > interface, which makes sense, but really I'd like to see what the > interface is before I make any judgments. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] bfd: Implements forwarding_if_rx
Thanks Ethan for the review,
On Tue, Aug 20, 2013 at 3:07 PM, Ethan Jackson wrote:
> Does this patch work while decay mode is on? Won't the fact that we
> share rx_packets between the two features, and that it's set so
> frequently by the forwarding_if_rx feature, break the decay mode
> feature's assumption that it's the only one setting it? I'm surprised
> this doesn't break the unit tests. Perhaps we should add one that
> runs decay mode and forwarding_if_rx in conjunctin?
I don't understand. I have used a separate decay_rx_packets to record the
rx_packets stats for bfd decay. So, rx_packets is not shared.
But I'll definitely add an unit test running the two together.
> > +uint64_t decay_rx_ctrl; /* Count bfd packets received within
> decay */
> >/* detect interval. */
> > +uint64_t decay_rx_packets;/* Packets received by 'netdev'. */
>
> decay_rx_packets is unused. Also, if changing the type of
> decay_rx_ctl is necessary, it should probably be in a separate patch.
>
>
This is a typo. It is not necessary to change the type of decay_rx_ctl.
Also, I think I use the decay_rx_packets in the bfd_try_decay() and
bfd_decay_update().
>
> > @@ -753,6 +776,7 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
> *netdev)
> > if (bfd->decay_min_rx) {
> > bfd_decay_update(bfd);
> > }
> > +bfd->rx_packets = bfd_rx_packets(bfd);
>
> we should probably reset the detect time here as well.
>
>
> > +if (diff < 0) {
> > +VLOG_WARN("rx_packets count is smaller than last time.");
>
> It's probably worth rate limiting this log message. I suspect if it
> happens it might happen frequently.
>
>
I'll adjust accordingly. Thanks,
>
I think the forwarding field in the database bfd_status column is far
> more important than that in the appctl output which is mostly for
> developers to debug things. I'd rewrite this and the commit message
> to mention that instead. Also both here and in the comment message, I
> would talk about "interfaces" instead of "tunnels". BFD can be run
> on any kind of interface, so there's no reason to restrict it in the
> documentation.
>
>
Thanks a lot, I agree (especially after I know how it will affect the
bundle ports). I'll redo them accordingly.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
To expand on that, usually one defines a non-static variable, in a .c
file, only when an "extern" declaration of the same variable is
already visible from #including an .h file. A definition without a
previous declaration usually means that the .h file was forgotten.
On Tue, Aug 20, 2013 at 02:48:57PM -0700, Alex Wang wrote:
> I found the answer from this thread:
> http://openvswitch.org/pipermail/dev/2011-May/008608.html
>
>
>
> On Tue, Aug 20, 2013 at 2:02 PM, Alex Wang wrote:
>
> > May I ask, what rule is this?
> >
> > Should we always declare global variable before defining it? or just when
> > it is enclosed by macros?
> >
> > Also, when the "!USE_LINKER_SECTIONS", we should modify the coverage.c the
> > same way. I'll send out a patch,
> >
> >
> > On Tue, Aug 20, 2013 at 1:44 PM, Ben Pfaff wrote:
> >
> >> On Mon, Aug 19, 2013 at 12:32:06PM -0700, Alex Wang wrote:
> >> > Just noticed, when compiling with sparse, it issues the warnings like:
> >> >
> >> > """
> >> > lib/netdev-linux.c:76:1: warning: symbol 'counter_netdev_set_policing'
> >> was
> >> > not declared. Should it be static?
> >> > lib/netdev-linux.c:77:1: warning: symbol 'counter_netdev_arp_lookup' was
> >> > not declared. Should it be static?
> >> > lib/netdev-linux.c:78:1: warning: symbol 'counter_netdev_get_ifindex'
> >> was
> >> > not declared. Should it be static?
> >> > """
> >>
> >> Ouch, thanks, I folded in this incremental to fix that:
> >>
> >> diff --git a/lib/coverage.h b/lib/coverage.h
> >> index 73b027a..3d1a115 100644
> >> --- a/lib/coverage.h
> >> +++ b/lib/coverage.h
> >> @@ -54,6 +54,7 @@ struct coverage_counter {
> >> { \
> >> *counter_##COUNTER##_get() += n;\
> >> } \
> >> +extern struct coverage_counter counter_##COUNTER; \
> >> struct coverage_counter counter_##COUNTER \
> >> = { #COUNTER, COUNTER##_count, 0 }; \
> >> extern struct coverage_counter *counter_ptr_##COUNTER; \
> >>
> >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] sparse: Suppress sparse warnings for global variables.
On Tue, Aug 20, 2013 at 03:24:19PM -0700, Alex Wang wrote: > sparse warns if a non-static variable with external linkage has an > initializer at first declaration. This commit suppresses the > warnings issued when adding custom section is not supported by > compiler. > > Signed-off-by: Alex Wang Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] bfd: Implements forwarding_if_rx
> I don't understand. I have used a separate decay_rx_packets to record the
> rx_packets stats for bfd decay. So, rx_packets is not shared.
hmm I think you're right. I must have messed up rebasing.
Ethan
>
> But I'll definitely add an unit test running the two together.
>
>
>>
>> > +uint64_t decay_rx_ctrl; /* Count bfd packets received within
>> > decay */
>> >/* detect interval. */
>> > +uint64_t decay_rx_packets;/* Packets received by 'netdev'. */
>>
>> decay_rx_packets is unused. Also, if changing the type of
>> decay_rx_ctl is necessary, it should probably be in a separate patch.
>>
>
>
> This is a typo. It is not necessary to change the type of decay_rx_ctl.
> Also, I think I use the decay_rx_packets in the bfd_try_decay() and
> bfd_decay_update().
>
>
>>
>>
>> > @@ -753,6 +776,7 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
>> > *netdev)
>> > if (bfd->decay_min_rx) {
>> > bfd_decay_update(bfd);
>> > }
>> > +bfd->rx_packets = bfd_rx_packets(bfd);
>>
>> we should probably reset the detect time here as well.
>>
>>
>> > +if (diff < 0) {
>> > +VLOG_WARN("rx_packets count is smaller than last time.");
>>
>> It's probably worth rate limiting this log message. I suspect if it
>> happens it might happen frequently.
>>
>
>
> I'll adjust accordingly. Thanks,
>
>
>>
>>
>> I think the forwarding field in the database bfd_status column is far
>> more important than that in the appctl output which is mostly for
>> developers to debug things. I'd rewrite this and the commit message
>> to mention that instead. Also both here and in the comment message, I
>> would talk about "interfaces" instead of "tunnels". BFD can be run
>> on any kind of interface, so there's no reason to restrict it in the
>> documentation.
>>
>
> Thanks a lot, I agree (especially after I know how it will affect the bundle
> ports). I'll redo them accordingly.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] bfd: Implements forwarding_if_rx
Np, i'll adjust accordingly and send a v2 soon,
with the conjunction test.
On Tue, Aug 20, 2013 at 3:53 PM, Ethan Jackson wrote:
> > I don't understand. I have used a separate decay_rx_packets to record the
> > rx_packets stats for bfd decay. So, rx_packets is not shared.
>
> hmm I think you're right. I must have messed up rebasing.
>
> Ethan
>
> >
> > But I'll definitely add an unit test running the two together.
> >
> >
> >>
> >> > +uint64_t decay_rx_ctrl; /* Count bfd packets received
> within
> >> > decay */
> >> >/* detect interval. */
> >> > +uint64_t decay_rx_packets;/* Packets received by 'netdev'. */
> >>
> >> decay_rx_packets is unused. Also, if changing the type of
> >> decay_rx_ctl is necessary, it should probably be in a separate patch.
> >>
> >
> >
> > This is a typo. It is not necessary to change the type of decay_rx_ctl.
> > Also, I think I use the decay_rx_packets in the bfd_try_decay() and
> > bfd_decay_update().
> >
> >
> >>
> >>
> >> > @@ -753,6 +776,7 @@ bfd_set_netdev(struct bfd *bfd, const struct
> netdev
> >> > *netdev)
> >> > if (bfd->decay_min_rx) {
> >> > bfd_decay_update(bfd);
> >> > }
> >> > +bfd->rx_packets = bfd_rx_packets(bfd);
> >>
> >> we should probably reset the detect time here as well.
> >>
> >>
> >> > +if (diff < 0) {
> >> > +VLOG_WARN("rx_packets count is smaller than last time.");
> >>
> >> It's probably worth rate limiting this log message. I suspect if it
> >> happens it might happen frequently.
> >>
> >
> >
> > I'll adjust accordingly. Thanks,
> >
> >
> >>
> >>
> >> I think the forwarding field in the database bfd_status column is far
> >> more important than that in the appctl output which is mostly for
> >> developers to debug things. I'd rewrite this and the commit message
> >> to mention that instead. Also both here and in the comment message, I
> >> would talk about "interfaces" instead of "tunnels". BFD can be run
> >> on any kind of interface, so there's no reason to restrict it in the
> >> documentation.
> >>
> >
> > Thanks a lot, I agree (especially after I know how it will affect the
> bundle
> > ports). I'll redo them accordingly.
> >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] Introduce odp_flow_key_to_mask() API
On Tue, Aug 20, 2013 at 10:40:50AM -0700, gy...@nicira.com wrote:
> From: Guolin Yang
>
> With megaflow support, there is API to convert mask to nlattr key based
> format. This change introduces API to do the reverse conversion. We
> leverage the existing odp_flow_key_to_flow() API to resue the code.
>
> Signed-off-by: Guolin Yang
There were some warnings from Clang:
../lib/odp-util.c:2913:33: error: comparison of constant 255 with
expression of type 'enum ovs_key_attr' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
if (is_mask && expected_bit != 0xff) {
^
and from sparse:
../lib/odp-util.c:2696:17: warning: restricted __be16 degrades to integer
../lib/odp-util.c:2737:44: warning: restricted __be16 degrades to integer
../lib/odp-util.c:2822:58: warning: restricted __be16 degrades to integer
../lib/odp-util.c:2903:33: warning: restricted __be16 degrades to integer
../lib/odp-util.c:3095:24: warning: incorrect type in assignment (different
base types)
../lib/odp-util.c:3095:24:expected restricted __be16 [usertype] vlan_tci
../lib/odp-util.c:3095:24:got int
I fixed these, and some style rules and simplifications, by folding in
the following incremental. I applied the full patch at the end of
this email.
diff --git a/AUTHORS b/AUTHORS
index fc665b3..e94150f 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -36,6 +36,7 @@ FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp
Gaetano Catalli gaetano.cata...@gmail.com
Giuseppe Lettieri g.letti...@iet.unipi.it
Glen Gibb g...@stanford.edu
+Guolin Yang gy...@nicira.com
Gurucharan Shetty gshe...@nicira.com
Henry Mai h...@nicira.com
Hao Zheng hzh...@nicira.com
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 4373bbd..b351c71 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2680,7 +2680,7 @@ check_expectations(uint64_t present_attrs, int
out_of_range_attr,
static bool
parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
uint64_t present_attrs, uint64_t *expected_attrs,
-struct flow *flow, struct flow *src_flow)
+struct flow *flow, const struct flow *src_flow)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
bool is_mask = flow != src_flow;
@@ -2692,8 +2692,8 @@ parse_ethertype(const struct nlattr
*attrs[OVS_KEY_ATTR_MAX + 1],
ntohs(flow->dl_type));
return false;
}
-if (is_mask && ntohs(src_flow->dl_type) < ETH_TYPE_MIN &&
-flow->dl_type != 0x) {
+if (is_mask && ntohs(src_flow->dl_type) < ETH_TYPE_MIN &&
+flow->dl_type != htons(0x)) {
return false;
}
*expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
@@ -2701,11 +2701,8 @@ parse_ethertype(const struct nlattr
*attrs[OVS_KEY_ATTR_MAX + 1],
if (!is_mask) {
flow->dl_type = htons(FLOW_DL_TYPE_NONE);
} else if (ntohs(src_flow->dl_type) < ETH_TYPE_MIN) {
-/*
- * see comments odp_flow_key_from_flow__
- */
-VLOG_ERR_RL(&rl, "Alway expect mask for non ethernet II "
-"frame");
+/* See comments in odp_flow_key_from_flow__(). */
+VLOG_ERR_RL(&rl, "mask expected for non-Ethernet II frame");
return false;
}
}
@@ -2717,12 +2714,12 @@ parse_l2_5_onward(const struct nlattr
*attrs[OVS_KEY_ATTR_MAX + 1],
uint64_t present_attrs, int out_of_range_attr,
uint64_t expected_attrs, struct flow *flow,
const struct nlattr *key, size_t key_len,
- struct flow *src_flow)
+ const struct flow *src_flow)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
bool is_mask = src_flow != flow;
-const uint8_t *checkStart = NULL;
-size_t checkLen = 0;
+const void *check_start = NULL;
+size_t check_len = 0;
enum ovs_key_attr expected_bit = 0xff;
if (eth_type_mpls(src_flow->dl_type)) {
@@ -2737,14 +2734,12 @@ parse_l2_5_onward(const struct nlattr
*attrs[OVS_KEY_ATTR_MAX + 1],
} else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS)) {
flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
-if (flow->mpls_lse != 0 && flow->dl_type != 0x) {
+if (flow->mpls_lse != 0 && flow->dl_type != htons(0x)) {
return ODP_FIT_ERROR;
}
expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
if (flow->mpls_lse) {
-/*
- * do we need to set the following? XXX
- */
+/* XXX Is this needed? */
flow->mpls_depth = 0x;
}
}
@@ -2764,8 +2759,8 @@ p
Re: [ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
On Tue, Aug 20, 2013 at 03:02:47PM -0700, Romain Lenglet wrote: > Implement a per-exporter flow cache with active timeout expiration. > Add columns "cache_active_timeout" and "cache_max_flows" into table > "IPFIX" to configure each cache. > > Add per-flow elements "octetDeltaSumOfSquares", > "minimumIpTotalLength", and "maximumIpTotalLength" to replace > "ethernetTotalLength". Add per-flow element "flowEndReason" to > indicate whether a flow has expired because of an active timeout, the > cache size limit being reached, or the exporter being stopped. > > Signed-off-by: Romain Lenglet I'm mostly happy with this but I still want something else instead of timercmp, whether that's a timeval_compare_3way() function in timeval.[ch] or something else. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
Thanks for the insight.
On Tue, Aug 20, 2013 at 3:39 PM, Ben Pfaff wrote:
> To expand on that, usually one defines a non-static variable, in a .c
> file, only when an "extern" declaration of the same variable is
> already visible from #including an .h file. A definition without a
> previous declaration usually means that the .h file was forgotten.
>
> On Tue, Aug 20, 2013 at 02:48:57PM -0700, Alex Wang wrote:
> > I found the answer from this thread:
> > http://openvswitch.org/pipermail/dev/2011-May/008608.html
> >
> >
> >
> > On Tue, Aug 20, 2013 at 2:02 PM, Alex Wang wrote:
> >
> > > May I ask, what rule is this?
> > >
> > > Should we always declare global variable before defining it? or just
> when
> > > it is enclosed by macros?
> > >
> > > Also, when the "!USE_LINKER_SECTIONS", we should modify the coverage.c
> the
> > > same way. I'll send out a patch,
> > >
> > >
> > > On Tue, Aug 20, 2013 at 1:44 PM, Ben Pfaff wrote:
> > >
> > >> On Mon, Aug 19, 2013 at 12:32:06PM -0700, Alex Wang wrote:
> > >> > Just noticed, when compiling with sparse, it issues the warnings
> like:
> > >> >
> > >> > """
> > >> > lib/netdev-linux.c:76:1: warning: symbol
> 'counter_netdev_set_policing'
> > >> was
> > >> > not declared. Should it be static?
> > >> > lib/netdev-linux.c:77:1: warning: symbol
> 'counter_netdev_arp_lookup' was
> > >> > not declared. Should it be static?
> > >> > lib/netdev-linux.c:78:1: warning: symbol
> 'counter_netdev_get_ifindex'
> > >> was
> > >> > not declared. Should it be static?
> > >> > """
> > >>
> > >> Ouch, thanks, I folded in this incremental to fix that:
> > >>
> > >> diff --git a/lib/coverage.h b/lib/coverage.h
> > >> index 73b027a..3d1a115 100644
> > >> --- a/lib/coverage.h
> > >> +++ b/lib/coverage.h
> > >> @@ -54,6 +54,7 @@ struct coverage_counter {
> > >> {
> \
> > >> *counter_##COUNTER##_get() += n;
>\
> > >> }
> \
> > >> +extern struct coverage_counter counter_##COUNTER;
> \
> > >> struct coverage_counter counter_##COUNTER
> \
> > >> = { #COUNTER, COUNTER##_count, 0 };
> \
> > >> extern struct coverage_counter *counter_ptr_##COUNTER;
>\
> > >>
> > >
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
- Original Message - > From: "Ben Pfaff" > To: "Romain Lenglet" > Cc: dev@openvswitch.org > Sent: Tuesday, August 20, 2013 5:08:02 PM > Subject: Re: [PATCH] ipfix: implement flow caching and aggregation in exporter > > On Tue, Aug 20, 2013 at 03:02:47PM -0700, Romain Lenglet wrote: > > Implement a per-exporter flow cache with active timeout expiration. > > Add columns "cache_active_timeout" and "cache_max_flows" into table > > "IPFIX" to configure each cache. > > > > Add per-flow elements "octetDeltaSumOfSquares", > > "minimumIpTotalLength", and "maximumIpTotalLength" to replace > > "ethernetTotalLength". Add per-flow element "flowEndReason" to > > indicate whether a flow has expired because of an active timeout, the > > cache size limit being reached, or the exporter being stopped. > > > > Signed-off-by: Romain Lenglet > > I'm mostly happy with this but I still want something else instead of > timercmp, whether that's a timeval_compare_3way() function in > timeval.[ch] or something else. Thank you for your review and your suggestions. I have replaced timevals with uint64_t microsecond timestamps, and that simplified the code a lot and allowed me to find a bug in the calculation of negative deltas in ipfix_put_data_set(). So now I use timevals only temporarily to get the current time and I don't compare timevals anymore. I will send an updated patch shortly after I finish testing. Thanks! -- Romain Lenglet ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/8] ofproto-dpif: Make vlan splinters thread safe.
Just an oversight. Feel free to fix it.
Ethan
On Mon, Aug 19, 2013 at 6:29 PM, Simon Horman wrote:
> On Sat, Aug 03, 2013 at 06:42:06PM -0700, Ethan Jackson wrote:
>> Signed-off-by: Ethan Jackson
>> ---
>> ofproto/ofproto-dpif.c | 69
>> +++-
>> 1 file changed, 51 insertions(+), 18 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 17bc12f..e2dcd3f 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -486,8 +486,9 @@ struct ofproto_dpif {
>> long long int stp_last_tick;
>>
>> /* VLAN splinters. */
>> -struct hmap realdev_vid_map; /* (realdev,vid) -> vlandev. */
>> -struct hmap vlandev_map; /* vlandev -> (realdev,vid). */
>> +struct ovs_mutex vsp_mutex;
>> +struct hmap realdev_vid_map OVS_GUARDED; /* (realdev,vid) -> vlandev. */
>> +struct hmap vlandev_map OVS_GUARDED; /* vlandev -> (realdev,vid). */
>>
>> /* Ports. */
>> struct sset ports; /* Set of standard port names. */
>> @@ -1273,6 +1274,7 @@ construct(struct ofproto *ofproto_)
>> ofproto->ml = mac_learning_create(MAC_ENTRY_DEFAULT_IDLE_TIME);
>> ofproto->mbridge = mbridge_create();
>> ofproto->has_bonded_bundles = false;
>> +ovs_mutex_init(&ofproto->vsp_mutex, PTHREAD_MUTEX_NORMAL);
>>
>> classifier_init(&ofproto->facets);
>> ofproto->consistency_rl = LLONG_MIN;
>> @@ -1459,6 +1461,8 @@ destruct(struct ofproto *ofproto_)
>> sset_destroy(&ofproto->ghost_ports);
>> sset_destroy(&ofproto->port_poll_set);
>>
>> +ovs_mutex_destroy(&ofproto->vsp_mutex);
>> +
>> close_dpif_backer(ofproto->backer);
>> }
>>
>> @@ -6384,20 +6388,20 @@ hash_realdev_vid(ofp_port_t realdev_ofp_port, int
>> vid)
>>
>> bool
>> ofproto_has_vlan_splinters(const struct ofproto_dpif *ofproto)
>> +OVS_EXCLUDED(ofproto->vsp_mutex)
>> {
>> -return !hmap_is_empty(&ofproto->realdev_vid_map);
>> +bool ret;
>> +
>> +ovs_mutex_lock(&ofproto->vsp_mutex);
>> +ret = !hmap_is_empty(&ofproto->realdev_vid_map);
>> +ovs_mutex_unlock(&ofproto->vsp_mutex);
>> +return ret;
>> }
>>
>> -/* Returns the OFP port number of the Linux VLAN device that corresponds to
>> - * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
>> - * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
>> - * 'vlan_tci' 9, it would return the port number of eth0.9.
>> - *
>> - * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
>> - * function just returns its 'realdev_ofp_port' argument. */
>> -ofp_port_t
>> -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
>> - ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
>> +static ofp_port_t
>> +vsp_realdev_to_vlandev__(const struct ofproto_dpif *ofproto,
>> + ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
>> +OVS_REQUIRES(ofproto->vsp_mutex)
>> {
>> if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
>> int vid = vlan_tci_to_vid(vlan_tci);
>> @@ -6415,6 +6419,26 @@ vsp_realdev_to_vlandev(const struct ofproto_dpif
>> *ofproto,
>> return realdev_ofp_port;
>> }
>>
>> +/* Returns the OFP port number of the Linux VLAN device that corresponds to
>> + * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
>> + * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
>> + * 'vlan_tci' 9, it would return the port number of eth0.9.
>> + *
>> + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
>> + * function just returns its 'realdev_ofp_port' argument. */
>> +ofp_port_t
>> +vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
>> + ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
>> +OVS_EXCLUDED(ofproto->vsp_mutex)
>> +{
>> +ofp_port_t ret;
>> +
>> +ovs_mutex_lock(&ofproto->vsp_mutex);
>> +ret = vsp_realdev_to_vlandev__(ofproto, realdev_ofp_port, vlan_tci);
>> +ovs_mutex_unlock(&ofproto->vsp_mutex);
>> +return ret;
>> +}
>> +
>> static struct vlan_splinter *
>> vlandev_find(const struct ofproto_dpif *ofproto, ofp_port_t
>> vlandev_ofp_port)
>> {
>> @@ -6443,6 +6467,7 @@ vlandev_find(const struct ofproto_dpif *ofproto,
>> ofp_port_t vlandev_ofp_port)
>> static ofp_port_t
>> vsp_vlandev_to_realdev(const struct ofproto_dpif *ofproto,
>> ofp_port_t vlandev_ofp_port, int *vid)
>> +OVS_REQ_WRLOCK(ofproto->vsp_mutex)
>> {
>> if (!hmap_is_empty(&ofproto->vlandev_map)) {
>> const struct vlan_splinter *vsp;
>> @@ -6466,11 +6491,14 @@ vsp_vlandev_to_realdev(const struct ofproto_dpif
>> *ofproto,
>> * making any changes. */
>> static bool
>> vsp_adjust_flow(const struct ofproto_dpif *ofproto, struct flow *flow)
>> +OVS_EXCLUDED(ofproto->vsp_mutex)
>> {
>> ofp_port_t realdev;
>> int vid;
>>
>> +ovs_mutex_lock(&ofproto->vsp_mutex);
>> reald
[ovs-dev] [PATCH V2 2/2] bfd: Implements forwarding_if_rx
This commit adds a new boolean option "forwarding_if_rx" to bfd.
When forwarding_if_rx is true the interface will be considered
capabale of packet I/O as long as there is packet received at
interface. This is important in that when link becomes temporarily
conjested, consecutive BFD control packets can be lost. And the
forwarding_if_rx can prevent link failover by detecting non-control
packets received at interface.
Signed-off-by: Alex Wang
---
v1 -> v2:
- update the forwarding_if_rx_detect_time when netdev is changed.
- use rate-limited log function.
- refine the commit log and vswitchd.xml.
- add unit test for situation when both forwarding_if_rx and bfd
decay are enabled.
---
lib/bfd.c| 72 +--
tests/bfd.at | 251 +-
vswitchd/vswitch.xml |8 ++
3 files changed, 321 insertions(+), 10 deletions(-)
diff --git a/lib/bfd.c b/lib/bfd.c
index 47e67df..c782481 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -191,12 +191,19 @@ struct bfd {
atomic_bool check_tnl_key;/* Verify tunnel key of inbound packets? */
atomic_int ref_cnt;
+/* When forward_if_rx is true, bfd_forwarding() will return
+ * true as long as there are incoming packets received.
+ * Note, forwarding_override still has higher priority. */
+bool forwarding_if_rx;
+long long int forwarding_if_rx_detect_time;
+
/* BFD decay related variables. */
bool in_decay;/* True when bfd is in decay. */
int decay_min_rx; /* min_rx is set to decay_min_rx when */
/* in decay. */
int decay_rx_ctl; /* Count bfd packets received within decay */
/* detect interval. */
+uint64_t decay_rx_packets;/* Packets received by 'netdev'. */
long long int decay_detect_time; /* Decay detection time. */
};
@@ -223,6 +230,8 @@ static void bfd_put_details(struct ds *, const struct bfd *)
static uint64_t bfd_rx_packets(const struct bfd *) OVS_REQUIRES(mutex);
static void bfd_try_decay(struct bfd *) OVS_REQUIRES(mutex);
static void bfd_decay_update(struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_check_rx(struct bfd *) OVS_REQUIRES(mutex);
+static void bfd_forwarding_if_rx_update(struct bfd *) OVS_REQUIRES(mutex);
static void bfd_unixctl_show(struct unixctl_conn *, int argc,
const char *argv[], void *aux OVS_UNUSED);
static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
@@ -280,7 +289,7 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg,
long long int min_tx, min_rx;
bool need_poll = false;
bool cfg_min_rx_changed = false;
-bool cpath_down;
+bool cpath_down, forwarding_if_rx;
const char *hwaddr;
uint8_t ea[ETH_ADDR_LEN];
@@ -311,6 +320,7 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg,
bfd->mult = 3;
atomic_init(&bfd->ref_cnt, 1);
bfd->netdev = netdev_ref(netdev);
+bfd->rx_packets = bfd_rx_packets(bfd);
bfd->in_decay = false;
/* RFC 5881 section 4
@@ -384,6 +394,16 @@ bfd_configure(struct bfd *bfd, const char *name, const
struct smap *cfg,
bfd->eth_dst_set = false;
}
+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_UP && bfd->forwarding_if_rx) {
+bfd_forwarding_if_rx_update(bfd);
+} else {
+bfd->forwarding_if_rx_detect_time = 0;
+}
+}
+
if (need_poll) {
bfd_poll(bfd);
}
@@ -458,6 +478,9 @@ bfd_run(struct bfd *bfd) OVS_EXCLUDED(mutex)
bfd_try_decay(bfd);
}
+/* Always checks the reception of any packet. */
+bfd_check_rx(bfd);
+
if (bfd->min_tx != bfd->cfg_min_tx
|| (bfd->min_rx != bfd->cfg_min_rx && bfd->min_rx != bfd->decay_min_rx)
|| bfd->in_decay != old_in_decay) {
@@ -752,9 +775,13 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
*netdev)
if (bfd->netdev != netdev) {
netdev_close(bfd->netdev);
bfd->netdev = netdev_ref(netdev);
-if (bfd->decay_min_rx) {
+if (bfd->decay_min_rx && bfd->state == STATE_UP) {
bfd_decay_update(bfd);
}
+if (bfd->forwarding_if_rx && bfd->state == STATE_UP) {
+bfd_forwarding_if_rx_update(bfd);
+}
+bfd->rx_packets = bfd_rx_packets(bfd);
}
ovs_mutex_unlock(&mutex);
}
@@ -767,10 +794,12 @@ bfd_forwarding__(const struct bfd *bfd)
OVS_REQUIRES(mutex)
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
[ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
Implement a per-exporter flow cache with active timeout expiration.
Add columns "cache_active_timeout" and "cache_max_flows" into table
"IPFIX" to configure each cache.
Add per-flow elements "octetDeltaSumOfSquares",
"minimumIpTotalLength", and "maximumIpTotalLength" to replace
"ethernetTotalLength". Add per-flow element "flowEndReason" to
indicate whether a flow has expired because of an active timeout, the
cache size limit being reached, or the exporter being stopped.
Signed-off-by: Romain Lenglet
---
ofproto/ofproto-dpif-ipfix.c | 704 --
ofproto/ofproto-dpif-ipfix.h |3 +
ofproto/ofproto-dpif.c | 23 +-
ofproto/ofproto.h|4 +
utilities/ovs-vsctl.8.in |5 +-
vswitchd/bridge.c| 10 +
vswitchd/vswitch.ovsschema | 12 +-
vswitchd/vswitch.xml | 12 +
8 files changed, 675 insertions(+), 98 deletions(-)
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 8e8e7a2..a9cc73e 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -16,14 +16,17 @@
#include
#include "ofproto-dpif-ipfix.h"
+#include
#include "byte-order.h"
#include "collectors.h"
#include "flow.h"
#include "hash.h"
#include "hmap.h"
+#include "list.h"
#include "ofpbuf.h"
#include "ofproto.h"
#include "packets.h"
+#include "poll-loop.h"
#include "sset.h"
#include "util.h"
#include "timeval.h"
@@ -42,6 +45,10 @@ struct dpif_ipfix_exporter {
struct collectors *collectors;
uint32_t seq_number;
time_t last_template_set_time;
+struct hmap cache_flow_key_map; /* ipfix_flow_cache_entry. */
+struct list cache_flow_start_timestamp_list; /* ipfix_flow_cache_entry. */
+uint32_t cache_active_timeout; /* In seconds. */
+uint32_t cache_max_flows;
};
struct dpif_ipfix_bridge_exporter {
@@ -62,7 +69,7 @@ struct dpif_ipfix_flow_exporter_map_node {
struct dpif_ipfix {
struct dpif_ipfix_bridge_exporter bridge_exporter;
-struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_nodes. */
+struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_node. */
atomic_int ref_cnt;
};
@@ -143,32 +150,30 @@ struct ipfix_template_field_specifier {
});
BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 4);
-/* Part of data record for common metadata and Ethernet entities. */
+/* Part of data record flow key for common metadata and Ethernet entities. */
OVS_PACKED(
-struct ipfix_data_record_common {
+struct ipfix_data_record_flow_key_common {
ovs_be32 observation_point_id; /* OBSERVATION_POINT_ID */
-ovs_be64 packet_delta_count; /* PACKET_DELTA_COUNT */
-ovs_be64 layer2_octet_delta_count; /* LAYER2_OCTET_DELTA_COUNT */
uint8_t source_mac_address[6]; /* SOURCE_MAC_ADDRESS */
uint8_t destination_mac_address[6]; /* DESTINATION_MAC_ADDRESS */
ovs_be16 ethernet_type; /* ETHERNET_TYPE */
-ovs_be16 ethernet_total_length; /* ETHERNET_TOTAL_LENGTH */
uint8_t ethernet_header_length; /* ETHERNET_HEADER_LENGTH */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_common) == 37);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 19);
-/* Part of data record for VLAN entities. */
+/* Part of data record flow key for VLAN entities. */
OVS_PACKED(
-struct ipfix_data_record_vlan {
+struct ipfix_data_record_flow_key_vlan {
ovs_be16 vlan_id; /* VLAN_ID */
ovs_be16 dot1q_vlan_id; /* DOT1Q_VLAN_ID */
uint8_t dot1q_priority; /* DOT1Q_PRIORITY */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_vlan) == 5);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_vlan) == 5);
-/* Part of data record for IP entities. */
+/* Part of data record flow key for IP entities. */
+/* XXX: Replace IP_TTL with MINIMUM_TTL and MAXIMUM_TTL? */
OVS_PACKED(
-struct ipfix_data_record_ip {
+struct ipfix_data_record_flow_key_ip {
uint8_t ip_version; /* IP_VERSION */
uint8_t ip_ttl; /* IP_TTL */
uint8_t protocol_identifier; /* PROTOCOL_IDENTIFIER */
@@ -176,32 +181,116 @@ struct ipfix_data_record_ip {
uint8_t ip_precedence; /* IP_PRECEDENCE */
uint8_t ip_class_of_service; /* IP_CLASS_OF_SERVICE */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_ip) == 6);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_ip) == 6);
-/* Part of data record for IPv4 entities. */
+/* Part of data record flow key for IPv4 entities. */
OVS_PACKED(
-struct ipfix_data_record_ipv4 {
+struct ipfix_data_record_flow_key_ipv4 {
ovs_be32 source_ipv4_address; /* SOURCE_IPV4_ADDRESS */
ovs_be32 destination_ipv4_address; /* DESTINATION_IPV4_ADDRESS */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_ipv4) == 8);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_ipv4) == 8);
-/* Part of data record for IPv4 entities. */
+/* Part of data record flow key for IPv6 entities. */
OVS_PACKED(
-struct ipfix_dat
[ovs-dev] [PATCH 1/2] ofproto-dpif.at: Remove push_vlan from an OF1.0 test.
Remove push_vlan from an OF1.0 test, as it requires OF1.1+ support, but was silently discarded. Signed-off-by: Jarno Rajahalme --- tests/ofproto-dpif.at |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 32e282d..7ef95db 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -266,7 +266,7 @@ cookie=0x6 table=4 in_port=83 actions=load:4->NXM_NX_REG3[[]],mod_nw_src:83.83.8 cookie=0x7 table=5 in_port=84 actions=load:5->NXM_NX_REG4[[]],load:6->NXM_NX_TUN_ID[[]],mod_nw_dst:84.84.84.84,controller,resubmit(85,6) cookie=0x8 table=6 in_port=85 actions=mod_tp_src:85,controller,resubmit(86,7) cookie=0x9 table=7 in_port=86 actions=mod_tp_dst:86,controller,controller -cookie=0xa dl_src=40:44:44:44:44:41 actions=push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,controller +cookie=0xa dl_src=40:44:44:44:44:41 actions=mod_vlan_vid:99,mod_vlan_pcp:1,controller cookie=0xa dl_src=40:44:44:44:44:42 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller cookie=0xa dl_src=40:44:44:44:44:43 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller cookie=0xa dl_src=40:44:44:44:44:44 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/8] ofproto-dpif: Make vlan splinters thread safe.
Thanks, I feel better about my understanding of the code now.
I'll compose a patch in the not to distant future.
On Tue, Aug 20, 2013 at 05:44:49PM -0700, Ethan Jackson wrote:
> Just an oversight. Feel free to fix it.
>
> Ethan
>
> On Mon, Aug 19, 2013 at 6:29 PM, Simon Horman wrote:
> > On Sat, Aug 03, 2013 at 06:42:06PM -0700, Ethan Jackson wrote:
> >> Signed-off-by: Ethan Jackson
> >> ---
> >> ofproto/ofproto-dpif.c | 69
> >> +++-
> >> 1 file changed, 51 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> index 17bc12f..e2dcd3f 100644
> >> --- a/ofproto/ofproto-dpif.c
> >> +++ b/ofproto/ofproto-dpif.c
> >> @@ -486,8 +486,9 @@ struct ofproto_dpif {
> >> long long int stp_last_tick;
> >>
> >> /* VLAN splinters. */
> >> -struct hmap realdev_vid_map; /* (realdev,vid) -> vlandev. */
> >> -struct hmap vlandev_map; /* vlandev -> (realdev,vid). */
> >> +struct ovs_mutex vsp_mutex;
> >> +struct hmap realdev_vid_map OVS_GUARDED; /* (realdev,vid) -> vlandev.
> >> */
> >> +struct hmap vlandev_map OVS_GUARDED; /* vlandev -> (realdev,vid).
> >> */
> >>
> >> /* Ports. */
> >> struct sset ports; /* Set of standard port names. */
> >> @@ -1273,6 +1274,7 @@ construct(struct ofproto *ofproto_)
> >> ofproto->ml = mac_learning_create(MAC_ENTRY_DEFAULT_IDLE_TIME);
> >> ofproto->mbridge = mbridge_create();
> >> ofproto->has_bonded_bundles = false;
> >> +ovs_mutex_init(&ofproto->vsp_mutex, PTHREAD_MUTEX_NORMAL);
> >>
> >> classifier_init(&ofproto->facets);
> >> ofproto->consistency_rl = LLONG_MIN;
> >> @@ -1459,6 +1461,8 @@ destruct(struct ofproto *ofproto_)
> >> sset_destroy(&ofproto->ghost_ports);
> >> sset_destroy(&ofproto->port_poll_set);
> >>
> >> +ovs_mutex_destroy(&ofproto->vsp_mutex);
> >> +
> >> close_dpif_backer(ofproto->backer);
> >> }
> >>
> >> @@ -6384,20 +6388,20 @@ hash_realdev_vid(ofp_port_t realdev_ofp_port, int
> >> vid)
> >>
> >> bool
> >> ofproto_has_vlan_splinters(const struct ofproto_dpif *ofproto)
> >> +OVS_EXCLUDED(ofproto->vsp_mutex)
> >> {
> >> -return !hmap_is_empty(&ofproto->realdev_vid_map);
> >> +bool ret;
> >> +
> >> +ovs_mutex_lock(&ofproto->vsp_mutex);
> >> +ret = !hmap_is_empty(&ofproto->realdev_vid_map);
> >> +ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> +return ret;
> >> }
> >>
> >> -/* Returns the OFP port number of the Linux VLAN device that corresponds
> >> to
> >> - * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
> >> - * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0
> >> and
> >> - * 'vlan_tci' 9, it would return the port number of eth0.9.
> >> - *
> >> - * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> >> - * function just returns its 'realdev_ofp_port' argument. */
> >> -ofp_port_t
> >> -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> >> - ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
> >> +static ofp_port_t
> >> +vsp_realdev_to_vlandev__(const struct ofproto_dpif *ofproto,
> >> + ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
> >> +OVS_REQUIRES(ofproto->vsp_mutex)
> >> {
> >> if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
> >> int vid = vlan_tci_to_vid(vlan_tci);
> >> @@ -6415,6 +6419,26 @@ vsp_realdev_to_vlandev(const struct ofproto_dpif
> >> *ofproto,
> >> return realdev_ofp_port;
> >> }
> >>
> >> +/* Returns the OFP port number of the Linux VLAN device that corresponds
> >> to
> >> + * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
> >> + * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0
> >> and
> >> + * 'vlan_tci' 9, it would return the port number of eth0.9.
> >> + *
> >> + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> >> + * function just returns its 'realdev_ofp_port' argument. */
> >> +ofp_port_t
> >> +vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> >> + ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
> >> +OVS_EXCLUDED(ofproto->vsp_mutex)
> >> +{
> >> +ofp_port_t ret;
> >> +
> >> +ovs_mutex_lock(&ofproto->vsp_mutex);
> >> +ret = vsp_realdev_to_vlandev__(ofproto, realdev_ofp_port, vlan_tci);
> >> +ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> +return ret;
> >> +}
> >> +
> >> static struct vlan_splinter *
> >> vlandev_find(const struct ofproto_dpif *ofproto, ofp_port_t
> >> vlandev_ofp_port)
> >> {
> >> @@ -6443,6 +6467,7 @@ vlandev_find(const struct ofproto_dpif *ofproto,
> >> ofp_port_t vlandev_ofp_port)
> >> static ofp_port_t
> >> vsp_vlandev_to_realdev(const struct ofproto_dpif *ofproto,
> >> ofp_port_t vlandev_ofp_port, int *vid)
> >> +OVS_REQ_WRLOCK(ofproto->vsp_mutex)
> >> {
> >> if (!hmap_i
