Re: [ovs-dev] [PATCH] bitmap: Optimize bitmap_equal and bitmap_scan
On Tue, Jul 29, 2014 at 1:01 AM, Ben Pfaff wrote: > Needs a sign-off. > Oh.. I will not forget it in the next version. > > I'm pretty sure that bitmap_equal() is wrong: doesn't it access one > past the end of the bitmap arrays? You are right. I'll fix it. > I think that rightmost_1bit_idx() might make bitmap_scan() faster. > I'll give it a try. > The nice thing about the previous implementations was that they were > obviously correct. Now, I'd prefer to have some tests. > > No problem, I'll add some tests in the next version. Thanks for reviewing ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 10/10] lib/classifier: Support variable sized miniflows.
On Sat, Apr 19, 2014 at 3:42 AM, Jarno Rajahalme wrote:
> Change the classifier to allocate variable sized miniflows and
> minimasks in cls_match and cls_subtable, respectively. Do not
> duplicate the mask in cls_rule any more.
>
> miniflow_clone and miniflow_move can now take variably sized miniflows
> as source. The destination is assumed to be regularly sized miniflow.
>
> Inlining miniflow and mask values reduces memory indirection and helps
> reduce cache misses.
>
> Signed-off-by: Jarno Rajahalme
> ---
> lib/classifier.c | 82
> +++---
> lib/flow.c | 55 +++-
> lib/flow.h | 30 ++--
> 3 files changed, 134 insertions(+), 33 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 75befad..1198a76 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -40,7 +40,6 @@ struct cls_trie {
>
> struct cls_subtable_entry {
> struct cls_subtable *subtable;
> -const uint32_t *mask_values;
> tag_type tag;
> unsigned int max_priority;
> };
> @@ -72,7 +71,6 @@ struct cls_subtable {
> struct hmap_node hmap_node; /* Within struct cls_classifier 'subtables'
> * hmap. */
> struct hmap rules; /* Contains "struct cls_rule"s. */
> -struct minimask mask; /* Wildcards for fields. */
> int n_rules;/* Number of rules, including duplicates. */
> unsigned int max_priority; /* Max priority of any rule in the subtable.
> */
> unsigned int max_count; /* Count of max_priority rules. */
> @@ -81,6 +79,8 @@ struct cls_subtable {
> uint8_t index_ofs[CLS_MAX_INDICES]; /* u32 flow segment boundaries. */
> struct hindex indices[CLS_MAX_INDICES]; /* Staged lookup indices. */
> unsigned int trie_plen[CLS_MAX_TRIES]; /* Trie prefix length in 'mask'.
> */
> +struct minimask mask; /* Wildcards for fields. */
> +/* 'mask' must be the last field. */
> };
>
> /* Associates a metadata value (that is, a value of the OpenFlow 1.1+
> metadata
> @@ -103,16 +103,21 @@ struct cls_match {
> unsigned int priority; /* Larger numbers are higher priorities. */
> struct cls_partition *partition;
> struct list list; /* List of identical, lower-priority rules.
> */
> -struct minimatch match; /* Matching rule. */
> +struct miniflow flow; /* Matching rule. Mask is in the subtable. */
> +/* 'flow' must be the last field. */
> };
>
> static struct cls_match *
> cls_match_alloc(struct cls_rule *rule)
> {
> -struct cls_match *cls_match = xmalloc(sizeof *cls_match);
> +int count = count_1bits(rule->match.flow.map);
> +
> +struct cls_match *cls_match
> += xmalloc(sizeof *cls_match - sizeof cls_match->flow.inline_values
> + + MINIFLOW_VALUES_SIZE(count));
Would it lead to a potential array access violation problem when
'sizeof cls_match->flow.inline_values' is bigger than
'MINIFLOW_VALUES_SIZE(count)'?
>
> cls_match->cls_rule = rule;
> -minimatch_clone(&cls_match->match, &rule->match);
> +miniflow_clone_inline(&cls_match->flow, &rule->match.flow, count);
> cls_match->priority = rule->priority;
> rule->cls_match = cls_match;
>
> @@ -875,7 +880,6 @@ static inline void
> lookahead_subtable(const struct cls_subtable_entry *subtables)
> {
> ovs_prefetch_range(subtables->subtable, sizeof *subtables->subtable);
> -ovs_prefetch_range(subtables->mask_values, 1);
> }
>
> /* Finds and returns the highest-priority rule in 'cls' that matches 'flow'.
> @@ -969,16 +973,19 @@ classifier_lookup(const struct classifier *cls_, const
> struct flow *flow,
> }
>
> /* Returns true if 'target' satisifies 'match', that is, if each bit for
> which
> - * 'match' specifies a particular value has the correct value in 'target'. */
> + * 'match' specifies a particular value has the correct value in 'target'.
> + *
> + * 'flow' and 'mask' have the same mask! */
> static bool
> -minimatch_matches_miniflow(const struct minimatch *match,
> - const struct miniflow *target)
> +miniflow_and_mask_matches_miniflow(const struct miniflow *flow,
> + const struct minimask *mask,
> + const struct miniflow *target)
> {
> -const uint32_t *flowp = miniflow_get_u32_values(&match->flow);
> -const uint32_t *maskp = miniflow_get_u32_values(&match->mask.masks);
> +const uint32_t *flowp = miniflow_get_u32_values(flow);
> +const uint32_t *maskp = miniflow_get_u32_values(&mask->masks);
> uint32_t target_u32;
>
> -MINIFLOW_FOR_EACH_IN_MAP(target_u32, target, match->mask.masks.map) {
> +MINIFLOW_FOR_EACH_IN_MAP(target_u32, target, mask->masks.map) {
> if ((*flowp++ ^ target_u32) & *maskp++) {
> return false;
> }
> @@ -995,7 +1002,8 @@ find_match_
Re: [ovs-dev] [PATCH 09/10] lib/flow: Maintain miniflow offline values explicitly.
On Sat, Apr 19, 2014 at 3:42 AM, Jarno Rajahalme wrote:
> This allows use of miniflows that have all of their values inline.
>
> Signed-off-by: Jarno Rajahalme
> ---
> lib/classifier.c | 36 +++--
> lib/dpif-netdev.c | 32 ++-
> lib/flow.c| 91
> ++---
> lib/flow.h| 70 +
> lib/match.c |4 +--
> 5 files changed, 127 insertions(+), 106 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 0517aa7..75befad 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -40,7 +40,7 @@ struct cls_trie {
>
> struct cls_subtable_entry {
> struct cls_subtable *subtable;
> -uint32_t *mask_values;
> +const uint32_t *mask_values;
> tag_type tag;
> unsigned int max_priority;
> };
> @@ -279,8 +279,9 @@ static inline uint32_t
> flow_hash_in_minimask(const struct flow *flow, const struct minimask *mask,
>uint32_t basis)
> {
> +const uint32_t *mask_values = miniflow_get_u32_values(&mask->masks);
> const uint32_t *flow_u32 = (const uint32_t *)flow;
> -const uint32_t *p = mask->masks.values;
> +const uint32_t *p = mask_values;
> uint32_t hash;
> uint64_t map;
>
> @@ -289,7 +290,7 @@ flow_hash_in_minimask(const struct flow *flow, const
> struct minimask *mask,
> hash = mhash_add(hash, flow_u32[raw_ctz(map)] & *p++);
> }
>
> -return mhash_finish(hash, (p - mask->masks.values) * 4);
> +return mhash_finish(hash, (p - mask_values) * 4);
> }
>
> /* Returns a hash value for the bits of 'flow' where there are 1-bits in
> @@ -301,7 +302,8 @@ static inline uint32_t
> miniflow_hash_in_minimask(const struct miniflow *flow,
>const struct minimask *mask, uint32_t basis)
> {
> -const uint32_t *p = mask->masks.values;
> +const uint32_t *mask_values = miniflow_get_u32_values(&mask->masks);
> +const uint32_t *p = mask_values;
> uint32_t hash = basis;
> uint32_t flow_u32;
>
> @@ -309,7 +311,7 @@ miniflow_hash_in_minimask(const struct miniflow *flow,
> hash = mhash_add(hash, flow_u32 & *p++);
> }
>
> -return mhash_finish(hash, (p - mask->masks.values) * 4);
> +return mhash_finish(hash, (p - mask_values) * 4);
> }
>
> /* Returns a hash value for the bits of range [start, end) in 'flow',
> @@ -322,11 +324,12 @@ flow_hash_in_minimask_range(const struct flow *flow,
> const struct minimask *mask,
> uint8_t start, uint8_t end, uint32_t *basis)
> {
> +const uint32_t *mask_values = miniflow_get_u32_values(&mask->masks);
> const uint32_t *flow_u32 = (const uint32_t *)flow;
> unsigned int offset;
> uint64_t map = miniflow_get_map_in_range(&mask->masks, start, end,
> &offset);
> -const uint32_t *p = mask->masks.values + offset;
> +const uint32_t *p = mask_values + offset;
> uint32_t hash = *basis;
>
> for (; map; map = zero_rightmost_1bit(map)) {
> @@ -334,7 +337,7 @@ flow_hash_in_minimask_range(const struct flow *flow,
> }
>
> *basis = hash; /* Allow continuation from the unfinished value. */
> -return mhash_finish(hash, (p - mask->masks.values) * 4);
> +return mhash_finish(hash, (p - mask_values) * 4);
> }
>
> /* Fold minimask 'mask''s wildcard mask into 'wc's wildcard mask. */
> @@ -356,7 +359,7 @@ flow_wildcards_fold_minimask_range(struct flow_wildcards
> *wc,
> unsigned int offset;
> uint64_t map = miniflow_get_map_in_range(&mask->masks, start, end,
> &offset);
> -const uint32_t *p = mask->masks.values + offset;
> +const uint32_t *p = miniflow_get_u32_values(&mask->masks) + offset;
>
> for (; map; map = zero_rightmost_1bit(map)) {
> dst_u32[raw_ctz(map)] |= *p++;
> @@ -367,7 +370,8 @@ flow_wildcards_fold_minimask_range(struct flow_wildcards
> *wc,
> static inline uint32_t
> miniflow_hash(const struct miniflow *flow, uint32_t basis)
> {
> -const uint32_t *p = flow->values;
> +const uint32_t *values = miniflow_get_u32_values(flow);
> +const uint32_t *p = values;
> uint32_t hash = basis;
> uint64_t hash_map = 0;
> uint64_t map;
> @@ -382,7 +386,7 @@ miniflow_hash(const struct miniflow *flow, uint32_t basis)
> hash = mhash_add(hash, hash_map);
> hash = mhash_add(hash, hash_map >> 32);
>
> -return mhash_finish(hash, p - flow->values);
> +return mhash_finish(hash, p - values);
> }
>
> /* Returns a hash value for 'mask', given 'basis'. */
> @@ -415,8 +419,8 @@ minimatch_hash_range(const struct minimatch *match,
> uint8_t start, uint8_t end,
>
> n = count_1bits(miniflow_get_map_in_range(&match->mask.masks, start, end,
>&offset));
> -q = match->mask.masks.values +
Re: [ovs-dev] [PATCH 09/10] lib/flow: Maintain miniflow offline values explicitly.
On Tue, Apr 29, 2014 at 9:43 AM, Ethan Jackson wrote:
> Acked-by: Ethan Jackson
>
> I'm not sure I totally follow Kmindg's comment, perhaps he could
> explain further? At any rate, once addressed I"m happy with this.
>
After a second look into this, I found that I misunderstood these code before.
Sorry for the noise.
> Ethan
>
> On Sat, Apr 19, 2014 at 10:09 PM, Kmindg G wrote:
>> On Sat, Apr 19, 2014 at 3:42 AM, Jarno Rajahalme
>> wrote:
>>> This allows use of miniflows that have all of their values inline.
>>>
>>> Signed-off-by: Jarno Rajahalme
>>> ---
>>> lib/classifier.c | 36 +++--
>>> lib/dpif-netdev.c | 32 ++-
>>> lib/flow.c| 91
>>> ++---
>>> lib/flow.h| 70 +
>>> lib/match.c |4 +--
>>> 5 files changed, 127 insertions(+), 106 deletions(-)
>>>
>>> diff --git a/lib/classifier.c b/lib/classifier.c
>>> index 0517aa7..75befad 100644
>>> --- a/lib/classifier.c
>>> +++ b/lib/classifier.c
>>> @@ -40,7 +40,7 @@ struct cls_trie {
>>>
>>> struct cls_subtable_entry {
>>> struct cls_subtable *subtable;
>>> -uint32_t *mask_values;
>>> +const uint32_t *mask_values;
>>> tag_type tag;
>>> unsigned int max_priority;
>>> };
>>> @@ -279,8 +279,9 @@ static inline uint32_t
>>> flow_hash_in_minimask(const struct flow *flow, const struct minimask *mask,
>>>uint32_t basis)
>>> {
>>> +const uint32_t *mask_values = miniflow_get_u32_values(&mask->masks);
>>> const uint32_t *flow_u32 = (const uint32_t *)flow;
>>> -const uint32_t *p = mask->masks.values;
>>> +const uint32_t *p = mask_values;
>>> uint32_t hash;
>>> uint64_t map;
>>>
>>> @@ -289,7 +290,7 @@ flow_hash_in_minimask(const struct flow *flow, const
>>> struct minimask *mask,
>>> hash = mhash_add(hash, flow_u32[raw_ctz(map)] & *p++);
>>> }
>>>
>>> -return mhash_finish(hash, (p - mask->masks.values) * 4);
>>> +return mhash_finish(hash, (p - mask_values) * 4);
>>> }
>>>
>>> /* Returns a hash value for the bits of 'flow' where there are 1-bits in
>>> @@ -301,7 +302,8 @@ static inline uint32_t
>>> miniflow_hash_in_minimask(const struct miniflow *flow,
>>>const struct minimask *mask, uint32_t basis)
>>> {
>>> -const uint32_t *p = mask->masks.values;
>>> +const uint32_t *mask_values = miniflow_get_u32_values(&mask->masks);
>>> +const uint32_t *p = mask_values;
>>> uint32_t hash = basis;
>>> uint32_t flow_u32;
>>>
>>> @@ -309,7 +311,7 @@ miniflow_hash_in_minimask(const struct miniflow *flow,
>>> hash = mhash_add(hash, flow_u32 & *p++);
>>> }
>>>
>>> -return mhash_finish(hash, (p - mask->masks.values) * 4);
>>> +return mhash_finish(hash, (p - mask_values) * 4);
>>> }
>>>
>>> /* Returns a hash value for the bits of range [start, end) in 'flow',
>>> @@ -322,11 +324,12 @@ flow_hash_in_minimask_range(const struct flow *flow,
>>> const struct minimask *mask,
>>> uint8_t start, uint8_t end, uint32_t *basis)
>>> {
>>> +const uint32_t *mask_values = miniflow_get_u32_values(&mask->masks);
>>> const uint32_t *flow_u32 = (const uint32_t *)flow;
>>> unsigned int offset;
>>> uint64_t map = miniflow_get_map_in_range(&mask->masks, start, end,
>>> &offset);
>>> -const uint32_t *p = mask->masks.values + offset;
>>> +const uint32_t *p = mask_values + offset;
>>> uint32_t hash = *basis;
>>>
>>> for (; map; map = zero_rightmost_1bit(map)) {
>>> @@ -334,7 +337,7 @@ flow_hash_in_minimask_range(const struct flow *flow,
>>> }
>>>
>>> *basis = hash; /* Allow continuation from the unfinished value. */
>>> -return mhash_finish(hash, (p - mask->masks.values) * 4);
>>> +return mhash_finish(hash, (p - mask_values) * 4);
>>> }
>>>
>>> /* Fold minimask 'mask''s wildcard mask into &#x
[ovs-dev] cmap.c gcc 4.9 compile error
Hi Ben,
When I compile ovs src on the master branch with gcc 4.9, I get this error:
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I ./include -I ./lib -I
./lib -I/usr/include -Wstrict-prototypes -Wall -Wextra
-Wno-sign-compare -Wpointer-arith -Wdeclaration-after-statement
-Wno-format-zero-length -Wswitch-enum -Wunused-parameter
-Wstrict-aliasing -Wbad-function-cast -Wcast-align
-Wmissing-prototypes -Wmissing-field-initializers -g -O2 -MT
lib/cmap.lo -MD -MP -MF lib/.deps/cmap.Tpo -c lib/cmap.c -o lib/cmap.o
In file included from lib/ovs-atomic-c11.h:23:0,
from lib/ovs-atomic.h:252,
from lib/ovs-rcu.h:110,
from lib/cmap.h:22,
from lib/cmap.c:18:
lib/cmap.c: In function 'cmap_find':
lib/ovs-atomic-c11.h:31:15: error: invalid memory model for '__atomic_load'
(*(DST) = atomic_load_explicit(SRC, ORDER), \
^
lib/cmap.c:252:5: note: in expansion of macro 'atomic_read_explicit'
atomic_read_explicit(&bucket->counter, &counter, order);
^
lib/ovs-atomic-c11.h:31:15: error: invalid memory model for '__atomic_load'
(*(DST) = atomic_load_explicit(SRC, ORDER), \
^
lib/cmap.c:252:5: note: in expansion of macro 'atomic_read_explicit'
atomic_read_explicit(&bucket->counter, &counter, order);
^
lib/ovs-atomic-c11.h:31:15: error: invalid memory model for '__atomic_load'
(*(DST) = atomic_load_explicit(SRC, ORDER), \
^
lib/cmap.c:252:5: note: in expansion of macro 'atomic_read_explicit'
atomic_read_explicit(&bucket->counter, &counter, order);
^
lib/ovs-atomic-c11.h:31:15: error: invalid memory model for '__atomic_load'
(*(DST) = atomic_load_explicit(SRC, ORDER), \
^
lib/cmap.c:252:5: note: in expansion of macro 'atomic_read_explicit'
atomic_read_explicit(&bucket->counter, &counter, order);
^
Makefile:3007: recipe for target 'lib/cmap.lo' failed
make[2]: *** [lib/cmap.lo] Error 1
make[2]: Leaving directory '/home/agong/src/openvswitch'
Makefile:3547: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/agong/src/openvswitch'
Makefile:1886: recipe for target 'all' failed
make: *** [all] Error 2
After look into cmap.c, I think that you might have a typo in counter_changed:
static bool
counter_changed(struct cmap_bucket *b, uint32_t c)
{
return OVS_UNLIKELY(read_counter(b, memory_order_release) != c);
// you probably meant memory_order_acquire ?
}
➜ ~ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-unknown-linux-gnu/4.9.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: /build/gcc/src/gcc-4.9-20140507/configure
--prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib
--mandir=/usr/share/man --infodir=/usr/share/info
--with-bugurl=https://bugs.archlinux.org/
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++
--enable-shared --enable-threads=posix --with-system-zlib
--enable-__cxa_atexit --disable-libunwind-exceptions
--enable-clocale=gnu --disable-libstdcxx-pch --disable-libssp
--enable-gnu-unique-object --enable-linker-build-id
--enable-cloog-backend=isl --disable-cloog-version-check --enable-lto
--enable-plugin --enable-install-libiberty
--with-linker-hash-style=gnu --disable-multilib --disable-werror
--enable-checking=release
Thread model: posix
gcc version 4.9.0 20140507 (prerelease) (GCC)
Thanks,
Kmindg
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovs-discuss] dl_src field didn't function
On Thu, May 22, 2014 at 4:54 PM, chen zhang <3zhangchen9...@gmail.com> wrote: > but i know that when a ofp_match has more specified fields than another,it > would be chosen to function even with the same priority.is it true? Please do not drop the list. No. When a packet matches multiple flows, the highest priority of flow will be chosen. > > > 2014-05-22 16:12 GMT+08:00 Kmindg G : > >> On Thu, May 22, 2014 at 3:21 PM, chen zhang <3zhangchen9...@gmail.com> >> wrote: >> > hi, everybody >> > this is my table entry: >> > fnl@fnl-sdn:~$ sudo ovs-ofctl dump-flows s1 -O OpenFlow13 table=1 >> > OFPST_FLOW reply (OF1.3) (xid=0x2): >> > cookie=0x0, duration=68.077s, table=1, n_packets=13, n_bytes=962, >> > tcp,dl_vlan=3,tp_dst=80 actions=drop >> > cookie=0x0, duration=68.077s, table=1, n_packets=0, n_bytes=0, >> > tcp,dl_vlan=3,dl_src=00:00:00:00:00:01,tp_dst=80 actions=goto_table:2 >> > >> > >> > the first entry works well,while the second which is more specified with >> > dl_src field didn't catch the pkts from host 00:00:00:00:00:01 in vlan >> > 3,and >> > the pkts is matched by the first entry. >> > how did it come?and what should i do to make it correct? >> > >> Those two flows have the same priority, so packets match one flow >> arbitrarily. >> In this case, you should specify higher priority to the second flow >> than the fisrt one. >> > >> > ___ >> > discuss mailing list >> > disc...@openvswitch.org >> > http://openvswitch.org/mailman/listinfo/discuss >> > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovs-discuss] dl_src field didn't function
On Thu, May 22, 2014 at 6:00 PM, chen zhang <3zhangchen9...@gmail.com> wrote: > is it true?but when i specify the table like this: > cookie=0x0, duration=68.077s, table=1, n_packets=13, n_bytes=962, in_port=1 > actions=goto_table:3 > cookie=0x0, duration=68.077s, table=1, n_packets=0, n_bytes=0, > in_port=1,dl_src=00:00:00:00:00:01 actions=goto_table:2 > > the latter entry works well though it is at the default priority level "Those two flows have the same priority, so packets match one flow arbitrarily." "Arbitrarily" means that you can not predict which flow will be chosen. It does not mean one flow will not be chosen forever. > > > 2014-05-22 17:46 GMT+08:00 Kmindg G : > >> On Thu, May 22, 2014 at 4:54 PM, chen zhang <3zhangchen9...@gmail.com> >> wrote: >> > but i know that when a ofp_match has more specified fields than >> > another,it >> > would be chosen to function even with the same priority.is it true? >> >> Please do not drop the list. >> No. When a packet matches multiple flows, the highest priority of >> flow will be chosen. >> > >> > >> > 2014-05-22 16:12 GMT+08:00 Kmindg G : >> > >> >> On Thu, May 22, 2014 at 3:21 PM, chen zhang <3zhangchen9...@gmail.com> >> >> wrote: >> >> > hi, everybody >> >> > this is my table entry: >> >> > fnl@fnl-sdn:~$ sudo ovs-ofctl dump-flows s1 -O OpenFlow13 table=1 >> >> > OFPST_FLOW reply (OF1.3) (xid=0x2): >> >> > cookie=0x0, duration=68.077s, table=1, n_packets=13, n_bytes=962, >> >> > tcp,dl_vlan=3,tp_dst=80 actions=drop >> >> > cookie=0x0, duration=68.077s, table=1, n_packets=0, n_bytes=0, >> >> > tcp,dl_vlan=3,dl_src=00:00:00:00:00:01,tp_dst=80 actions=goto_table:2 >> >> > >> >> > >> >> > the first entry works well,while the second which is more specified >> >> > with >> >> > dl_src field didn't catch the pkts from host 00:00:00:00:00:01 in >> >> > vlan >> >> > 3,and >> >> > the pkts is matched by the first entry. >> >> > how did it come?and what should i do to make it correct? >> >> > >> >> Those two flows have the same priority, so packets match one flow >> >> arbitrarily. >> >> In this case, you should specify higher priority to the second flow >> >> than the fisrt one. >> >> > >> >> > ___ >> >> > discuss mailing list >> >> > disc...@openvswitch.org >> >> > http://openvswitch.org/mailman/listinfo/discuss >> >> > >> > >> > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Question about miniflow_hash in lib/flow.c
Hi all, I have a little question about miniflow_hash(). At the end of miniflow_hash, "p - flow->values" is used as the second paramter of mhash_finish. But "p - flow->valuse" is not the number of bytes which has been added to hash in miniflow_hash. The second parameter should be "sizeof hash_map + count_1bits(hash_map) * sizeof *p". Is this a real problem or I'm missing something? diff --git a/lib/flow.c b/lib/flow.c index 06ba036..dc6f4b8 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -1680,7 +1680,8 @@ miniflow_hash(const struct miniflow *flow, uint32_t basis) hash = mhash_add(hash, hash_map); hash = mhash_add(hash, hash_map >> 32); -return mhash_finish(hash, p - flow->values); +return mhash_finish(hash, +sizeof hash_map + count_1bits(hash_map) * sizeof *p); } cheers, kmindg ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Question about miniflow_hash in lib/flow.c
On Sun, Feb 9, 2014 at 9:56 AM, Jarno Rajahalme wrote: > >> On Feb 8, 2014, at 12:49 AM, Kmindg G wrote: >> >> Hi all, >> I have a little question about miniflow_hash(). >> At the end of miniflow_hash, "p - flow->values" is used as the second >> paramter of mhash_finish. But "p - flow->valuse" is not the number of >> bytes which has been added to hash in miniflow_hash. The second >> parameter should be "sizeof hash_map + count_1bits(hash_map) * sizeof >> *p". >> Is this a real problem or I'm missing something? >> > > It is not a real problem. The reason for adding the count of bytes at the > finishing step is to differentiate between inputs of different lengths that > would otherwise be made the same due to zero padding to 32 bits. > > Miniflows are always a multiple of 32 bits, so any consistent value for the > given input is fine. Thanks for explanation. > > Jarno > >> >> diff --git a/lib/flow.c b/lib/flow.c >> index 06ba036..dc6f4b8 100644 >> --- a/lib/flow.c >> +++ b/lib/flow.c >> @@ -1680,7 +1680,8 @@ miniflow_hash(const struct miniflow *flow, uint32_t >> basis) >> hash = mhash_add(hash, hash_map); >> hash = mhash_add(hash, hash_map >> 32); >> >> -return mhash_finish(hash, p - flow->values); >> +return mhash_finish(hash, >> +sizeof hash_map + count_1bits(hash_map) * sizeof >> *p); >> } >> >> cheers, >> kmindg >> ___ >> 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] ofproto: Send port status message for port-mods, right away.
On Thu, Feb 20, 2014 at 3:20 AM, Ben Pfaff wrote:
> Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has waited
> for ports to notify it that their status has changed before it sends a
> port status update to controllers.
>
> Also, Open vSwitch never sent port config updates at all for port
> modifications other than OFPPC_PORT_DOWN. I guess that this is a relic
> from the era when there was only one controller, since presumably the
> controller already knew that it changed the port configuration. But in the
> multi-controller world, it makes sense to send such port status updates,
> and I couldn't quickly find anything in OF1.3 or OF1.4 that said they
> shouldn't be sent.
>
> Signed-off-by: Ben Pfaff
> Reported-by: Kmindg G
> ---
> AUTHORS |1 +
> ofproto/ofproto.c | 25 +
> 2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index c557303..2fda8d7 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -197,6 +197,7 @@ John Hurley john.hur...@netronome.com
> Kevin Mancuso kevin.manc...@rackspace.com
> Kiran Shanbhog ki...@vmware.com
> Kirill Kabardin
> +Kmindg Gkmi...@gmail.com
> Koichi Yagishitayagishita.koi...@jrc.co.jp
> Konstantin Khorenko khore...@openvz.org
> Kris zhang zhang.k...@gmail.com
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 62c97a2..48e10ca 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2986,22 +2986,23 @@ update_port_config(struct ofport *port,
> enum ofputil_port_config config,
> enum ofputil_port_config mask)
> {
> -enum ofputil_port_config old_config = port->pp.config;
> -enum ofputil_port_config toggle;
> -
> -toggle = (config ^ port->pp.config) & mask;
> -if (toggle & OFPUTIL_PC_PORT_DOWN) {
> -if (config & OFPUTIL_PC_PORT_DOWN) {
> -netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL);
> -} else {
> -netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL);
> -}
> +enum ofputil_port_config toggle = (config ^ port->pp.config) & mask;
> +
> +if (toggle & OFPUTIL_PC_PORT_DOWN
> +&& (config & OFPUTIL_PC_PORT_DOWN
> +? netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL)
> +: netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL))) {
> +/* We tried to bring the port up or down, but it failed, so don't
> + * update the "down" bit. */
> toggle &= ~OFPUTIL_PC_PORT_DOWN;
> }
>
> -port->pp.config ^= toggle;
> -if (port->pp.config != old_config) {
> +if (toggle) {
> +enum ofputil_port_config old_config = port->pp.config;
> +port->pp.config ^= toggle;
> port->ofproto->ofproto_class->port_reconfigured(port, old_config);
> +connmgr_send_port_status(port->ofproto->connmgr, &port->pp,
> + OFPPR_MODIFY);
> }
> }
>
> --
> 1.7.10.4
>
looks good to me.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto: Send port status message for port-mods, right away.
On Fri, Feb 21, 2014 at 5:19 AM, Ben Pfaff wrote:
> On Thu, Feb 20, 2014 at 12:45:49PM +0800, Kmindg G wrote:
>> On Thu, Feb 20, 2014 at 3:20 AM, Ben Pfaff wrote:
>> > Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has waited
>> > for ports to notify it that their status has changed before it sends a
>> > port status update to controllers.
>> >
>> > Also, Open vSwitch never sent port config updates at all for port
>> > modifications other than OFPPC_PORT_DOWN. I guess that this is a relic
>> > from the era when there was only one controller, since presumably the
>> > controller already knew that it changed the port configuration. But in the
>> > multi-controller world, it makes sense to send such port status updates,
>> > and I couldn't quickly find anything in OF1.3 or OF1.4 that said they
>> > shouldn't be sent.
>> >
>> > Signed-off-by: Ben Pfaff
>> > Reported-by: Kmindg G
>> > ---
>> > AUTHORS |1 +
>> > ofproto/ofproto.c | 25 +
>> > 2 files changed, 14 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/AUTHORS b/AUTHORS
>> > index c557303..2fda8d7 100644
>> > --- a/AUTHORS
>> > +++ b/AUTHORS
>> > @@ -197,6 +197,7 @@ John Hurley john.hur...@netronome.com
>> > Kevin Mancuso kevin.manc...@rackspace.com
>> > Kiran Shanbhog ki...@vmware.com
>> > Kirill Kabardin
>> > +Kmindg Gkmi...@gmail.com
>> > Koichi Yagishitayagishita.koi...@jrc.co.jp
>> > Konstantin Khorenko khore...@openvz.org
>> > Kris zhang zhang.k...@gmail.com
>> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> > index 62c97a2..48e10ca 100644
>> > --- a/ofproto/ofproto.c
>> > +++ b/ofproto/ofproto.c
>> > @@ -2986,22 +2986,23 @@ update_port_config(struct ofport *port,
>> > enum ofputil_port_config config,
>> > enum ofputil_port_config mask)
>> > {
>> > -enum ofputil_port_config old_config = port->pp.config;
>> > -enum ofputil_port_config toggle;
>> > -
>> > -toggle = (config ^ port->pp.config) & mask;
>> > -if (toggle & OFPUTIL_PC_PORT_DOWN) {
>> > -if (config & OFPUTIL_PC_PORT_DOWN) {
>> > -netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL);
>> > -} else {
>> > -netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL);
>> > -}
>> > +enum ofputil_port_config toggle = (config ^ port->pp.config) & mask;
>> > +
>> > +if (toggle & OFPUTIL_PC_PORT_DOWN
>> > +&& (config & OFPUTIL_PC_PORT_DOWN
>> > +? netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL)
>> > +: netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL))) {
>> > +/* We tried to bring the port up or down, but it failed, so don't
>> > + * update the "down" bit. */
>> > toggle &= ~OFPUTIL_PC_PORT_DOWN;
>> > }
>> >
>> > -port->pp.config ^= toggle;
>> > -if (port->pp.config != old_config) {
>> > +if (toggle) {
>> > +enum ofputil_port_config old_config = port->pp.config;
>> > +port->pp.config ^= toggle;
>> > port->ofproto->ofproto_class->port_reconfigured(port, old_config);
>> > +connmgr_send_port_status(port->ofproto->connmgr, &port->pp,
>> > + OFPPR_MODIFY);
>> > }
>> > }
>> >
>> > --
>> > 1.7.10.4
>> >
>>
>> looks good to me.
>
> Thanks. What do you think of this version? I think that it retains
> better compatibility with OpenFlow 1.0 in particular.
>
> --8<--cut here-->8--
>
> From: Ben Pfaff
> Date: Thu, 20 Feb 2014 13:18:51 -0800
> Subject: [PATCH] ofproto: Send port status message for port-mods, right away.
>
> Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has waited
> for ports to notify it that their status has changed before it sends a
> port status update to controllers.
>
> Also, Open vSwitch never sent port config updates at all for port
> modifications other than OFPPC_PORT_DOWN. I guess that this is a relic
> from the era when there was only one controller, since
Re: [ovs-dev] [PATCH] ofproto: Send port status message for port-mods, right away.
On Sat, Feb 22, 2014 at 12:42 AM, Ben Pfaff wrote:
> On Fri, Feb 21, 2014 at 10:07:14AM +0800, Kmindg G wrote:
>> On Fri, Feb 21, 2014 at 5:19 AM, Ben Pfaff wrote:
>> > On Thu, Feb 20, 2014 at 12:45:49PM +0800, Kmindg G wrote:
>> >> On Thu, Feb 20, 2014 at 3:20 AM, Ben Pfaff wrote:
>> >> > Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has
>> >> > waited
>> >> > for ports to notify it that their status has changed before it sends a
>> >> > port status update to controllers.
>> >> >
>> >> > Also, Open vSwitch never sent port config updates at all for port
>> >> > modifications other than OFPPC_PORT_DOWN. I guess that this is a relic
>> >> > from the era when there was only one controller, since presumably the
>> >> > controller already knew that it changed the port configuration. But in
>> >> > the
>> >> > multi-controller world, it makes sense to send such port status updates,
>> >> > and I couldn't quickly find anything in OF1.3 or OF1.4 that said they
>> >> > shouldn't be sent.
>> >> >
>> >> > Signed-off-by: Ben Pfaff
>> >> > Reported-by: Kmindg G
>> >> > ---
>> >> > AUTHORS |1 +
>> >> > ofproto/ofproto.c | 25 +
>> >> > 2 files changed, 14 insertions(+), 12 deletions(-)
>> >> >
>> >> > diff --git a/AUTHORS b/AUTHORS
>> >> > index c557303..2fda8d7 100644
>> >> > --- a/AUTHORS
>> >> > +++ b/AUTHORS
>> >> > @@ -197,6 +197,7 @@ John Hurley john.hur...@netronome.com
>> >> > Kevin Mancuso kevin.manc...@rackspace.com
>> >> > Kiran Shanbhog ki...@vmware.com
>> >> > Kirill Kabardin
>> >> > +Kmindg Gkmi...@gmail.com
>> >> > Koichi Yagishitayagishita.koi...@jrc.co.jp
>> >> > Konstantin Khorenko khore...@openvz.org
>> >> > Kris zhang zhang.k...@gmail.com
>> >> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> >> > index 62c97a2..48e10ca 100644
>> >> > --- a/ofproto/ofproto.c
>> >> > +++ b/ofproto/ofproto.c
>> >> > @@ -2986,22 +2986,23 @@ update_port_config(struct ofport *port,
>> >> > enum ofputil_port_config config,
>> >> > enum ofputil_port_config mask)
>> >> > {
>> >> > -enum ofputil_port_config old_config = port->pp.config;
>> >> > -enum ofputil_port_config toggle;
>> >> > -
>> >> > -toggle = (config ^ port->pp.config) & mask;
>> >> > -if (toggle & OFPUTIL_PC_PORT_DOWN) {
>> >> > -if (config & OFPUTIL_PC_PORT_DOWN) {
>> >> > -netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL);
>> >> > -} else {
>> >> > -netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL);
>> >> > -}
>> >> > +enum ofputil_port_config toggle = (config ^ port->pp.config) &
>> >> > mask;
>> >> > +
>> >> > +if (toggle & OFPUTIL_PC_PORT_DOWN
>> >> > +&& (config & OFPUTIL_PC_PORT_DOWN
>> >> > +? netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL)
>> >> > +: netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL))) {
>> >> > +/* We tried to bring the port up or down, but it failed, so
>> >> > don't
>> >> > + * update the "down" bit. */
>> >> > toggle &= ~OFPUTIL_PC_PORT_DOWN;
>> >> > }
>> >> >
>> >> > -port->pp.config ^= toggle;
>> >> > -if (port->pp.config != old_config) {
>> >> > +if (toggle) {
>> >> > +enum ofputil_port_config old_config = port->pp.config;
>> >> > +port->pp.config ^= toggle;
>> >> > port->ofproto->ofproto_class->port_reconfigured(port,
>> >> > old_config);
>> >> > +connmgr_send_port_status(port->ofproto->connmgr, &port->pp,
>> >> > + OFPPR_MODIFY);
>> >> >
Re: [ovs-dev] [PATCH] ofproto: Update rule's priority in eviction group.
On Wed, Mar 12, 2014 at 1:36 PM, Ben Pfaff wrote: > On Sun, Mar 09, 2014 at 05:48:04PM +0800, kmindg wrote: >> We do call heap_rebuild in ofproto_run, but we do not update rule's >> priority with latest hard_timeout and idle_timeout before heap_rebuild. >> >> This patch ensures that rule's priority has been updated before >> heap_rebuild, and adds two test cases to check eviction with modified >> hard_timeout and idle_timwout. >> >> Signed-off-by: kmindg > > Good catch. I applied this. > > I left the declaration of heap_raw_change() in heap.h. I like to be > able to see a summary of all the available functions at the top of the > header, even if some of the functions have inline implementations later > in the header. OK, I understand that now. :) Thanks for applying this patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] stp: Fix bpdu tx problem in listening state
On Wed, Mar 12, 2014 at 1:41 PM, Ben Pfaff wrote: > On Sun, Mar 09, 2014 at 05:48:52PM +0800, kmindg wrote: >> The restriction only allows to send bpdu in forwarding state in >> compose_output_action__. But a port could send bpdu in listening >> and learning state according to comments in lib/stp.h(State of >> an STP port). >> >> Signed-off-by: kmindg > > This appears to fix a bug, but I'm not sure exactly how bad that bug is, > or what the symptoms of it are. Does this patch mean that currently OVS > isn't sending out BPDUs in certain STP states? (In other words, that > STP is currently broken?) Yes, currently OVS isn't sending out BPDUs in listening and learning states. But those two states are temporary, the stp port will be in forwarding state and send out BPDUs eventually (In the default configuration listening and learning states last 15+15 second). So this minor problem may only increase the convergence time, in my opinion. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] stp: Fix bpdu tx problem in listening state
On Sun, Mar 16, 2014 at 12:50 AM, Ben Pfaff wrote: > On Wed, Mar 12, 2014 at 03:31:26PM +0800, Kmindg G wrote: >> On Wed, Mar 12, 2014 at 1:41 PM, Ben Pfaff wrote: >> > On Sun, Mar 09, 2014 at 05:48:52PM +0800, kmindg wrote: >> >> The restriction only allows to send bpdu in forwarding state in >> >> compose_output_action__. But a port could send bpdu in listening >> >> and learning state according to comments in lib/stp.h(State of >> >> an STP port). >> >> >> >> Signed-off-by: kmindg >> > >> > This appears to fix a bug, but I'm not sure exactly how bad that bug is, >> > or what the symptoms of it are. Does this patch mean that currently OVS >> > isn't sending out BPDUs in certain STP states? (In other words, that >> > STP is currently broken?) >> >> Yes, currently OVS isn't sending out BPDUs in listening and learning states. >> But those two states are temporary, the stp port will be in forwarding state >> and send out BPDUs eventually (In the default configuration listening and >> learning states last 15+15 second). >> >> So this minor problem may only increase the convergence time, in my opinion. > > Thanks for the extra explanation. I added that to the commit message > and applied this to master, branch-2.1, and branch-2.0. Thanks for applying. :) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovs-discuss] How to Search in Archives?
On Fri, Mar 14, 2014 at 7:13 PM, Rizwan Jamil wrote: > Hi! > > Search is provided in mininet-discuss and openflow-discuss mailing archives. > But why it has not been provided in ovs-dev and ovs-discuss archives? How > can I search my question in ovs-dev and ovs-discuss? google always is your friend. Search this "Rizwan site:openvswitch.org" in google > > Regards, > Rizwan Jamil > > ___ > discuss mailing list > disc...@openvswitch.org > http://openvswitch.org/mailman/listinfo/discuss > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/5] ofproto.at: Fix races in rule eviciton tests
On Mon, Mar 31, 2014 at 2:24 PM, YAMAMOTO Takashi
wrote:
> Bump timeout differences, because timeouts different by 1s might end up
> to have the same position in the heap as rule_eviction_priority() uses
> 1024ms as a unit.
>
> Also, use time/stop to avoid relying on how long an add-flow would take.
>
> These tests were introduced by commit 6d56c1f1.
> ("ofproto: Update rule's priority in eviction group.")
>
> Signed-off-by: YAMAMOTO Takashi
> ---
> tests/ofproto.at | 42 ++
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index e98b526..46ae9e9 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1375,27 +1375,28 @@ AT_CHECK(
> | ${PERL} $srcdir/uuidfilt.pl],
>[0], [<0>
> ])
> +ovs-appctl time/stop
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> hard_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0
> hard_timeout=1${in_port}0,in_port=$in_port,actions=drop
> done
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=12, in_port=2 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=110, in_port=1 actions=drop
> + hard_timeout=120, in_port=2 actions=drop
> + hard_timeout=130, in_port=3 actions=drop
> + hard_timeout=140, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and modify the one that expires soonest
> -sleep 2
> +ovs-appctl time/warp 2
I think we should warp as small as possible to verify precision.
What do you think about warp 12000?
> AT_CHECK([ovs-ofctl mod-flows br0 in_port=1,actions=drop])
> -sleep 2
> +ovs-appctl time/warp 2
FYI: This warp is fine as far as it's greater than 1000, but is no
harm to choose a bigger one.
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=110, in_port=1 actions=drop
> + hard_timeout=130, in_port=3 actions=drop
> + hard_timeout=140, in_port=4 actions=drop
> in_port=5 actions=drop
> NXST_FLOW reply:
> ])
> @@ -1414,26 +1415,27 @@ AT_CHECK(
> ])
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> idle_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0
> idle_timeout=1${in_port}0,in_port=$in_port,actions=drop
> done
> +ovs-appctl time/stop
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=11, in_port=1 actions=drop
> - idle_timeout=12, in_port=2 actions=drop
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=110, in_port=1 actions=drop
> + idle_timeout=120, in_port=2 actions=drop
> + idle_timeout=130, in_port=3 actions=drop
> + idle_timeout=140, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and receive on the flow that expires soonest
> -sleep 2
> +ovs-appctl time/warp 2
Same comment as above.
> AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1)'])
> -sleep 2
> +ovs-appctl time/warp 2
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=130, in_port=3 actions=drop
> + idle_timeout=140, in_port=4 actions=drop
> in_port=5 actions=drop
> - n_packets=1, n_bytes=60, idle_timeout=11, in_port=1 actions=drop
> + n_packets=1, n_bytes=60, idle_timeout=110, in_port=1 actions=drop
> NXST_FLOW reply:
> ])
> OVS_VSWITCHD_STOP
> --
> 1.8.3.1
>
> ___
> 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 4/5] ofproto.at: Fix races in rule eviciton tests
On Tue, Apr 1, 2014 at 1:12 PM, YAMAMOTO Takashi wrote:
>>> # Sleep and modify the one that expires soonest
>>> -sleep 2
>>> +ovs-appctl time/warp 2
>>
>> I think we should warp as small as possible to verify precision.
>> What do you think about warp 12000?
>
> i agree smaller is better.
> how about the following?
>
> YAMAMOTO Takashi
>
>
> commit b4a624761fe90aa0d57264013287f4236077ac9c
> Author: YAMAMOTO Takashi
> Date: Mon Mar 31 14:04:35 2014 +0900
>
> ofproto.at: Fix races in rule eviciton tests
>
> Bump timeout differences, because timeouts different by 1s might end up
> to have the same position in the heap as rule_eviction_priority() uses
> 1024ms as a unit.
>
> Also, use time/stop to avoid relying on how long an add-flow would take.
>
> These tests were introduced by commit 6d56c1f1.
> ("ofproto: Update rule's priority in eviction group.")
>
> Signed-off-by: YAMAMOTO Takashi
> Cc: Kmindg G
> Acked-by: Ben Pfaff
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index e98b526..a5116db 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1375,27 +1375,28 @@ AT_CHECK(
> | ${PERL} $srcdir/uuidfilt.pl],
>[0], [<0>
> ])
> +ovs-appctl time/stop
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> hard_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0 hard_timeout=$((10 + in_port *
> 2)),in_port=$in_port,actions=drop
> done
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=12, in_port=2 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=12, in_port=1 actions=drop
> + hard_timeout=14, in_port=2 actions=drop
> + hard_timeout=16, in_port=3 actions=drop
> + hard_timeout=18, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and modify the one that expires soonest
> -sleep 2
> +ovs-appctl time/warp 3000
> AT_CHECK([ovs-ofctl mod-flows br0 in_port=1,actions=drop])
at this point, flow table would be:
hard_timeout=12, in_port=1 actions=drop
hard_timeout=11, in_port=2 actions=drop
hard_timeout=13, in_port=3 actions=drop
hard_timeout=15, in_port=4 actions=drop
And according to your comment, 1s interval may not be suitable.
I would like to change (10 + in_port * 2) to (10 + in_port * 3), and warp 4000.
What do you think?
Thanks.
> -sleep 2
> +ovs-appctl time/warp 2000
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=12, in_port=1 actions=drop
> + hard_timeout=16, in_port=3 actions=drop
> + hard_timeout=18, in_port=4 actions=drop
> in_port=5 actions=drop
> NXST_FLOW reply:
> ])
> @@ -1414,26 +1415,27 @@ AT_CHECK(
> ])
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> idle_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0 idle_timeout=$((10 + in_port *
> 2)),in_port=$in_port,actions=drop
> done
> +ovs-appctl time/stop
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=11, in_port=1 actions=drop
> - idle_timeout=12, in_port=2 actions=drop
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=12, in_port=1 actions=drop
> + idle_timeout=14, in_port=2 actions=drop
> + idle_timeout=16, in_port=3 actions=drop
> + idle_timeout=18, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and receive on the flow that expires soonest
> -sleep 2
> +ovs-appctl time/warp 3000
> AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1)'])
> -sleep 2
> +ovs-appctl time/warp 2000
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=16, in_port=3 actions=drop
> + idle_timeout=18, in_port=4 actions=drop
> in_port=5 actions=drop
> - n_packets=1, n_bytes=60, idle_timeout=11, in_port=1 actions=drop
> + n_packets=1, n_bytes=60, idle_timeout=12, in_port=1 actions=drop
> NXST_FLOW reply:
> ])
> OVS_VSWITCHD_STOP
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/5] ofproto.at: Fix races in rule eviciton tests
On Tue, Apr 1, 2014 at 6:20 PM, YAMAMOTO Takashi wrote:
>>> # Sleep and modify the one that expires soonest
>>> -sleep 2
>>> +ovs-appctl time/warp 3000
>>> AT_CHECK([ovs-ofctl mod-flows br0 in_port=1,actions=drop])
>>
>> at this point, flow table would be:
>> hard_timeout=12, in_port=1 actions=drop
>> hard_timeout=11, in_port=2 actions=drop
>> hard_timeout=13, in_port=3 actions=drop
>> hard_timeout=15, in_port=4 actions=drop
>> And according to your comment, 1s interval may not be suitable.
>>
>> I would like to change (10 + in_port * 2) to (10 + in_port * 3), and warp
>> 4000.
>> What do you think?
>
> good point. i tweaked timeouts and comments.
>
> YAMAMOTO Takashi
>
>
> commit b048a2bc1e8af76dcc4b2870a76a0b2f02876188
> Author: YAMAMOTO Takashi
> Date: Mon Mar 31 14:04:35 2014 +0900
>
> ofproto.at: Fix races in rule eviciton tests
>
> Bump timeout differences, because timeouts different by 1s might end up
> to have the same position in the heap as rule_eviction_priority() uses
> 1024ms as a unit.
>
> Also, use time/stop to avoid relying on how long an add-flow would take.
>
> These tests were introduced by commit 6d56c1f1.
> ("ofproto: Update rule's priority in eviction group.")
>
> Signed-off-by: YAMAMOTO Takashi
> Cc: Kmindg G
> Acked-by: Ben Pfaff
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index e98b526..23629c0 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1375,27 +1375,34 @@ AT_CHECK(
> | ${PERL} $srcdir/uuidfilt.pl],
>[0], [<0>
> ])
> +ovs-appctl time/stop
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> hard_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0 hard_timeout=$((10 + in_port *
> 3)),in_port=$in_port,actions=drop
> done
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=12, in_port=2 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=13, in_port=1 actions=drop
> + hard_timeout=16, in_port=2 actions=drop
> + hard_timeout=19, in_port=3 actions=drop
> + hard_timeout=22, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and modify the one that expires soonest
> -sleep 2
> +ovs-appctl time/warp 4000
> AT_CHECK([ovs-ofctl mod-flows br0 in_port=1,actions=drop])
> -sleep 2
> +# At this point the table would looks like:
> +# in_port seconds to expire
> +# 113
> +# 211
This seems to be 12s to expire. And I realize that I made a mistake
about warp 4000.
It should be warp 5000 or even bigger.
I'm so sorry to bother you so many times. I probably need a rest.
Thanks for all your work.
> +# 315
> +# 418
> +ovs-appctl time/warp 2000
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=13, in_port=1 actions=drop
> + hard_timeout=19, in_port=3 actions=drop
> + hard_timeout=22, in_port=4 actions=drop
> in_port=5 actions=drop
> NXST_FLOW reply:
> ])
> @@ -1414,26 +1421,33 @@ AT_CHECK(
> ])
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> idle_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0 idle_timeout=$((10 + in_port *
> 3)),in_port=$in_port,actions=drop
> done
> +ovs-appctl time/stop
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=11, in_port=1 actions=drop
> - idle_timeout=12, in_port=2 actions=drop
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=13, in_port=1 actions=drop
> + idle_timeout=16, in_port=2 actions=drop
> + idle_timeout=19, in_port=3 actions=drop
> + idle_timeout=22, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and receive on the flow that expires soonest
> -sleep 2
> +ovs-appctl time/warp 4000
> AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1)'])
> -sleep 2
> +# At this point the table would looks like:
> +# in_port seconds to expire
> +# 113
> +# 211
> +# 315
> +# 418
> +ovs-appctl time/warp 2000
>
Re: [ovs-dev] [PATCH 4/5] ofproto.at: Fix races in rule eviciton tests
On Tue, Apr 1, 2014 at 8:38 PM, YAMAMOTO Takashi wrote:
>>> +ovs-appctl time/warp 4000
>>> AT_CHECK([ovs-ofctl mod-flows br0 in_port=1,actions=drop])
>>> -sleep 2
>>> +# At this point the table would looks like:
>>> +# in_port seconds to expire
>>> +# 113
>>> +# 211
>> This seems to be 12s to expire. And I realize that I made a mistake
>> about warp 4000.
>> It should be warp 5000 or even bigger.
>> I'm so sorry to bother you so many times. I probably need a rest.
>> Thanks for all your work.
>
> apparently this math is too difficult for us. :-)
>
> YAMAMOTO Takashi
>
> commit 06f0a9e0e4da144e0bdc3a3b49d5f2572f67c626
> Author: YAMAMOTO Takashi
> Date: Mon Mar 31 14:04:35 2014 +0900
>
> ofproto.at: Fix races in rule eviciton tests
>
> Bump timeout differences, because timeouts different by 1s might end up
> to have the same position in the heap as rule_eviction_priority() uses
> 1024ms as a unit.
>
> Also, use time/stop to avoid relying on how long an add-flow would take.
>
> These tests were introduced by commit 6d56c1f1.
> ("ofproto: Update rule's priority in eviction group.")
>
> Signed-off-by: YAMAMOTO Takashi
> Cc: Kmindg G
> Acked-by: Ben Pfaff
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index e98b526..de2d004 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1375,27 +1375,34 @@ AT_CHECK(
> | ${PERL} $srcdir/uuidfilt.pl],
>[0], [<0>
> ])
> +ovs-appctl time/stop
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> hard_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0 hard_timeout=$((10 + in_port *
> 3)),in_port=$in_port,actions=drop
> done
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=12, in_port=2 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=13, in_port=1 actions=drop
> + hard_timeout=16, in_port=2 actions=drop
> + hard_timeout=19, in_port=3 actions=drop
> + hard_timeout=22, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and modify the one that expires soonest
> -sleep 2
> +ovs-appctl time/warp 5000
> AT_CHECK([ovs-ofctl mod-flows br0 in_port=1,actions=drop])
> -sleep 2
> +# At this point the table would looks like:
> +# in_port seconds to expire
> +# 113
> +# 211
> +# 314
> +# 417
> +ovs-appctl time/warp 2000
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - hard_timeout=11, in_port=1 actions=drop
> - hard_timeout=13, in_port=3 actions=drop
> - hard_timeout=14, in_port=4 actions=drop
> + hard_timeout=13, in_port=1 actions=drop
> + hard_timeout=19, in_port=3 actions=drop
> + hard_timeout=22, in_port=4 actions=drop
> in_port=5 actions=drop
> NXST_FLOW reply:
> ])
> @@ -1414,26 +1421,33 @@ AT_CHECK(
> ])
> # Add 4 flows.
> for in_port in 4 3 2 1; do
> -ovs-ofctl add-flow br0
> idle_timeout=1${in_port},in_port=$in_port,actions=drop
> +ovs-ofctl add-flow br0 idle_timeout=$((10 + in_port *
> 3)),in_port=$in_port,actions=drop
> done
> +ovs-appctl time/stop
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=11, in_port=1 actions=drop
> - idle_timeout=12, in_port=2 actions=drop
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=13, in_port=1 actions=drop
> + idle_timeout=16, in_port=2 actions=drop
> + idle_timeout=19, in_port=3 actions=drop
> + idle_timeout=22, in_port=4 actions=drop
> NXST_FLOW reply:
> ])
> # Sleep and receive on the flow that expires soonest
> -sleep 2
> +ovs-appctl time/warp 5000
> AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1)'])
> -sleep 2
> +# At this point the table would looks like:
> +# in_port seconds to expire
> +# 113
> +# 211
> +# 314
> +# 417
> +ovs-appctl time/warp 2000
> # Adding another flow will cause the one that expires soonest to be evicted.
> AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop])
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> - idle_timeout=13, in_port=3 actions=drop
> - idle_timeout=14, in_port=4 actions=drop
> + idle_timeout=19, in_port=3 actions=drop
> + idle_timeout=22, in_port=4 actions=drop
> in_port=5 actions=drop
> - n_packets=1, n_bytes=60, idle_timeout=11, in_port=1 actions=drop
> + n_packets=1, n_bytes=60, idle_timeout=13, in_port=1 actions=drop
> NXST_FLOW reply:
> ])
> OVS_VSWITCHD_STOP
Acked-by: Kmindg G
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
