[ovs-dev] Clang compile failure with C11 atomics
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?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] OpenvSwitch DKMS compatibility with Linux 3.11
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Hi Folks Ubuntu Saucy is going to ship the 3.11 kernel; I'd only just got caught up with backporting the 3.10 patch from the master branch to ovs 1.10 and everything broke again... Anyway - digging through the kernel source I noticed that it looks like the native openvswitch module in the kernel is growing tunnel support in 3.11 - which was one of the key reasons we still ship the DKMS packages in Ubuntu. Is this the case? Is the delta between DKMS and native kernel disappearing? Can I just ship the userspace tooling for 13.10 of Ubuntu? If not, some help diagnosing the issue I'm seeing would be great: https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1213879 Cheers James - -- James Page Ubuntu and Debian Developer james.p...@ubuntu.com jamesp...@debian.org -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with undefined - http://www.enigmail.net/ iQIcBAEBCAAGBQJSEgYIAAoJEL/srsug59jDWe0QALA/LqnTtlfL3MDuxqXU065C ntzmgMewPzH7W56KpSo5ylWlP4Rsl0t60hqoCcxmMJsQUgbXcwg7uXzdqQcn5s3R a57Aq+PuIufIMbtM+8OKBYZaWJGVJxRr08vl3f8SIPos53MpYk776KmUNMvsDF/m gYf/93qeNc0zBxOVISejRKTNyGfrEzc2LQnhW558bqWb12A9kd5VSyaMdUQaD11J zjVcg4SsEi8bwQFiPpcKDMILJFQjQZSlYloL5HScNA5qSR81SLjNj4wAiI6BeRAK Ufxw1kC+TP++31lVFIBWtCwQmhDmD0+/Q6af7q9GnGkoTRwvvTA8Hrd0wlAjvW3n IkPNm1JqsLf67RbU8LdYUq1smfaZ1VeNDhHBlkRZQngqU+niTUN1L/sFLfeQQhJv B3yfEL4XlHcCsMwNfHggcYl15ikYaHs5gE48/YOuIMiAfSgi/icSl0OET6wXKu8F A7+4rky8n2hdfwceKqXYl4vuRX6MRTmJflpWVZNqr+qSaRKfzx1/fTtNMq1OnBWx KOde+5h6WYNxodydhuLqHypeEsEmgWTkHI+94CoyYZgQkSan8f76oNG+POjsMqsL 5fpBi8OWSLvyF8yvbzmt+NJ1x8CZiwyF/7rs0iKEbGkvfBSsSnIfmij6o5S6XGdz S5d44loVrRh705Bkffx7 =LMEY -END PGP SIGNATURE- ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
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++) {
On Fri, Aug 16, 2013 at 3:47 PM, Ben Pfaff wrote:
> This makes each of the coverage counters per-thread. It abandons the
> idea of trying to keep track of the number of hits in the "current" poll
> loop, since there are many poll loops running, each in its own thread, as
> well as the idea of numbering epochs for the same reason. Instead, we
> just keep track of overall totals for the process for each coverage
> counter, accumulating per-thread counts into the global total each time
> a thread's main loop passes through poll_block().
>
> Signed-off-by: Ben Pfaff
> ---
> lib/coverage.c | 73
> +++-
> lib/coverage.h | 41 ++-
> 2 files changed, 71 insertions(+), 43 deletions(-)
>
> diff --git a/lib/coverage.c b/lib/coverage.c
> index f152474..ac5dd67 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2009, 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.
> @@ -35,7 +35,19 @@ extern struct coverage_counter *__stop_coverage[];
> #define coverage_counters __start_coverage
> #define n_coverage_counters (__stop_coverage - __start_coverage)
> #else /* !USE_LINKER_SECTIONS */
> -#define COVERAGE_COUNTER(NAME) COVERAGE_DEFINE__(NAME);
> +#define COVERAGE_COUNTER(COUNTER) \
> +DECLARE_EXTERN_PER_THREAD_DATA(unsigned int,\
> + counter_##COUNTER); \
> +DEFINE_EXTERN_PER_THREAD_DATA(counter_##COUNTER, 0);\
> +static unsigned int COUNTER##_count(void) \
> +{ \
> +unsigned int *countp = counter_##COUNTER##_get(); \
> +unsigned int count = *countp; \
> +*countp = 0;\
> +return count; \
> +} \
> +struct coverage_counter counter_##COUNTER \
> += { #COUNTER, COUNTER##_count, 0 };
> #include "coverage.def"
> #undef COVERAGE_COUNTER
>
> @@ -47,7 +59,7 @@ struct coverage_counter *coverage_counters[] = {
> #define n_coverage_counters ARRAY_SIZE(coverage_counters)
> #endif /* !USE_LINKER_SECTIONS */
>
> -static unsigned int epoch;
> +static struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER;
>
> static void coverage_read(struct svec *);
>
> @@ -82,8 +94,8 @@ compare_coverage_counters(const void *a_, const void *b_)
> const struct coverage_counter *const *bp = b_;
> const struct coverage_counter *a = *ap;
> const struct coverage_counter *b = *bp;
> -if (a->count != b->count) {
> -return a->count < b->count ? 1 : -1;
> +if (a->total != b->total) {
> +return a->total < b->total ? 1 : -1;
> } else {
> return strcmp(a->name, b->name);
> }
> @@ -98,9 +110,11 @@ coverage_hash(void)
>
> /* Sort coverage counters into groups with equal counts. */
> c = xmalloc(n_coverage_counters * sizeof *c);
> +ovs_mutex_lock(&coverage_mutex);
> for (i = 0; i < n_coverage_counters; i++) {
> c[i] = coverage_counters[i];
> }
> +ovs_mutex_unlock(&coverage_mutex);
> qsort(c, n_coverage_counters, sizeof *c, compare_coverage_counters);
>
> /* Hash the names in each group along with the rank. */
> @@ -108,13 +122,13 @@ coverage_hash(void)
> for (i = 0; i < n_coverage_counters; ) {
> int j;
>
> -if (!c[i]->count) {
> +if (!c[i]->total) {
> break;
> }
> n_groups++;
> hash = hash_int(i, hash);
> for (j = i; j < n_coverage_counters; j++) {
> -if (c[j]->count != c[i]->count) {
> +if (c[j]->total != c[i]->total) {
> break;
> }
>
Re: [ovs-dev] OpenvSwitch DKMS compatibility with Linux 3.11
On Mon, Aug 19, 2013 at 4:48 AM, James Page wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA256 > > Hi Folks > > Ubuntu Saucy is going to ship the 3.11 kernel; I'd only just got > caught up with backporting the 3.10 patch from the master branch to > ovs 1.10 and everything broke again... > > Anyway - digging through the kernel source I noticed that it looks > like the native openvswitch module in the kernel is growing tunnel > support in 3.11 - which was one of the key reasons we still ship the > DKMS packages in Ubuntu. > > Is this the case? Is the delta between DKMS and native kernel > disappearing? Can I just ship the userspace tooling for 13.10 of Ubuntu? It's going away but it's not quite there yet. The 3.11 upstream kernel supports GRE but not VXLAN (that's hopefully coming in 3.12) so I think it probably makes sense to go with the out of tree version for one more release and then switch over. > If not, some help diagnosing the issue I'm seeing would be great: > > https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1213879 Here's an upstream commit that is definitely needed as part of 3.11 support and looks at least somewhat related, so it might be a good place to start: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/openvswitch?id=351638e7deeed2ec8ce451b53d33921b3da68f83 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif-sflow: Fix memory leak.
dpif_sflow_set_options() uses xcalloc() to allocate space for the SFLAgent
structure, but nothing ever freed it. This fixes the problem.
Found by valgrind.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif-sflow.c |1 +
1 file changed, 1 insertion(+)
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index b387b94..44ad927 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -280,6 +280,7 @@ dpif_sflow_clear__(struct dpif_sflow *ds)
OVS_REQUIRES(mutex)
{
if (ds->sflow_agent) {
sfl_agent_release(ds->sflow_agent);
+free(ds->sflow_agent);
ds->sflow_agent = NULL;
}
collectors_destroy(ds->collectors);
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] Fix typo.
Signed-off-by: pritesh
---
datapath/flow.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/datapath/flow.c b/datapath/flow.c
index 4075350..3b3e0aa 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1202,7 +1202,7 @@ int ovs_ipv4_tun_from_nlattr(const struct nlattr *attr,
if (ovs_tunnel_key_lens[type] != nla_len(a)) {
OVS_NLERR("IPv4 tunnel attribute type has unexpected "
- " legnth (type=%d, length=%d,
expected=%d).\n",
+ " length (type=%d, length=%d,
expected=%d).\n",
type, nla_len(a), ovs_tunnel_key_lens[type]);
return -EINVAL;
}
@@ -1389,7 +1389,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
*match, u64 attrs,
/* Always exact match EtherType. */
eth_type = htons(0x);
} else if (ntohs(eth_type) < ETH_P_802_3_MIN) {
- OVS_NLERR("EtherType is less than mimimum (type=%x,
min=%x).\n",
+ OVS_NLERR("EtherType is less than minimum (type=%x,
min=%x).\n",
ntohs(eth_type), ETH_P_802_3_MIN);
return -EINVAL;
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] coverage: Make thread-safe.
Hey Ben,
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?
"""
On Mon, Aug 19, 2013 at 8:08 AM, 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++) {
>
>
> On Fri, Aug 16, 2013 at 3:47 PM, Ben Pfaff wrote:
>
>> This makes each of the coverage counters per-thread. It abandons the
>> idea of trying to keep track of the number of hits in the "current" poll
>> loop, since there are many poll loops running, each in its own thread, as
>> well as the idea of numbering epochs for the same reason. Instead, we
>> just keep track of overall totals for the process for each coverage
>> counter, accumulating per-thread counts into the global total each time
>> a thread's main loop passes through poll_block().
>>
>> Signed-off-by: Ben Pfaff
>> ---
>> lib/coverage.c | 73
>> +++-
>> lib/coverage.h | 41 ++-
>> 2 files changed, 71 insertions(+), 43 deletions(-)
>>
>> diff --git a/lib/coverage.c b/lib/coverage.c
>> index f152474..ac5dd67 100644
>> --- a/lib/coverage.c
>> +++ b/lib/coverage.c
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
>> + * Copyright (c) 2009, 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.
>> @@ -35,7 +35,19 @@ extern struct coverage_counter *__stop_coverage[];
>> #define coverage_counters __start_coverage
>> #define n_coverage_counters (__stop_coverage - __start_coverage)
>> #else /* !USE_LINKER_SECTIONS */
>> -#define COVERAGE_COUNTER(NAME) COVERAGE_DEFINE__(NAME);
>> +#define COVERAGE_COUNTER(COUNTER) \
>> +DECLARE_EXTERN_PER_THREAD_DATA(unsigned int,\
>> + counter_##COUNTER); \
>> +DEFINE_EXTERN_PER_THREAD_DATA(counter_##COUNTER, 0);\
>> +static unsigned int COUNTER##_count(void) \
>> +{ \
>> +unsigned int *countp = counter_##COUNTER##_get(); \
>> +unsigned int count = *countp; \
>> +*countp = 0;\
>> +return count; \
>> +} \
>> +struct coverage_counter counter_##COUNTER \
>> += { #COUNTER, COUNTER##_count, 0 };
>> #include "coverage.def"
>> #undef COVERAGE_COUNTER
>>
>> @@ -47,7 +59,7 @@ struct coverage_counter *coverage_counters[] = {
>> #define n_coverage_counters ARRAY_SIZE(coverage_counters)
>> #endif /* !USE_LINKER_SECTIONS */
>>
>> -static unsigned int epoch;
>> +static struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER;
>>
>> static void coverage_read(struct svec *);
>>
>> @@ -82,8 +94,8 @@ compare_coverage_counters(const void *a_, const void
>> *b_)
>> const struct coverage_counter *const *bp = b_;
>> const struct coverage_counter *a = *ap;
>> const struct coverage_counter *b = *bp;
>> -if (a->count != b->count) {
>> -return a->count < b->count ? 1 : -1;
>> +if (a->total != b->total) {
>> +return a->total < b->total ? 1 : -1;
>> } else {
>> return strcmp(a->name, b->name);
>> }
>> @@ -98,9 +110,11 @@ coverage_hash(void)
>>
>> /* Sort coverage counters into groups with equal counts. */
>> c = xmalloc(n_coverage_counters * sizeof *c);
>> +ovs_mutex_lock(&coverage_mutex);
>> for (i = 0; i < n_coverage_counters; i++) {
>> c[i] = coverage_counters[i];
>> }
>> +ovs_mutex_unlock(&coverage_mutex);
>> qsort(c, n_cover
Re: [ovs-dev] [PATCH] ofproto-dpif-sflow: Fix memory leak.
Acked-by: Ethan Jackson
On Mon, Aug 19, 2013 at 10:26 AM, Ben Pfaff wrote:
> dpif_sflow_set_options() uses xcalloc() to allocate space for the SFLAgent
> structure, but nothing ever freed it. This fixes the problem.
>
> Found by valgrind.
>
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto-dpif-sflow.c |1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index b387b94..44ad927 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -280,6 +280,7 @@ dpif_sflow_clear__(struct dpif_sflow *ds)
> OVS_REQUIRES(mutex)
> {
> if (ds->sflow_agent) {
> sfl_agent_release(ds->sflow_agent);
> +free(ds->sflow_agent);
> ds->sflow_agent = NULL;
> }
> collectors_destroy(ds->collectors);
> --
> 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] [PATCH v2] Introduce odp_flow_key_to_mask() API
It looks correct to me. Ben, would you please consider committing it?
Acked-by: Andy Zhou
git am reported a white space error: patch:28: trailing whitespace.
if (is_mask && ntohs(src_flow->dl_type) < 1536 &&
warning: 1 line adds whitespace errors.
I'd like to further improving the patch in the following area:
1. Modify test-odp.c to make use of this new function.
2. It seems exception handler can be further simplified, but I don't have
any specific suggestion at the moment.
3. Consider Simon's feed back in using ETH_TYPE_MIN in place of the hard
coded 1536.
Thanks,
Andy
On Thu, Aug 15, 2013 at 11:28 PM, wrote:
> From: gyang
>
> 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..e9bb548 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) < 1536) {
> VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key",
> ntohs(flow->dl_type));
> return false;
> }
> +if (is_mask && ntohs(src_flow->dl_type) < 1536 &&
> +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) < 1536) {
> +/*
> + * 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)) {
> -expecte
[ovs-dev] [PATCH 1/3] datapath: Remove old argument description in flow.c.
Signed-off-by: Justin Pettit --- datapath/flow.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/datapath/flow.c b/datapath/flow.c index 4075350..9a1e01c 100644 --- a/datapath/flow.c +++ b/datapath/flow.c @@ -799,7 +799,6 @@ invalid: * Ethernet header * @in_port: port number on which @skb was received. * @key: output flow key - * @key_lenp: length of output flow key * * The caller must ensure that skb->len >= ETH_HLEN. * -- 1.7.5.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] datapath: Fix argument description in vport-lisp.c.
Signed-off-by: Justin Pettit --- datapath/vport-lisp.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c index 2f62d11..28d3e20 100644 --- a/datapath/vport-lisp.c +++ b/datapath/vport-lisp.c @@ -211,7 +211,7 @@ static void lisp_build_header(const struct vport *vport, * * @vport: port this packet was received on * @skb: received packet - * @tos: ToS from encapsulating IP packet, used to copy ECN bits + * @tun_key: tunnel that carried packet * * Must be called with rcu_read_lock. * -- 1.7.5.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto: Start ofport allocation from the previous max after restart.
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
---
ofproto/ofproto.c |4
1 file changed, 4 insertions(+)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bbdb2d2..d7ed5f6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2089,6 +2089,10 @@ init_ports(struct ofproto *p)
netdev = ofport_open(p, &ofproto_port, &pp);
if (netdev) {
ofport_install(p, netdev, &pp);
+if (ofproto_port.ofp_port < p->max_ports) {
+p->alloc_port_no = MAX(p->alloc_port_no,
+ ofproto_port.ofp_port);
+}
}
}
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 3/3] datapath: Fix argument descriptions in vport.c.
Signed-off-by: Justin Pettit --- datapath/vport.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/datapath/vport.c b/datapath/vport.c index 03b775d..f26beaf 100644 --- a/datapath/vport.c +++ b/datapath/vport.c @@ -206,7 +206,7 @@ out: * ovs_vport_set_options - modify existing vport device (for kernel callers) * * @vport: vport to modify. - * @port: New configuration. + * @options: New configuration. * * Modifies an existing device with the specified configuration (which is * dependent on device type). ovs_mutex must be held. @@ -352,6 +352,7 @@ int ovs_vport_get_options(const struct vport *vport, struct sk_buff *skb) * * @vport: vport that received the packet * @skb: skb that was received + * @tun_key: tunnel (if any) that carried packet * * Must be called with rcu_read_lock. The packet cannot be shared and * skb->data should point to the Ethernet header. The caller must have already -- 1.7.5.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] datapath: Remove old argument description in flow.c.
On Mon, Aug 19, 2013 at 5:28 PM, Justin Pettit wrote: > Signed-off-by: Justin Pettit Thanks, all of these look good to me. Acked-by: Jesse Gross ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] datapath: Fix argument description in vport-lisp.c.
On Mon, Aug 19, 2013 at 5:28 PM, Justin Pettit wrote: > Signed-off-by: Justin Pettit > --- > datapath/vport-lisp.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c > index 2f62d11..28d3e20 100644 > --- a/datapath/vport-lisp.c > +++ b/datapath/vport-lisp.c > @@ -211,7 +211,7 @@ static void lisp_build_header(const struct vport *vport, > * > * @vport: port this packet was received on > * @skb: received packet > - * @tos: ToS from encapsulating IP packet, used to copy ECN bits > + * @tun_key: tunnel that carried packet > * > * Must be called with rcu_read_lock. > * If you want, there's another outdated part of this comment - the line about skb->csum can just be removed. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Fix typo.
On Mon, Aug 19, 2013 at 12:00 PM, pritesh wrote: > Signed-off-by: pritesh Applied, thanks (I added some detail to the commit message). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] datapath: Remove old argument description in flow.c.
On Aug 19, 2013, at 5:39 PM, Jesse Gross wrote: > On Mon, Aug 19, 2013 at 5:28 PM, Justin Pettit wrote: >> Signed-off-by: Justin Pettit > > Thanks, all of these look good to me. > > Acked-by: Jesse Gross Thanks. I pushed them all and removed the line about skb->csum, as you mentioned in the review of patch 2. --Justin ___ 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.
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);
> realdev = vsp_vlandev_to_realdev(ofproto, flow->in_port.ofp_port, &vid);
> +ovs_mutex_unlock(&ofproto->vsp_mutex);
> if (!realdev) {
> return false;
> }
> @@ -6488,6 +6516,7 @@ vsp_remove(struct o
[ovs-dev] [PATCH v2.37 2/6] odp: Allow VLAN actions after MPLS actions
From: Joe Stringer
OpenFlow 1.2 and 1.3 differ on their handling of MPLS actions in the
presence of VLAN tags. To allow correct behaviour to be committed in
each situation, this patch adds a second round of VLAN tag action
handling to commit_odp_actions(), which occurs after MPLS actions. This
is implemented with a new field in 'struct xlate_in' called 'vlan_tci'.
When an push_mpls action is composed, the flow's current VLAN state is
stored into xin->vlan_tci, and flow->vlan_tci is set to 0 (pop_vlan). If
a VLAN tag is present, it is stripped; if not, then there is no change.
Any later modifications to the VLAN state is written to xin->vlan_tci.
When committing the actions, flow->vlan_tci is used before MPLS actions,
and xin->vlan_tci is used afterwards. This retains the current datapath
behaviour, but allows VLAN actions to be applied in a more flexible
manner.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v2.37
* Rebase
v2.36
* No change
v2.5
* First post
---
lib/odp-util.c | 10 +-
lib/odp-util.h |4 +-
ofproto/ofproto-dpif-xlate.c | 95 ++-
ofproto/ofproto-dpif-xlate.h |5 +
tests/ofproto-dpif.at| 209 ++
5 files changed, 297 insertions(+), 26 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index d0c9bbe..ad0cd0f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3324,10 +3324,15 @@ commit_set_pkt_mark_action(const struct flow *flow,
struct flow *base,
* key from 'base' into 'flow', and then changes 'base' the same way. Does not
* commit set_tunnel actions. Users should call commit_odp_tunnel_action()
* in addition to this function if needed. Sets fields in 'wc' that are
- * used as part of the action. */
+ * used as part of the action.
+ *
+ * VLAN actions may be committed twice; If vlan_tci in 'flow' differs from the
+ * one in 'base', then it is committed before MPLS actions. If 'final_vlan_tci'
+ * differs from 'flow->vlan_tci', it is committed afterwards. */
void
commit_odp_actions(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+ ovs_be16 final_vlan_tci)
{
commit_set_ether_addr_action(flow, base, odp_actions, wc);
commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
@@ -3338,6 +3343,7 @@ commit_odp_actions(const struct flow *flow, struct flow
*base,
* that it is no longer IP and thus nw and port actions are no longer
valid.
*/
commit_mpls_action(flow, base, odp_actions, wc);
+commit_vlan_action(final_vlan_tci, base, odp_actions, wc);
commit_set_priority_action(flow, base, odp_actions, wc);
commit_set_pkt_mark_action(flow, base, odp_actions, wc);
}
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 0c40f38..74a2ce8 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -127,8 +127,8 @@ const char *odp_key_fitness_to_string(enum odp_key_fitness);
void commit_odp_tunnel_action(const struct flow *, struct flow *base,
struct ofpbuf *odp_actions);
void commit_odp_actions(const struct flow *, struct flow *base,
-struct ofpbuf *odp_actions,
-struct flow_wildcards *wc);
+struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+ovs_be16 final_vlan_tci);
/* ofproto-dpif interface.
*
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e80ec84..5a4b415 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -974,10 +974,11 @@ static void
output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
uint16_t vlan)
{
-ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
+ovs_be16 *flow_tci = &ctx->xin->vlan_tci;
uint16_t vid;
ovs_be16 tci, old_tci;
struct xport *xport;
+bool flow_tci_equal_to_xin = (*flow_tci == ctx->xin->flow.vlan_tci);
vid = output_vlan_to_vid(out_xbundle, vlan);
if (list_is_empty(&out_xbundle->xports)) {
@@ -1008,9 +1009,15 @@ output_normal(struct xlate_ctx *ctx, const struct
xbundle *out_xbundle,
}
}
*flow_tci = tci;
+if (flow_tci_equal_to_xin) {
+ctx->xin->flow.vlan_tci = tci;
+}
compose_output_action(ctx, xport->ofp_port);
*flow_tci = old_tci;
+if (flow_tci_equal_to_xin) {
+ctx->xin->flow.vlan_tci = old_tci;
+}
}
/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
@@ -1242,7 +1249,7 @@ xlate_normal(struct xlate_ctx *ctx)
/* Drop malformed frames. */
if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
-!(flow->vlan_tci & htons(VLAN_CFI))) {
+!(ctx->xin->vlan_tci & htons(VLAN_CFI))) {
if (ctx->xin->packet != NULL) {
static struct vlog_rate_limit rl = VLOG_RATE
[ovs-dev] [PATCH v2.37 0/6] MPLS actions and matches
Hi, This series implements MPLS actions and matches based on work by Ravi K, Leo Alterman, Yamahata-san and Joe Stringer. This series provides two changes * Provide user-space support for the VLAN/MPLS tag insertion order up to and including OpenFlow 1.2, and the different ordering specified from OpenFlow 1.3. In a nutshell the datapath always uses the OpenFlow 1.3 ordering, which is to always insert tags immediately after the L2 header, regardless of the presence of other tags. And ovs-vswtichd provides compatibility for the behaviour up to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags if present. * Adding basic MPLS action and match support to the kernel datapath Differences between v2.37 and v2.36: * Rebase Differences between v2.36 and v2.35: * Rebase * Do not add set_ethertype() to datapath/actions.c. As this patch has evolved this function had devolved into to sets of functionality wrapped into a single function with only one line of common code. Refactor things to simply open-code setting the ether type in the two locations where set_ethertype() was previously used. The aim here is to improve readability. * Update setting skb->ethertype after mpls push and pop. - In the case of push_mpls it should be set unconditionally as in v2.35 the behaviour of this function to always push an MPLS LSE before any VLAN tags. - In the case of mpls_pop eth_p_mpls(skb->protocol) is a better test than skb->protocol != htons(ETH_P_8021Q) as it will give the correct behaviour in the presence of other VLAN ethernet types, for example 0x88a8 which is used by 802.1ad. Moreover, it seems correct to update the ethernet type if it was previously set according to the top-most MPLS LSE. * Deaccelerate VLANs when pushing MPLS tags the - Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags. This means that if an accelerated tag is present it should be deaccelerated to ensure it ends up in the correct position. * Update skb->mac_len in push_mpls() so that it will be correct when used by a subsequent call to pop_mpls(). As things stand I do not believe this is strictly necessary as ovs-vswitchd will not send a pop MPLS action after a push MPLS action. However, I have added this in order to code more defensively as I believe that if such a sequence did occur it would be rather unobvious why it didn't work. * Do not add skb_cow_head() call in push_mpls(). It is unnecessary as there is a make_writable() call. This change was also made in v2.30 but some how the code regressed between then and v2.35. Differences between v2.35 and v2.34: * Add support for the tag ordering specified up until OpenFlow 1.2 and the ordering specified from OpenFlow 1.3. * Correct error in datapath patch's handling of GSO in the presence of MPLS and absence of VLANs. Patch overview: * The first 5 patches of this series are new, adding support for different tag ordering. * The last patch is a revised version of the patch to add MPLS support to the datapath. It has been updated to use OpenFlow 1.3 tag ordering and resolve a GSO handling bug: both mentioned above. Its change log includes a history of changes. To aid review this series is available in git at: git://github.com/horms/openvswitch.git devel/mpls-v2.37 Patch list and overall diffstat: Joe Stringer (5): odp: Only pass vlan_tci to commit_vlan_action() odp: Allow VLAN actions after MPLS actions ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS ofp-actions: Add separate OpenFlow 1.3 action parser lib: Push MPLS tags in the OpenFlow 1.3 ordering Simon Horman (1): datapath: Add basic MPLS support to kernel datapath/Modules.mk |1 + datapath/actions.c | 122 ++- datapath/datapath.c | 254 +++-- datapath/datapath.h |9 + datapath/flow.c | 58 ++- datapath/flow.h | 17 +- datapath/linux/compat/gso.c | 50 ++- datapath/linux/compat/gso.h | 39 ++ datapath/linux/compat/include/linux/netdevice.h | 12 - datapath/linux/compat/netdevice.c | 28 -- datapath/mpls.h | 15 + datapath/vport-lisp.c |1 + datapath/vport-netdev.c | 44 ++- include/linux/openvswitch.h |7 +- lib/flow.c |2 +- lib/odp-util.c | 22 +- lib/odp-util.h |4 +- lib/ofp-actions.c | 68 +++- lib/ofp-parse.c |1 + lib/ofp-util.c |3 + lib/ofp-util.h |1 + lib/packets.c
[ovs-dev] [PATCH v2.37 1/6] odp: Only pass vlan_tci to commit_vlan_action()
From: Joe Stringer
This allows for future patches to pass different tci values to
commit_vlan_action() without passing an entire flow structure.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v2.36 - v2.37
* No change
v2.35
* First post
---
lib/odp-util.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index a09042e..d0c9bbe 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3097,10 +3097,10 @@ commit_set_ether_addr_action(const struct flow *flow,
struct flow *base,
}
static void
-commit_vlan_action(const struct flow *flow, struct flow *base,
+commit_vlan_action(ovs_be16 vlan_tci, struct flow *base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
-if (base->vlan_tci == flow->vlan_tci) {
+if (base->vlan_tci == vlan_tci) {
return;
}
@@ -3110,15 +3110,15 @@ commit_vlan_action(const struct flow *flow, struct flow
*base,
nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
}
-if (flow->vlan_tci & htons(VLAN_CFI)) {
+if (vlan_tci & htons(VLAN_CFI)) {
struct ovs_action_push_vlan vlan;
vlan.vlan_tpid = htons(ETH_TYPE_VLAN);
-vlan.vlan_tci = flow->vlan_tci;
+vlan.vlan_tci = vlan_tci;
nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
&vlan, sizeof vlan);
}
-base->vlan_tci = flow->vlan_tci;
+base->vlan_tci = vlan_tci;
}
static void
@@ -3330,7 +3330,7 @@ commit_odp_actions(const struct flow *flow, struct flow
*base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
commit_set_ether_addr_action(flow, base, odp_actions, wc);
-commit_vlan_action(flow, base, odp_actions, wc);
+commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
commit_set_nw_action(flow, base, odp_actions, wc);
commit_set_port_action(flow, base, odp_actions, wc);
/* Committing MPLS actions should occur after committing nw and port
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2.37 4/6] ofp-actions: Add separate OpenFlow 1.3 action parser
From: Joe Stringer
This patch adds new ofpact_from_openflow13() and
ofpacts_from_openflow13() functions parallel to the existing ofpact
handling code. In the OpenFlow 1.3 version, push_mpls is handled
differently, but all other actions are handled by the existing code.
For push_mpls, ofpact_push_mpls.ofpact.compat is set to
OFPUTIL_OFPAT13_PUSH_MPLS, which allows correct VLAN+MPLS datapath
behaviour to be determined at odp translation time.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v2.36 - v2.37
* No change
v2.35
* First post
---
lib/ofp-actions.c | 63 ++---
1 file changed, 60 insertions(+), 3 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 921aa27..454b9a1 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -878,6 +878,40 @@ ofpacts_from_openflow11(const union ofp_action *in, size_t
n_in,
return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
}
+static enum ofperr
+ofpact_from_openflow13(const union ofp_action *a, struct ofpbuf *out)
+{
+enum ofputil_action_code code;
+enum ofperr error;
+
+error = decode_openflow11_action(a, &code);
+if (error) {
+return error;
+}
+
+if (code == OFPUTIL_OFPAT11_PUSH_MPLS) {
+struct ofpact_push_mpls *oam;
+struct ofp11_action_push *oap = (struct ofp11_action_push *)a;
+if (!eth_type_mpls(oap->ethertype)) {
+return OFPERR_OFPBAC_BAD_ARGUMENT;
+}
+oam = ofpact_put_PUSH_MPLS(out);
+oam->ethertype = oap->ethertype;
+oam->ofpact.compat = OFPUTIL_OFPAT13_PUSH_MPLS;
+} else {
+return ofpact_from_openflow11(a, out);
+}
+
+return error;
+}
+
+static enum ofperr
+ofpacts_from_openflow13(const union ofp_action *in, size_t n_in,
+struct ofpbuf *out)
+{
+return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow13);
+}
+
/* OpenFlow 1.1 instructions. */
#define DEFINE_INST(ENUM, STRUCT, EXTENSIBLE, NAME) \
@@ -1081,6 +1115,17 @@ get_actions_from_instruction(const struct
ofp11_instruction *inst,
*n_actions = (ntohs(inst->len) - sizeof *inst) / OFP11_INSTRUCTION_ALIGN;
}
+static uint8_t
+get_version_from_ofpbuf(const struct ofpbuf *openflow)
+{
+if (openflow && openflow->l2) {
+struct ofp_header *oh = openflow->l2;
+return oh->version;
+}
+
+return OFP10_VERSION;
+}
+
/* Attempts to convert 'actions_len' bytes of OpenFlow 1.1 actions from the
* front of 'openflow' into ofpacts. On success, replaces any existing content
* in 'ofpacts' by the converted ofpacts; on failure, clears 'ofpacts'.
@@ -1100,8 +1145,15 @@ ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
unsigned int actions_len,
struct ofpbuf *ofpacts)
{
-return ofpacts_pull_actions(openflow, actions_len, ofpacts,
-ofpacts_from_openflow11);
+uint8_t version = get_version_from_ofpbuf(openflow);
+
+if (version < OFP13_VERSION) {
+return ofpacts_pull_actions(openflow, actions_len, ofpacts,
+ofpacts_from_openflow11);
+} else {
+return ofpacts_pull_actions(openflow, actions_len, ofpacts,
+ofpacts_from_openflow13);
+}
}
enum ofperr
@@ -1153,10 +1205,15 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf
*openflow,
if (insts[OVSINST_OFPIT11_APPLY_ACTIONS]) {
const union ofp_action *actions;
size_t n_actions;
+uint8_t version = get_version_from_ofpbuf(openflow);
get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
&actions, &n_actions);
-error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
+if (version < OFP13_VERSION) {
+error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
+} else {
+error = ofpacts_from_openflow13(actions, n_actions, ofpacts);
+}
if (error) {
goto exit;
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2.37 6/6] datapath: Add basic MPLS support to kernel
Allow datapath to recognize and extract MPLS labels into flow keys and execute actions which push, pop, and set labels on packets. Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer. Cc: Ravi K Cc: Leo Alterman Cc: Isaku Yamahata Cc: Joe Stringer Signed-off-by: Simon Horman --- v2.36 - v2.37 * Rebase * Do not add set_ethertype() to datapath/actions.c. As this patch has evolved this function had devolved into to sets of functionality wrapped into a single function with only one line of common code. Refactor things to simply open-code setting the ether type in the two locations where set_ethertype() was previously used. The aim here is to improve readability. * Update setting skb->ethertype after mpls push and pop. - In the case of push_mpls it should be set unconditionally as in v2.35 the behaviour of this function to always push an MPLS LSE before any VLAN tags. - In the case of mpls_pop eth_p_mpls(skb->protocol) is a better test than skb->protocol != htons(ETH_P_8021Q) as it will give the correct behaviour in the presence of other VLAN ethernet types, for example 0x88a8 which is used by 802.1ad. Moreover, it seems correct to update the ethernet type if it was previously set according to the top-most MPLS LSE. * Deaccelerate VLANs when pushing MPLS tags the - Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags. This means that if an accelerated tag is present it should be deaccelerated to ensure it ends up in the correct position. * Update skb->mac_len in push_mpls() so that it will be correct when used by a subsequent call to pop_mpls(). As things stand I do not believe this is strictly necessary as ovs-vswitchd will not send a pop MPLS action after a push MPLS action. However, I have added this in order to code more defensively as I believe that if such a sequence did occur it would be rather unobvious why it didn't work. * Do not add skb_cow_head() call in push_mpls(). It is unnecessary as there is a make_writable() call. This change was also made in v2.30 but some how the code regressed between then and v2.35. v2.35 * Rebase * Move MPLS constants to mpls.h * Push MPLS tags after ethernet, before VLAN tags - This is consistent with the OpenFlow 1.3 specification - Compatibility with OpenFlow 1.2 and earlier versions may be provided by ovs-vswitchd. * Correct GSO behaviour in the presence of MPLS but absence of VLANs v2.34 * Rebase for megaflow changes v2.33 * Ensure that inner_protocol is always set to to the current skb->protocol value in ovs_execute_actions(). This ensures it is set to the correct value in the absence of a push_mpls action. Also remove setting of inner_protocol in push_mpls() as it duplicates the code now in ovs_execute_actions(). * Call __skb_gso_segment() instead of skb_gso_segment() from rpl___skb_gso_segment() in the case that HAVE___SKB_GSO_SEGMENT is set. This was a typo. v2.32 * As suggested by Jesse Gross - Use int instead of size_t in validate_and_copy_actions__(). - Fix crazy edit mess in pop_mpls() action comment - Move eth_p_mpls() into mpls.h - Refactor skb_gso_segment MPLS handling into rpl_skb_gso_segment Address Jesse's comments regarding this code: "Can we push this completely into the skb_gso_segment() compatibility code? It's both nicer and may make the interactions with the vlan code less confusing." - Move GSO compatibility code into linux/compat/gso.* - Set skb->protocol on mpls_push and mpls_pop in the presence of an offloaded VLAN. v2.31 * As suggested by Jesse Gross - There is no need to make mac_header_end inline as it is not in a header file - Remove dubious if (*skb_ethertype == ethertype) optimisation from set_ethertype - Only set skb->protocol in push_mpls() or pop_mpls() for non-VLAN packets - Use MAX_ETH_TYPES instead of SAMPLE_ACTION_DEPTH for array size of types in struct eth_types. This corrects a typo/thinko. - Correct eth type tracking logic such that start isn't advanced when entering a sample action, ensuring that all possibly types are checked when verifying nested actions. * Define HAVE_INNER_PROTOCOL based on kernel version. inner_protocol has been merged into net-next and should appear in v3.11 so there is no longer a need for a acinclude.m4 test to check for it. * Add MPLS GSO compatibility code. This is for use on kernels that do not have MPLS GSO support. Thanks to Joe Stringer for his work on this. v2.30 * As suggested by Jesse Gross - Use skb_cow_head in push_mpls to ensure there is sufficient headroom for skb_push - Call make_writable with skb->mac_len instead of skb->mac_len + MPLS_HLEN in push_mpls as only the first skb->mac_len bytes of existing packet data are modified. - Rename skb_mac_header_end as mac_header_end, this seems to be a more appropriate name for a local function. - Remove OV
[ovs-dev] [PATCH v2.37 3/6] ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS
From: Joe Stringer
This patch adds a new compatibility enum for use with MPLS, so that the
differing behaviour between OpenFlow 1.2 and 1.3 can be implemented in
ofproto-dpif-xlate.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v2.36 - v2.37
* No change
v2.35
* First post
---
lib/ofp-actions.c |5 -
lib/ofp-parse.c |1 +
lib/ofp-util.c|3 +++
lib/ofp-util.h|1 +
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 61e2854..921aa27 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -322,6 +322,7 @@ ofpact_from_nxast(const union ofp_action *a, enum
ofputil_action_code code,
#define OFPAT10_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM:
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) case OFPUTIL_##ENUM:
#include "ofp-util.def"
+case OFPUTIL_OFPAT13_PUSH_MPLS:
NOT_REACHED();
case OFPUTIL_NXAST_RESUBMIT:
@@ -480,6 +481,7 @@ ofpact_from_openflow10(const union ofp_action *a, struct
ofpbuf *out)
case OFPUTIL_ACTION_INVALID:
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) case OFPUTIL_##ENUM:
#include "ofp-util.def"
+case OFPUTIL_OFPAT13_PUSH_MPLS:
NOT_REACHED();
case OFPUTIL_OFPAT10_OUTPUT:
@@ -842,7 +844,8 @@ ofpact_from_openflow11(const union ofp_action *a, struct
ofpbuf *out)
ofpact_put_DEC_MPLS_TTL(out);
break;
-case OFPUTIL_OFPAT11_PUSH_MPLS: {
+case OFPUTIL_OFPAT11_PUSH_MPLS:
+case OFPUTIL_OFPAT13_PUSH_MPLS: {
struct ofp11_action_push *oap = (struct ofp11_action_push *)a;
if (!eth_type_mpls(oap->ethertype)) {
return OFPERR_OFPBAC_BAD_ARGUMENT;
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 5cb39f5..5e719d3 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -799,6 +799,7 @@ parse_named_action(enum ofputil_action_code code,
break;
case OFPUTIL_OFPAT11_PUSH_MPLS:
+case OFPUTIL_OFPAT13_PUSH_MPLS:
case OFPUTIL_NXAST_PUSH_MPLS:
error = str_to_u16(arg, "push_mpls", ðertype);
if (!error) {
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 45ff0a1..47d2050 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4764,6 +4764,9 @@ ofputil_put_action(enum ofputil_action_code code, struct
ofpbuf *buf)
case OFPUTIL_ACTION_INVALID:
NOT_REACHED();
+case OFPUTIL_OFPAT13_PUSH_MPLS:
+return ofputil_put_OFPAT11_PUSH_MPLS(buf);
+
#define OFPAT10_ACTION(ENUM, STRUCT, NAME) \
case OFPUTIL_##ENUM: return ofputil_put_##ENUM(buf);
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) \
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index f3348c0..b5f3506 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -751,6 +751,7 @@ enum OVS_PACKED_ENUM ofputil_action_code {
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) OFPUTIL_##ENUM,
#define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) OFPUTIL_##ENUM,
#include "ofp-util.def"
+OFPUTIL_OFPAT13_PUSH_MPLS
};
/* The number of values of "enum ofputil_action_code". */
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2.37 5/6] lib: Push MPLS tags in the OpenFlow 1.3 ordering
From: Joe Stringer
This patch modifies the push_mpls behaviour to follow the OpenFlow 1.3
specification in the presence of VLAN tagged packets. From the spec:
"Newly pushed tags should always be inserted as the outermost tag in the
outermost valid location for that tag. When a new VLAN tag is pushed, it
should be the outermost tag inserted, immediately after the Ethernet
header and before other tags. Likewise, when a new MPLS tag is pushed,
it should be the outermost tag inserted, immediately after the Ethernet
header and before other tags."
When the push_mpls action was inserted using OpenFlow 1.2, we implement
the previous behaviour by inserting VLAN actions around the MPLS action
in the odp translation; Pop VLAN tags before committing MPLS actions,
and push the expected VLAN tag afterwards. The trigger condition for
this is based on the ofpact->compat field.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v2.36 - v2.37
* No change
v2.35
* First post
---
lib/flow.c |2 +-
lib/packets.c| 10 +-
lib/packets.h|2 +-
ofproto/ofproto-dpif-xlate.c | 10 +-
tests/ofproto-dpif.at| 237 ++
5 files changed, 253 insertions(+), 8 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index 3e29aa1..eae08fa 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1036,7 +1036,7 @@ flow_compose(struct ofpbuf *b, const struct flow *flow)
}
if (eth_type_mpls(flow->dl_type)) {
-b->l2_5 = b->l3;
+b->l2_5 = (char*)b->l2 + ETH_HEADER_LEN;
push_mpls(b, flow->dl_type, flow->mpls_lse);
}
}
diff --git a/lib/packets.c b/lib/packets.c
index b95e1e0..54a3b20 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -244,11 +244,11 @@ eth_mpls_depth(const struct ofpbuf *packet)
/* Set ethertype of the packet. */
void
-set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type)
+set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type, bool inner)
{
struct eth_header *eh = packet->data;
-if (eh->eth_type == htons(ETH_TYPE_VLAN)) {
+if (inner && eh->eth_type == htons(ETH_TYPE_VLAN)) {
ovs_be16 *p;
p = ALIGNED_CAST(ovs_be16 *,
(char *)(packet->l2_5 ? packet->l2_5 : packet->l3) - 2);
@@ -356,8 +356,8 @@ push_mpls(struct ofpbuf *packet, ovs_be16 ethtype, ovs_be32
lse)
if (!is_mpls(packet)) {
/* Set ethtype and MPLS label stack entry. */
-set_ethertype(packet, ethtype);
-packet->l2_5 = packet->l3;
+set_ethertype(packet, ethtype, false);
+packet->l2_5 = (char*)packet->l2 + ETH_HEADER_LEN;
}
/* Push new MPLS shim header onto packet. */
@@ -378,7 +378,7 @@ pop_mpls(struct ofpbuf *packet, ovs_be16 ethtype)
size_t len;
mh = packet->l2_5;
len = (char*)packet->l2_5 - (char*)packet->l2;
-set_ethertype(packet, ethtype);
+set_ethertype(packet, ethtype, true);
if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
packet->l2_5 = NULL;
} else {
diff --git a/lib/packets.h b/lib/packets.h
index 33be891..1b999e0 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -145,7 +145,7 @@ void eth_pop_vlan(struct ofpbuf *);
uint16_t eth_mpls_depth(const struct ofpbuf *packet);
-void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type);
+void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type, bool inner);
const char *eth_from_hex(const char *hex, struct ofpbuf **packetp);
void eth_format_masked(const uint8_t eth[ETH_ADDR_LEN],
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a4b415..46fd2a1 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2185,6 +2185,12 @@ may_receive(const struct xport *xport, struct xlate_ctx
*ctx)
return true;
}
+static bool
+mpls_compat_behaviour(enum ofputil_action_code compat)
+{
+return (compat != OFPUTIL_OFPAT13_PUSH_MPLS);
+}
+
static void
vlan_tci_restore(struct xlate_in *xin, ovs_be16 *tci_ptr, ovs_be16 orig_tci)
{
@@ -2366,7 +2372,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
/* Save and pop any existing VLAN tags if running in OF1.2 mode. */
ctx->xin->vlan_tci = *vlan_tci;
-flow->vlan_tci = htons(0);
+if (mpls_compat_behaviour(a->compat)) {
+flow->vlan_tci = htons(0);
+}
vlan_tci = &ctx->xin->vlan_tci;
break;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 37634a9..3d379d5 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1017,6 +1017,243 @@ NXST_FLOW reply:
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - OF1.3+ VLAN+MPLS handling])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+ON_EXIT([kill `cat ovs-ofctl.pid`])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [dnl
+cookie=0xa dl_src=40:44:44:44:
[ovs-dev] [branch-1.11] ofproto-dpif: Fix memory leak in handle_flow_miss().
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
Re: [ovs-dev] [branch-1.11] ofproto-dpif: Fix memory leak in handle_flow_miss().
Great catch. Also, very cool with sleuthing with your malloc histogram (as
discussed off-list).
Do you think it's worth mentioning in the comment describing xlate_actions()?
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
