[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
[ovs-dev] make check failing on 2.x
Hello, 'ofproto - flow monitoring pause and resume' test failing continuously during make check on all 2.x major branches. The problem is in 'ovs-ofctl add-flows br0 flows.txt' execution which actually hangs a test switch because set count is insanely high and test execution actually hitting the timeout: wc -l ./tests/testsuite.dir/0613/flows.txt 1400833 ./tests/testsuite.dir/0613/flows.txt filled with cookie=1,reg1=[incrementing count],actions=drop Can anyone please fix this? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] tests: Don't rely on strace for IPv6 IDL testcases.
On Fri, Feb 07, 2014 at 03:38:07PM -0800, Joe Stringer wrote: > This would cause testsuite failures if someone runs the testsuite > without strace installed. > > Signed-off-by: Joe Stringer Oops, I was using that for local debugging and did not mean for it to be checked in. Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Question about VLOG
I add code in lib and ofproto direcotry. I wonder how to use VLOG? I include "vlog.h" and decalare VLOG_DEFINE_THIS_MODULE(FILENAME); Do I need more? Thanks ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Question about miniflow_hash in lib/flow.c
> 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. 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 v5 2/2] datapath: Per NUMA node flow stats.
On Thu, Feb 6, 2014 at 3:13 PM, Jarno Rajahalme wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index abe6789..e86034e 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -80,96 +76,126 @@ void ovs_flow_stats_update(struct sw_flow *flow, struct
> sk_buff *skb)
> tcp_flags = TCP_FLAGS_BE16(tcp_hdr(skb));
> }
>
> - spin_lock(&stats->lock);
> + /* Check if already have node-specific stats. */
> + if (likely(stats))
> + spin_lock(&stats->lock);
> + else {
> + stats = flow->stats[0]; /* Pre-allocated. */
> + spin_lock(&stats->lock);
> +
> + /* If the current NUMA-node is the only writer on the
> +* pre-allocated stats keep using them.
> +* A previous locker may have already allocated the stats,
> +* so we need to check again. If the node-specific stats
> +* were already allocated, we update the pre-allocated stats
> +* as we have already locked them. */
> + if (likely(stats->last_writer != node && stats->last_writer
> >= 0
> + && !flow->stats[node])) {
I don't know if this likely() annotation is right - it seems that it
would be more common that we are using node 0's stats than allocating.
> static int check_header(struct sk_buff *skb, int len)
> diff --git a/datapath/flow.h b/datapath/flow.h
> index eafcfd8..f6cce35 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -155,14 +155,9 @@ struct flow_stats {
> unsigned long used; /* Last used time (in jiffies). */
> spinlock_t lock;/* Lock for atomic stats update. */
> __be16 tcp_flags; /* Union of seen TCP flags. */
> -};
> -
> -struct sw_flow_stats {
> - bool is_percpu;
> - union {
> - struct flow_stats *stat;
> - struct flow_stats __percpu *cpu_stats;
> - };
> + int last_writer;/* NUMA-node id of the last writer or
> +* -1. Meaningful for 'stats[0]' only.
> +*/
Should we put last_writer directly in struct sw_flow stats? It would
reduce the size a little bit and might even be a little clearer.
> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index 4e6b1c0..3f0829c 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> @@ -608,16 +603,28 @@ int ovs_flow_init(void)
> BUILD_BUG_ON(__alignof__(struct sw_flow_key) % __alignof__(long));
> BUILD_BUG_ON(sizeof(struct sw_flow_key) % sizeof(long));
>
> - flow_cache = kmem_cache_create("sw_flow", sizeof(struct sw_flow), 0,
> - 0, NULL);
> + flow_cache = kmem_cache_create("sw_flow", sizeof(struct sw_flow)
> + + (num_possible_nodes()
> + * sizeof(struct flow_stats *)),
> + 0, SLAB_HWCACHE_ALIGN, NULL);
Why do we need to cache line align the flows? We shouldn't be writing
to them very often.
One other thing, it might be good to do a revert of the 5-tuple patch
first and then layer this on top as it would probably make it clearer
what is going on.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Do not read tcp flags from a later fragment.
On Thu, Feb 6, 2014 at 3:13 PM, Jarno Rajahalme wrote: > Only the first IP fragment can have a TCP header, check for this. > > Signed-off-by: Jarno Rajahalme Acked-by: Jesse Gross ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Question about VLOG
On Sat, Feb 08, 2014 at 06:32:56PM -0700, Junguk Cho wrote: > I add code in lib and ofproto direcotry. > > I wonder how to use VLOG? > I include "vlog.h" and decalare VLOG_DEFINE_THIS_MODULE(FILENAME); > Do I need more? That's all you need to do. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Question about VLOG
However, I recompiled ovs. I showed this message. "ofproto/libofproto.a(debug.o): In function `show_tnl_port': /home/ubuntu2/ovs/ofproto/debug.c:23: undefined reference to `VLM_debug'" Does anyone know reason? Thanks, 2014-02-08 21:58 GMT-07:00 Ben Pfaff : > On Sat, Feb 08, 2014 at 06:32:56PM -0700, Junguk Cho wrote: > > I add code in lib and ofproto direcotry. > > > > I wonder how to use VLOG? > > I include "vlog.h" and decalare VLOG_DEFINE_THIS_MODULE(FILENAME); > > Do I need more? > > That's all you need to do. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
