date:20161007

Re: [ovs-dev] [PATCH v10] ovn-nbctl: Add LB commands.

2016-10-07 Thread nickcooper-zhangtonghao

Good idea. I am interested in that. The patch will be submitted. Thanks.

> On Oct 8, 2016, at 2:09 AM, Guru Shetty  wrote:
> 
> On 5 October 2016 at 04:25, nickcooper-zhangtonghao 
>  > wrote:
> Thanks very much.
> 
> If it is of interest,  something similar can be done for NAT too.

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] meta-flow: Fix the NXM_NX_* names of xxreg2 and xxreg3.

2016-10-07 Thread Justin Pettit

That seems like a reasonable idea to me.  It's obviously not strictly 
necessary, but it's nice to be able to deal with contiguous ranges.

--Justin


> On Oct 7, 2016, at 5:44 PM, Jarno Rajahalme  wrote:
> 
> Should we also reserve some range of NXM_NX numbers after 114 for potential 
> future registers, say 115-118 at least? Other register types do not have this 
> problem as they have their own classes of numbers.
> 
>  Jarno
> 
>> On Oct 7, 2016, at 5:41 PM, Jarno Rajahalme  wrote:
>> 
>> xxreg2 and xxreg3 had the same NXM_NX_* names as xxreg0 and xxreg1,
>> correspondingly.
>> 
>> Found by inspection.
>> 
>> CC: Justin Pettit 
>> Fixes: b23ada8eecfd ("Introduce 128-bit xxregs.")
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> include/openvswitch/meta-flow.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/openvswitch/meta-flow.h 
>> b/include/openvswitch/meta-flow.h
>> index 966ff7f..ef07561 100644
>> --- a/include/openvswitch/meta-flow.h
>> +++ b/include/openvswitch/meta-flow.h
>> @@ -940,8 +940,8 @@ enum OVS_PACKED_ENUM mf_field_id {
>> * Access: read/write.
>> * NXM: NXM_NX_XXREG0(111) since v2.6.  <0>
>> * NXM: NXM_NX_XXREG1(112) since v2.6.  <1>
>> - * NXM: NXM_NX_XXREG0(113) since v2.6.  <2>
>> - * NXM: NXM_NX_XXREG1(114) since v2.6.  <3>
>> + * NXM: NXM_NX_XXREG2(113) since v2.6.  <2>
>> + * NXM: NXM_NX_XXREG3(114) since v2.6.  <3>
>> * OXM: none.
>> */
>>MFF_XXREG0,
>> -- 
>> 2.1.4
>> 
> 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] meta-flow: Fix the NXM_NX_* names of xxreg2 and xxreg3.

2016-10-07 Thread Justin Pettit


> On Oct 7, 2016, at 5:41 PM, Jarno Rajahalme  wrote:
> 
> xxreg2 and xxreg3 had the same NXM_NX_* names as xxreg0 and xxreg1,
> correspondingly.
> 
> Found by inspection.
> 
> CC: Justin Pettit 
> Fixes: b23ada8eecfd ("Introduce 128-bit xxregs.")
> Signed-off-by: Jarno Rajahalme 

Thanks!

Acked-by: Justin Pettit 

--Justin


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] meta-flow: Fix the NXM_NX_* names of xxreg2 and xxreg3.

2016-10-07 Thread Jarno Rajahalme

Should we also reserve some range of NXM_NX numbers after 114 for potential 
future registers, say 115-118 at least? Other register types do not have this 
problem as they have their own classes of numbers.

  Jarno

> On Oct 7, 2016, at 5:41 PM, Jarno Rajahalme  wrote:
> 
> xxreg2 and xxreg3 had the same NXM_NX_* names as xxreg0 and xxreg1,
> correspondingly.
> 
> Found by inspection.
> 
> CC: Justin Pettit 
> Fixes: b23ada8eecfd ("Introduce 128-bit xxregs.")
> Signed-off-by: Jarno Rajahalme 
> ---
> include/openvswitch/meta-flow.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index 966ff7f..ef07561 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -940,8 +940,8 @@ enum OVS_PACKED_ENUM mf_field_id {
>  * Access: read/write.
>  * NXM: NXM_NX_XXREG0(111) since v2.6.  <0>
>  * NXM: NXM_NX_XXREG1(112) since v2.6.  <1>
> - * NXM: NXM_NX_XXREG0(113) since v2.6.  <2>
> - * NXM: NXM_NX_XXREG1(114) since v2.6.  <3>
> + * NXM: NXM_NX_XXREG2(113) since v2.6.  <2>
> + * NXM: NXM_NX_XXREG3(114) since v2.6.  <3>
>  * OXM: none.
>  */
> MFF_XXREG0,
> -- 
> 2.1.4
> 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] meta-flow: Fix the NXM_NX_* names of xxreg2 and xxreg3.

2016-10-07 Thread Jarno Rajahalme

xxreg2 and xxreg3 had the same NXM_NX_* names as xxreg0 and xxreg1,
correspondingly.

Found by inspection.

CC: Justin Pettit 
Fixes: b23ada8eecfd ("Introduce 128-bit xxregs.")
Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/meta-flow.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 966ff7f..ef07561 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -940,8 +940,8 @@ enum OVS_PACKED_ENUM mf_field_id {
  * Access: read/write.
  * NXM: NXM_NX_XXREG0(111) since v2.6.  <0>
  * NXM: NXM_NX_XXREG1(112) since v2.6.  <1>
- * NXM: NXM_NX_XXREG0(113) since v2.6.  <2>
- * NXM: NXM_NX_XXREG1(114) since v2.6.  <3>
+ * NXM: NXM_NX_XXREG2(113) since v2.6.  <2>
+ * NXM: NXM_NX_XXREG3(114) since v2.6.  <3>
  * OXM: none.
  */
 MFF_XXREG0,
-- 
2.1.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [patch_v4] ovn: Add additional comments regarding arp responders.

2016-10-07 Thread Han Zhou

Overall it looks good to me. Just suggestions for rewording.

On Thu, Oct 6, 2016 at 10:34 AM, Darrell Ball  wrote:
>
> There has been enough confusion regarding logical switch datapath
> arp responders in ovn to warrant some additional comments;
> hence add a general description regarding why they exist and
> document the special cases.
>
> Signed-off-by: Darrell Ball 
> ---
>
> Note this patch is meant to be merge with the code change for vtep
> inport handling here.
> https://patchwork.ozlabs.org/patch/675796/
>
> v3->v4: Capitalization fixes.
> Reinstate comment regarding L2 learning confusion.
>
> v2->v3: Reword and further elaborate.
>
> v1->v2: Dropped RFC code change for logical switch router
> type ports
>
>  ovn/northd/ovn-northd.8.xml | 52 ++
+--
>  1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 77eb3d1..5ac351d 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -415,20 +415,60 @@
>  Ingress Table 9: ARP/ND responder
>
>  
> -  This table implements ARP/ND responder for known IPs.

To avoid confusing with lrouter ARP responder, it is better to be: this
table implements ARP/ND responder in *logical switch*, for known IPs.

> +  ARP requests received by multiple hypervisors, as in the case of
> +  localnet and vtep logical inports need
> +  to skip these logical switch ARP responders;  the reason being
> +  that northd downloads the same mac binding rules to all hypervisors
> +  and all hypervisors will receive the ARP request from the external
> +  network and respond.  This will confuse L2 learning on the source
> +  of the ARP requests.  These skip rules are mentioned under
> +  priority-100 flows.

Suggest to avoid this detail here, but just describe in the priority-100
flow section. Too much details here adds redundancy and not helpful to
overall understanding. For example, there is also another case to skip
which is when IP is owned by the inport, and it is described separately
already.

> +   ARP requests arrive from VMs with a
logical
> +  switch inport type of type empty, which is the default.  For this
> +  case,

This part of text is correct, but I feel it doesn't help understanding but
add confusion, so suggest to remove.

> +   If this logical switch router type port does not have an
> +  IP address configured, ARP requests will hit another ARP responder
> +  on the logical router datapath itself, which is most commonly a
> +  distributed logical router.  The advantage of using the logical
> +  switch proxy ARP rule for logical router ports is that this rule
> +  is hit before the logical switch L2 broadcast rule.  This means
> +  the ARP request is not broadcast on this logical switch.

How about:

If this logical switch router type port does not have an IP address
configured, although the request will still be responded by the ARP
responder on the logical router datapath, the ARP request will be broadcast
on the logical switch.

> + Note that ARP requests
received from
> +  localnet or vtep logical inports can
> +  either go directly to VMs, in which case the VM responds or can
> +  hit an ARP responder for a logical router port if the packet is
> +  used to resolve a logical router port next hop address.

This part of text is somehow redundant with the above. If this is to be
emphasised, we may put it under the section for router ARP responder flows
description, to explain why we still need ARP responder for on router
datapath.

> + The
inport being of type
> +router has no known use case for these ARP
> +responders.  However, no skip flows are installed for these
> +packets, as there would be some additional flow cost for this
> +and the value appears limited.

Maybe we don't even need to mention this if we don't want to skip this kind
of packet. If there is no such case, it means we don't need to add flows to
skip it; if there is such case, but we want the ARP responder to work for
that case, we don't need to a flows to skip either.

Acked-by: Han Zhou 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] vagrant: use RECHECK=yes for system checks

2016-10-07 Thread Daniele Di Proietto






On 04/10/2016 12:02, "Ben Pfaff"  wrote:

>On Mon, Sep 19, 2016 at 04:31:04PM -0300, Thadeu Lima de Souza Cascardo wrote:
>> Use RECHECK=yes for both kernel and userspace datapath tests.
>> 
>> Signed-off-by: Thadeu Lima de Souza Cascardo 
>
>Looks good to me but Daniele knows the system testsuite (much!) better
>than me so I'll leave this to him.

Looks good to me, applied to master, thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 07/12] dpif-netdev: Cache align netdev_flow_keys.

2016-10-07 Thread Daniele Di Proietto

2016-10-07 14:10 GMT-07:00 Jarno Rajahalme :

>
> > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <
> bhanuprakash.bodire...@intel.com> wrote:
> >
> > Aligning the 'keys' array seems to positively impact performance.
> >
> > Signed-off-by: Bhanuprakash Bodireddy 
> > Signed-off-by: Antonio Fischetti 
> > ---
> > lib/dpif-netdev.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index d0bb191..dfc9cbd 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4157,7 +4157,7 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
> > /* Sparse or MSVC doesn't like variable length array. */
> > enum { PKT_ARRAY_SIZE = NETDEV_MAX_BURST };
> > #endif
> > -struct netdev_flow_key keys[PKT_ARRAY_SIZE];
> > +struct netdev_flow_key keys[PKT_ARRAY_SIZE]
> __attribute__((aligned(64)));
>
> Due to compiler compatibility you must use OVS_ALIGNED_VAR(64) instead.
>

I would also use the CACHE_LINE_SIZE define, instead of 64

Thanks,

Daniele


> > struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
> > long long now = time_msec();
> > size_t newcnt, n_batches, i;
> > --
> > 2.4.11
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
> ___
> 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 05/12] dpif-netdev: Clear flow batches inside packet_batch_execute.

2016-10-07 Thread Daniele Di Proietto

This patch basically reverts 603f2ce04d00("dpif-netdev: Clear flow batches
before execute.")

As explained in that commit message the problem is that
packet_batch_per_flow_execute() can trigger recirculation.  This means that
we will call recursively dp_netdev_input__().  Here's a stack frame:

dp_netdev_input__()
{
struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
/*...*/
packet_batch_per_flow_execute()
{
dp_execute_cb()
{
case OVS_ACTION_ATTR_RECIRC:
dp_netdev_input__() {
struct packet_batch_per_flow
batches[PKT_ARRAY_SIZE];
/*...*/
}
}

}

In the inner dp_netdev_input__() a lookup could theoretically find the same
flows that it finds in the outer.
If we don't clear _all_ the batch pointers before calling the inner
dp_netdev_input__() we could find a flow that still has a pointer to a
batch in the outer dp_netdev_input__(). Then we will append packets to the
outer batch, which is clearly wrong.

2016-10-07 14:09 GMT-07:00 Jarno Rajahalme :

> Daniele had a comment on this, I believe?
>
> > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <
> bhanuprakash.bodire...@intel.com> wrote:
> >
> > There is a slight negative performance impact, by zeroing out the flow
> > batch pointers in dp_netdev_input__ ahead of packet_batch_execute(). The
> > issue has been observed with multiple batches test scenario.
> >
> > This patch fixes the problem by removing the extra for loop and clear
> > the flow batches inside the packet_batch_per_flow_execute(). Also the
> > vtune analysis showed that the overall no. of instructions retired for
> > dp_netdev_input__ reduced by 1,273,800,000 with this patch.
> >
> > Fixes: 603f2ce04d00 ("dpif-netdev: Clear flow batches before execute.")
> > Signed-off-by: Bhanuprakash Bodireddy 
> > Signed-off-by: Antonio Fischetti 
> > ---
> > lib/dpif-netdev.c | 5 +
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index c002dd3..d0bb191 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3878,6 +3878,7 @@ packet_batch_per_flow_execute(struct
> packet_batch_per_flow *batch,
> > {
> > struct dp_netdev_actions *actions;
> > struct dp_netdev_flow *flow = batch->flow;
> > +flow->batch = NULL;
> >
> > dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
> > batch->tcp_flags, now);
> > @@ -4173,10 +4174,6 @@ dp_netdev_input__(struct dp_netdev_pmd_thread
> *pmd,
> > }
> >
> > for (i = 0; i < n_batches; i++) {
> > -batches[i].flow->batch = NULL;
> > -}
> > -
> > -for (i = 0; i < n_batches; i++) {
> > packet_batch_per_flow_execute([i], pmd, now);
> > }
> > }
> > --
> > 2.4.11
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
> ___
> 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 00/12] Improve performance of OVS-DPDK classifier

2016-10-07 Thread Daniele Di Proietto

Thanks for posting this.

I quickly tried this with some simple flow tables and it seems to be
beneficial.

I agree with Jarno's comments and I posted a couple more to the single
patches.

I see two signoff, but a single author, should you add a Co-authored-by,
perhaps?

Other than these I am fine with the series. Could you maybe send another
version with the suggested changes, please?

Thanks,

Daniele

2016-10-07 9:17 GMT-07:00 Bhanuprakash Bodireddy <
bhanuprakash.bodire...@intel.com>:

> This patch series is aimed at improving the performance of OVS-DPDK
> dpcls.
>
> With few thousands flows installed, the EMC becomes inefficient due
> to thrashing and the bottleneck moves to the dpcls. In EMC disabled
> case, through VTune we found that significant performance degradation
> is due to LLC thrashing, memory latency, machine clears and expensive
> hash computation.
>
> This first patch-set improves the dpcls performance by 15% (~1 Mpps)
> when EMC is disabled and OVS-DPDK built with CFLAGS="-O2 -g".
>
> Bhanuprakash Bodireddy (12):
>   dpcls: Use 32 packet batches for lookups.
> Comment: ~120k performance throughput improvement.
>
>   flow: Add comments to mf_get_next_in_map()
> Comment: Add comments to function.
>
>   flow: Skip invoking expensive count_1bits() with zero input.
> Comment: ~630k performance throughput improvement.
>
>   hash: Skip invoking mhash_add__() with zero input.
> Comment: ~150k performance throughput improvement.
>
>   dpif-netdev: Clear flow batches inside packet_batch_execute.
> Comment: ~50k performance throughput improvement with multiple
> batches test case.
>
>   cmap: Remove prefetching in cmap_find_batch().
> Comment: ~39k performance throughput improvement.
>
>   dpif-netdev: Cache align netdev_flow_keys.
> Comment: ~170k performance throughput improvement in EMC enabled
> case.
>
>   dpif-netdev: Reorder elements in dp_netdev_port structure.
>   dpif: Reorder elements in dpif_upcall structure.
>   ovsdb: Reorder elements in ovsdb_table_schema structure.
>   netlink-socket: Reorder elements in nl_dump structure.
>   timeval: Reorder elements in clock structure.
> Comment: Reorder memeber variables of the structures to reduce pad
> bytes
>  and there by memory footprint.
>
>  lib/cmap.c   |   4 --
>  lib/dpif-netdev.c| 118 +-
> -
>  lib/dpif.h   |  17 
>  lib/flow.h   |  29 +++--
>  lib/hash.h   |   2 +-
>  lib/netlink-socket.h |   6 +--
>  lib/timeval.c|   4 +-
>  ovsdb/table.h|   4 +-
>  8 files changed, 91 insertions(+), 93 deletions(-)
>
> --
> 2.4.11
>
> ___
> 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 2/2] ovn-trace: Include source file and line number reference in output.

2016-10-07 Thread Ben Pfaff

On Fri, Oct 07, 2016 at 02:39:00PM -0700, Andy Zhou wrote:
> On Fri, Oct 7, 2016 at 10:02 AM, Ben Pfaff  wrote:
> 
> > This should make it that much easier to track down the code that emitted
> > a particular flow.
> >
> > Signed-off-by: Ben Pfaff 
> >
> 
>  Acked-by: Andy Zhou 
> 
> Also tested in the OVN sandbox.

Thanks, I applied both of these to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: Fix "trace" test to wait for synchronization of southbound ports.

2016-10-07 Thread Ben Pfaff

On Fri, Oct 07, 2016 at 02:41:22PM -0700, Andy Zhou wrote:
> On Fri, Oct 7, 2016 at 9:23 AM, Ben Pfaff  wrote:
> 
> > Signed-off-by: Ben Pfaff 
> 
>  Acked-by: Andy Zhou 

Thanks, I applied this to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] smap: New macro SMAP_CONST2 for an immutable map of 2 key-value pairs.

2016-10-07 Thread Ben Pfaff

On Fri, Oct 07, 2016 at 02:37:29PM -0700, Andy Zhou wrote:
> On Fri, Oct 7, 2016 at 10:02 AM, Ben Pfaff  wrote:
> 
> > Future commits will add a user.
> >
> > Signed-off-by: Ben Pfaff 
> >
> 
> Acked-by: Andy Zhou 
> 
> This is the first time I encountered a linked-list being built statically.
> Very cool trick!

Thanks for the review!

I was kind of surprised that it could work, but it did.

I don't think it's possible to build a doubly linked list in the same
way.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: Fix "trace" test to wait for synchronization of southbound ports.

2016-10-07 Thread Andy Zhou

On Fri, Oct 7, 2016 at 9:23 AM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 

 Acked-by: Andy Zhou 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ovn-trace: Include source file and line number reference in output.

2016-10-07 Thread Andy Zhou

On Fri, Oct 7, 2016 at 10:02 AM, Ben Pfaff  wrote:

> This should make it that much easier to track down the code that emitted
> a particular flow.
>
> Signed-off-by: Ben Pfaff 
>

 Acked-by: Andy Zhou 

Also tested in the OVN sandbox.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] smap: New macro SMAP_CONST2 for an immutable map of 2 key-value pairs.

2016-10-07 Thread Andy Zhou

On Fri, Oct 7, 2016 at 10:02 AM, Ben Pfaff  wrote:

> Future commits will add a user.
>
> Signed-off-by: Ben Pfaff 
>

Acked-by: Andy Zhou 

This is the first time I encountered a linked-list being built statically.
Very cool trick!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 12/12] timeval: Reorder elements in clock structure.

2016-10-07 Thread Jarno Rajahalme

I would leave the ‘stopped’ member below the comment.

Also, the 2nd cacheline is only ever accessed during unit tests, so this should 
not have real performance impact.

Acked-by: Jarno Rajahalme 

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> By reordering the elements in clock structure, pad bytes can be reduced
> and also a cache line is saved.
> 
> Before: structure size:136, holes:3, sum padbytes:18, cachelines:3
> After: structure size:120, holes:1, sum padbytes:2, cachelines:2
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/timeval.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/timeval.c b/lib/timeval.c
> index 0e8709a..ee755db 100644
> --- a/lib/timeval.c
> +++ b/lib/timeval.c
> @@ -69,12 +69,12 @@ struct large_warp {
> 
> struct clock {
> clockid_t id;   /* CLOCK_MONOTONIC or CLOCK_REALTIME. */
> +atomic_bool slow_path; /* True if warped or stopped. */
> +bool stopped OVS_GUARDED;  /* Disable real-time updates if true. 
> */
> 
> /* Features for use by unit tests.  Protected by 'mutex'. */
> struct ovs_mutex mutex;
> -atomic_bool slow_path; /* True if warped or stopped. */
> struct timespec warp OVS_GUARDED;  /* Offset added for unit tests. */
> -bool stopped OVS_GUARDED;  /* Disable real-time updates if true. 
> */
> struct timespec cache OVS_GUARDED; /* Last time read from kernel. */
> struct large_warp large_warp OVS_GUARDED; /* Connection information 
> waiting
>  for warp response. */
> -- 
> 2.4.11
> 
> ___
> 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 11/12] netlink-socket: Reorder elements in nl_dump structure.

2016-10-07 Thread Jarno Rajahalme

Acked-by: Jarno Rajahalme 

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> By reordering the elements in nl_dump structure, pad bytes can be
> reduced there by saving a cache line.
> 
> Before: structure size:72, holes:1, sum padbytes:4, cachelines:2
> After: structure size:64, holes:0, sum padbytes:0, cachelines:1
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/netlink-socket.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
> index f73fc7d..d3cc642 100644
> --- a/lib/netlink-socket.h
> +++ b/lib/netlink-socket.h
> @@ -260,12 +260,12 @@ struct nl_dump {
> /* These members are immutable during the lifetime of the nl_dump. */
> struct nl_sock *sock;   /* Socket being dumped. */
> uint32_t nl_seq;/* Expected nlmsg_seq for replies. */
> -
> -/* 'mutex' protects 'status' and serializes access to 'sock'. */
> -struct ovs_mutex mutex; /* Protects 'status', synchronizes recv(). */
> int status OVS_GUARDED; /* 0: dump in progress,
>  * positive errno: dump completed with error,
>  * EOF: dump completed successfully. */
> +
> +/* 'mutex' protects 'status' and serializes access to 'sock'. */
> +struct ovs_mutex mutex; /* Protects 'status', synchronizes recv(). */
> };
> 
> void nl_dump_start(struct nl_dump *, int protocol,
> -- 
> 2.4.11
> 
> ___
> 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 10/12] ovsdb: Reorder elements in ovsdb_table_schema structure.

2016-10-07 Thread Jarno Rajahalme

Acked-by: Jarno Rajahalme 

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> By reordering the elements in ovsdb_table_schema structure, pad bytes
> can be reduced and also a cache line is saved.
> 
> Before: structure size:72, holes:2, sum padbytes:10, cachelines:2
> After: structure size:64, holes:1, sum padbytes:2, cachelines:1
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> ovsdb/table.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ovsdb/table.h b/ovsdb/table.h
> index f910d18..69dd649 100644
> --- a/ovsdb/table.h
> +++ b/ovsdb/table.h
> @@ -28,9 +28,9 @@ struct uuid;
> struct ovsdb_table_schema {
> char *name;
> bool mutable;
> -struct shash columns;   /* Contains "struct ovsdb_column *"s. */
> -unsigned int max_rows;  /* Maximum number of rows. */
> bool is_root;   /* Part of garbage collection root set? */
> +unsigned int max_rows;  /* Maximum number of rows. */
> +struct shash columns;   /* Contains "struct ovsdb_column *"s. */
> struct ovsdb_column_set *indexes;
> size_t n_indexes;
> };
> -- 
> 2.4.11
> 
> ___
> 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 09/12] dpif: Reorder elements in dpif_upcall structure.

2016-10-07 Thread Jarno Rajahalme

CodingStyle.md instructs to group struct members into related groups. Also, 
changing the relative order of pointers should not make any difference. Could 
you achieve the same by reordering just the members above the ‘DPIF_UC_ACTION 
only.’ comment?

  Jarno

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> By reordering the data elements in dpif_upcall structure, pad bytes can
> be reduced and also a cache line.
> 
> Before: structure size:768, holes:1, sum padbytes:60, cachelines:12
> After: structure size:656, holes:1, sum padbytes:4, cachelines:11
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/dpif.h | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/dpif.h b/lib/dpif.h
> index a7c5097..4a4bb3d 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum 
> dpif_upcall_type);
> struct dpif_upcall {
> /* All types. */
> enum dpif_upcall_type type;
> -struct dp_packet packet;   /* Packet data. */
> -struct nlattr *key; /* Flow key. */
> -size_t key_len; /* Length of 'key' in bytes. */
> -ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
> -struct nlattr *mru; /* Maximum receive unit. */
> -struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
> 
> /* DPIF_UC_ACTION only. */
> -struct nlattr *userdata;/* Argument to OVS_ACTION_ATTR_USERSPACE. */
> -struct nlattr *out_tun_key;/* Output tunnel key. */
> struct nlattr *actions;/* Argument to OVS_ACTION_ATTR_USERSPACE. */
> +struct nlattr *out_tun_key;/* Output tunnel key. */
> +struct nlattr *userdata;/* Argument to OVS_ACTION_ATTR_USERSPACE. */
> +
> +struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
> +struct nlattr *mru; /* Maximum receive unit. */
> +ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
> +struct dp_packet packet;   /* Packet data. */
> +struct nlattr *key; /* Flow key. */
> +size_t key_len; /* Length of 'key' in bytes. */
> };
> 
> /* A callback to notify higher layer of dpif about to be purged, so that
> -- 
> 2.4.11
> 
> ___
> 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 08/12] dpif-netdev: Reorder elements in dp_netdev_port structure.

2016-10-07 Thread Jarno Rajahalme

Would equivalent packing be achieved by moving the line down before the bool 
instead? If yes, it would be preferable.

Acked-by: Jarno Rajahalme 

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> By reordering the data elements in dp_netdev_port structure, pad bytes
> can be reduced and there by saving a cache line.
> 
> Before: structure size:136, holes:3, sum padbytes:15, cachelines:3
> After: structure size:128, holes:1, sum padbytes:7, cachelines:2
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/dpif-netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index dfc9cbd..262f4de 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -284,10 +284,10 @@ struct dp_netdev_rxq {
> /* A port in a netdev-based datapath. */
> struct dp_netdev_port {
> odp_port_t port_no;
> +unsigned n_rxq; /* Number of elements in 'rxq' */
> struct netdev *netdev;
> struct hmap_node node;  /* Node in dp_netdev's 'ports'. */
> struct netdev_saved_flags *sf;
> -unsigned n_rxq; /* Number of elements in 'rxq' */
> struct dp_netdev_rxq *rxqs;
> bool dynamic_txqs;  /* If true XPS will be used. */
> unsigned *txq_used; /* Number of threads that uses each tx queue. 
> */
> -- 
> 2.4.11
> 
> ___
> 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 07/12] dpif-netdev: Cache align netdev_flow_keys.

2016-10-07 Thread Jarno Rajahalme


> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> Aligning the 'keys' array seems to positively impact performance.
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/dpif-netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d0bb191..dfc9cbd 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4157,7 +4157,7 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
> /* Sparse or MSVC doesn't like variable length array. */
> enum { PKT_ARRAY_SIZE = NETDEV_MAX_BURST };
> #endif
> -struct netdev_flow_key keys[PKT_ARRAY_SIZE];
> +struct netdev_flow_key keys[PKT_ARRAY_SIZE] __attribute__((aligned(64)));

Due to compiler compatibility you must use OVS_ALIGNED_VAR(64) instead.

> struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
> long long now = time_msec();
> size_t newcnt, n_batches, i;
> -- 
> 2.4.11
> 
> ___
> 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 06/12] cmap: Remove prefetching in cmap_find_batch().

2016-10-07 Thread Jarno Rajahalme


> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> prefetching the data in to the caches isn't improving the performance in
> cmap_find_batch(). Moreover its found that there is slight improvement
> in performance with out prefetching.
> 

I recall doing some performance testing for this earlier, but have no records 
of the system or other circumstances. Is it likely that this is at least 
somewhat system and test case dependent? Also the modified batch size may be a 
factor here. Anyway, I don’t currently have any proof to the contrary, so I 
have no problem removing the prefetching.

However, you should also update all the comments referring to prefetching.

  Jarno

> This patch removes prefetching from cmap_find_batch().
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/cmap.c | 4 
> 1 file changed, 4 deletions(-)
> 
> diff --git a/lib/cmap.c b/lib/cmap.c
> index 8c7312d..4c34bda 100644
> --- a/lib/cmap.c
> +++ b/lib/cmap.c
> @@ -397,7 +397,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long 
> map,
> ULLONG_FOR_EACH_1(i, map) {
> h1s[i] = rehash(impl, hashes[i]);
> b1s[i] = >buckets[h1s[i] & impl->mask];
> -OVS_PREFETCH(b1s[i]);
> }
> /* Lookups, Round 1. Only look up at the first bucket. */
> ULLONG_FOR_EACH_1(i, map) {
> @@ -413,13 +412,11 @@ cmap_find_batch(const struct cmap *cmap, unsigned long 
> map,
> if (!node) {
> /* Not found (yet); Prefetch the 2nd bucket. */
> b2s[i] = >buckets[other_hash(h1s[i]) & impl->mask];
> -OVS_PREFETCH(b2s[i]);
> c1s[i] = c1; /* We may need to check this after Round 2. */
> continue;
> }
> /* Found. */
> ULLONG_SET0(map, i); /* Ignore this on round 2. */
> -OVS_PREFETCH(node);
> nodes[i] = node;
> }
> /* Round 2. Look into the 2nd bucket, if needed. */
> @@ -453,7 +450,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long 
> map,
> continue;
> }
> found:
> -OVS_PREFETCH(node);
> nodes[i] = node;
> }
> return result;
> -- 
> 2.4.11
> 
> ___
> 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 05/12] dpif-netdev: Clear flow batches inside packet_batch_execute.

2016-10-07 Thread Jarno Rajahalme

Daniele had a comment on this, I believe?

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> There is a slight negative performance impact, by zeroing out the flow
> batch pointers in dp_netdev_input__ ahead of packet_batch_execute(). The
> issue has been observed with multiple batches test scenario.
> 
> This patch fixes the problem by removing the extra for loop and clear
> the flow batches inside the packet_batch_per_flow_execute(). Also the
> vtune analysis showed that the overall no. of instructions retired for
> dp_netdev_input__ reduced by 1,273,800,000 with this patch.
> 
> Fixes: 603f2ce04d00 ("dpif-netdev: Clear flow batches before execute.")
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/dpif-netdev.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c002dd3..d0bb191 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3878,6 +3878,7 @@ packet_batch_per_flow_execute(struct 
> packet_batch_per_flow *batch,
> {
> struct dp_netdev_actions *actions;
> struct dp_netdev_flow *flow = batch->flow;
> +flow->batch = NULL;
> 
> dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
> batch->tcp_flags, now);
> @@ -4173,10 +4174,6 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
> }
> 
> for (i = 0; i < n_batches; i++) {
> -batches[i].flow->batch = NULL;
> -}
> -
> -for (i = 0; i < n_batches; i++) {
> packet_batch_per_flow_execute([i], pmd, now);
> }
> }
> -- 
> 2.4.11
> 
> ___
> 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 04/12] hash: Skip invoking mhash_add__() with zero input.

2016-10-07 Thread Jarno Rajahalme


> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> mhash_add__() is expensive and should be only called with valid input.
> This patch will validate the input before invoking the mhash_add__ and
> there by saving some cpu cycles.
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/hash.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/hash.h b/lib/hash.h
> index 114a419..9bfebdb 100644
> --- a/lib/hash.h
> +++ b/lib/hash.h
> @@ -70,7 +70,7 @@ static inline uint32_t mhash_add__(uint32_t hash, uint32_t 
> data)
> 
> static inline uint32_t mhash_add(uint32_t hash, uint32_t data)
> {
> -hash = mhash_add__(hash, data);
> +hash = data ? mhash_add__(hash, data): hash;

IMO the zero check is best placed in the function mhash_add__(), where it is 
also more evident that zero-valued data would not change the hash anyway. Maybe 
a comment to that effect would be also nice?

> hash = hash_rot(hash, 13);
> return hash * 5 + 0xe6546b64;
> }
> -- 
> 2.4.11
> 
> ___
> 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 03/12] flow: Skip invoking expensive count_1bits() with zero input.

2016-10-07 Thread Jarno Rajahalme

With the nit below,

Acked-by: Jarno Rajahalme 

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> This patch checks if trash is non-zero and only then resets the flowmap
> bit and increment the pointer by set bits as found in trash.
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/flow.h | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/flow.h b/lib/flow.h
> index 4eb19ae..8cfd243 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -609,11 +609,13 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
>  * corresponding data chunks should be skipped accordingly. */
> map_t trash = *fmap & (rm1bit - 1);
> 
> -*fmap -= trash;
> +if (trash) {
> +*fmap -= trash;
> /* count_1bits() is fast for systems where speed matters (e.g.,
>  * DPDK), so we don't try avoid using it.
>  * Advance 'aux->values' to point to the value for 'rm1bit'. */

You should update the comment, as we now avoid calling count_1bits().

> -aux->values += count_1bits(trash);
> +aux->values += count_1bits(trash);
> +}
> 
> *value = *aux->values;
> } else {
> -- 
> 2.4.11
> 
> ___
> 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 02/12] flow: Add comments to mf_get_next_in_map()

2016-10-07 Thread Jarno Rajahalme

> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 

A brief commit message should be included here. E.g.:

This patch adds comments to mf)get_next_in_map() to make it more comprehensible.

> Signed-off-by: Bhanuprakash Bodireddy 
> Signed-off-by: Antonio Fischetti 
> ---
> lib/flow.h | 23 +--
> 1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/flow.h b/lib/flow.h
> index ea24e28..4eb19ae 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -570,15 +570,27 @@ struct mf_for_each_in_map_aux {
> const uint64_t *values;
> };
> 
> +/*
> + * Parse the bitmap mask of the subtable and output the next value
> + * from the search-key.
> + */

While it is helpful to refer to higher level concepts such as subtable and 
search-key, I’d prefer the comment to rather state what the function does in 
terms of the abstractions at hand and then provide an example of use referring 
to subtables and such. Like this for example:

/* Get the data from ‘aux->values’ corresponding to the next lowest 1-bit in
 * ‘aux->map’, given that ‘aux->values’ points to an array of 64-bit words
 * corresponding to the 1-bits in ‘aux->fmap’, starting from the rightmost 
1-bit.
 * 
 * Returns ’true’ if the traversal is incomplete, ‘false’ otherwise. ‘aux’ is 
prepared
 * for the next iteration after each call.
 *
 * This is used to traverse through, for example, the values in a miniflow
 * representation of a flow key selected by non-zero 64-bit words in a
 * corresponding subtable mask. */ 

> static inline bool
> mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
>uint64_t *value)
> {
> -map_t *map, *fmap;
> +map_t *map;/* map refers to the bitmap mask of the subtable. */
> +map_t *fmap;   /* fmap refers to the bitmap of the search-key. */

Again, given that the example use was referred in the main comment above, these 
comments should stick to the abstractions at hand. Better yet, these comments 
should instead be placed into the aux struct definition above, e.g.:

struct mf_for_each_in_map_aux {
size_t unit;  /* Current 64-bit unit of the flowmaps 
being processed. */
struct flowmap fmap;  /* Remaining 1-bits corresponding to the 64-bit 
words in ‘values’
struct flowmap map;   /* Remaining 1-bits corresponding to the 64-bit 
words of interest.
const uint64_t *values;   /* 64-bit words corresponding to the 1-bits in 
‘fmap’. */
};

> map_t rm1bit;
> 
> +/* The bitmap mask selects which value must be considered from the
> + * search-key. We process the corresponding 'unit' of size 64 bits.
> + * 'aux->unit' is an index to the current unit of 64 bits. */

Given the above comments this could be changed to:

/* Skip empty map units. */

> while (OVS_UNLIKELY(!*(map = >map.bits[aux->unit]))) {
> -/* Skip remaining data in the previous unit. */
> +/* No bits are enabled, so no search-key value will be output.
> + * In case some of the corresponding data chunks in the search-key
> + * bitmap are set, the data chunks must be skipped, as they are not
> + * considered by this mask. This is handled by incrementing 
> aux->values
> + * accordingly. */

This comment is incorrect, as the determination of the iteration termination is 
done later (the ‘false’ return case).

How about this:

/* Skip remaining data in the current unit before advancing to the next. */

> aux->values += count_1bits(aux->fmap.bits[aux->unit]);
> if (++aux->unit == FLOWMAP_UNITS) {
> return false;
> @@ -589,7 +601,12 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
> *map -= rm1bit;
> fmap = >fmap.bits[aux->unit];
> 
> +/* If the 8-byte value referred by 'rm1bit' is present in the
> + * search-key return 'value', otherwise return 0. */

How about this instead:

/* If the rightmost 1-bit found from the current unit in ‘aux->map’
 * (‘rm1bit’) is also present in ‘aux->fmap’, store the corresponding
 * value from ‘aux->values’ to ‘*value', otherwise store 0. */

> if (OVS_LIKELY(*fmap & rm1bit)) {
> +/* The value is in the search-key, if the search-key contains other
> + * values preceeding the 'rm1bit' bit, we consider it trash and the
> + * corresponding data chunks should be skipped accordingly. */

How about this instead:

/* Skip all 64-bit words in ‘values’ preceding the one corresponding to 
‘rm1bit’. */

> map_t trash = *fmap & (rm1bit - 1);
> 
> *fmap -= trash;
> @@ -600,6 +617,8 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
> 
> *value = *aux->values;
> } else {
> +/* The search-key does not contain a value that corresponds to
> + * rm1bit. */

Given the above, I believe this comment can be omitted.

> *value = 0;
> }
>

Re: [ovs-dev] [PATCH 01/12] dpcls: Use 32 packet batches for lookups.

2016-10-07 Thread Jarno Rajahalme


> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy 
>  wrote:
> 
> This patch increases the number of packets processed in a batch during a
> lookup from 16 to 32. Processing batches of 32 packets improves
> performance and also one of the internal loops can be avoided here.
> 

Can you provide some qualification of the performance test conditions? Do you 
believe the performance difference applies universally?

> Signed-off-by: Antonio Fischetti 
> Signed-off-by: Bhanuprakash Bodireddy 
> ---
> lib/dpif-netdev.c | 109 +++---
> 1 file changed, 46 insertions(+), 63 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 6e09e44..c002dd3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4975,23 +4975,20 @@ dpcls_lookup(struct dpcls *cls, const struct 
> netdev_flow_key keys[],
>  int *num_lookups_p)
> {
> /* The received 'cnt' miniflows are the search-keys that will be processed
> - * in batches of 16 elements.  N_MAPS will contain the number of these
> - * 16-elements batches.  i.e. for 'cnt' = 32, N_MAPS will be 2.  The 
> batch
> - * size 16 was experimentally found faster than 8 or 32. */
> -typedef uint16_t map_type;
> + * to find a matching entry into the available subtables.
> + * The number of bits in map_type is equal to NETDEV_MAX_BURST. */

This needs a build time assertion to catch any future change in 
NETDEV_MAX_BURST.

Preferably, if you can verify that the compiler is able to remove the 
unnecessary loop in this case, you should consider not removing the extra loop 
that would kick in only if NETDEV_MAX_BURST becomes larger than 32.

> +typedef uint32_t map_type;
> #define MAP_BITS (sizeof(map_type) * CHAR_BIT)
> 
> -#if !defined(__CHECKER__) && !defined(_WIN32)
> -const int N_MAPS = DIV_ROUND_UP(cnt, MAP_BITS);
> -#else
> -enum { N_MAPS = DIV_ROUND_UP(NETDEV_MAX_BURST, MAP_BITS) };
> -#endif
> -map_type maps[N_MAPS];
> struct dpcls_subtable *subtable;
> 
> -memset(maps, 0xff, sizeof maps);
> -if (cnt % MAP_BITS) {
> -maps[N_MAPS - 1] >>= MAP_BITS - cnt % MAP_BITS; /* Clear extra bits. 
> */
> +map_type keys_map = 0x;
> +map_type found_map;
> +uint32_t hashes[MAP_BITS];
> +const struct cmap_node *nodes[MAP_BITS];
> +
> +if (OVS_UNLIKELY(cnt != NETDEV_MAX_BURST)) {
> +keys_map >>= NETDEV_MAX_BURST - cnt; /* Clear extra bits. */
> }
> memset(rules, 0, cnt * sizeof *rules);
> 
> @@ -5007,59 +5004,45 @@ dpcls_lookup(struct dpcls *cls, const struct 
> netdev_flow_key keys[],
> PVECTOR_FOR_EACH (subtable, >subtables) {
> const struct netdev_flow_key *mkeys = keys;
> struct dpcls_rule **mrules = rules;
> -map_type remains = 0;
> -int m;
> -
> -BUILD_ASSERT_DECL(sizeof remains == sizeof *maps);
> -
> -/* Loops on each batch of 16 search-keys. */
> -for (m = 0; m < N_MAPS; m++, mkeys += MAP_BITS, mrules += MAP_BITS) {
> -uint32_t hashes[MAP_BITS];
> -const struct cmap_node *nodes[MAP_BITS];
> -unsigned long map = maps[m];
> -int i;
> -
> -if (!map) {
> -continue; /* Skip empty maps. */
> -}
> -
> -/* Compute hashes for the remaining keys.  Each search-key is
> - * masked with the subtable's mask to avoid hashing the 
> wildcarded
> - * bits. */
> -ULLONG_FOR_EACH_1(i, map) {
> -hashes[i] = netdev_flow_key_hash_in_mask([i],
> - >mask);
> -}
> -/* Lookup. */
> -map = cmap_find_batch(>rules, map, hashes, nodes);
> -/* Check results.  When the i-th bit of map is set, it means 
> that a
> - * set of nodes with a matching hash value was found for the i-th
> - * search-key.  Due to possible hash collisions we need to check
> - * which of the found rules, if any, really matches our masked
> - * search-key. */
> -ULLONG_FOR_EACH_1(i, map) {
> -struct dpcls_rule *rule;
> -
> -CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
> -if (OVS_LIKELY(dpcls_rule_matches_key(rule, [i]))) 
> {
> -mrules[i] = rule;
> -/* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
> - * within one second optimization interval  */
> -subtable->hit_cnt++;
> -lookups_match += subtable_pos;
> -goto next;
> -}
> +int i;
> +found_map = keys_map;
> +
> +/* Compute hashes for the remaining keys.  Each search-key is
> + * masked with

[ovs-dev] [PATCH 3/6] checkpatch: print the final warning / error count

2016-10-07 Thread Aaron Conole

Because we track it, might as well report it.

Signed-off-by: Aaron Conole 
---
 utilities/checkpatch.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index ed53f32..61678a9 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -247,6 +247,7 @@ def usage():
 
 
 def ovs_checkpatch_file(filename):
+global __warnings, __errors
 try:
 mail = email.message_from_file(open(filename, 'r'))
 except:
@@ -256,7 +257,10 @@ def ovs_checkpatch_file(filename):
 for part in mail.walk():
 if part.get_content_maintype() == 'multipart':
 continue
-return ovs_checkpatch_parse(part.get_payload(decode=True))
+result = ovs_checkpatch_parse(part.get_payload(decode=True))
+if result < 0:
+print("Warnings: %d, Errors: %d" % (__warnings, __errors))
+return result
 
 if __name__ == '__main__':
 try:
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 6/6] checkpatch: Print file line numbers

2016-10-07 Thread Aaron Conole

The line numbers being printed were the line numbers for the patchfile.
This is sometimes okay to fix simple things (trailing or leading
whitespace, missing signoffs, etc).  But more complicated fixes, or
those fixes which require a bit more care, aren't helped by this.  So,
we use the implied file line number.

This can be useful with future work to 'mock' apply and build a real
contextual scanner for checking multi-line changes.

Signed-off-by: Aaron Conole 
---
 utilities/checkpatch.py | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index c4361f0..26ecc9e 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -52,6 +52,7 @@ def print_warning(message, lineno=None):
 
 
 __regex_added_line = re.compile(r'^\+{1,2}[^\+][\w\W]*')
+__regex_subtracted_line = re.compile(r'^\-{1,2}[^\-][\w\W]*')
 __regex_leading_with_whitespace_at_all = re.compile(r'^\s+')
 __regex_leading_with_spaces = re.compile(r'^ +[\S]+')
 __regex_trailing_whitespace = re.compile(r'[^\S]+$')
@@ -77,6 +78,10 @@ line_length_blacklist = ['.am', '.at', 'etc', '.in', '.m4', 
'.mk', '.patch',
  '.py']
 
 
+def is_subtracted_line(line):
+"""Returns TRUE if the line in question has been removed."""
+return __regex_subtracted_line.search(line) is not None
+
 def is_added_line(line):
 """Returns TRUE if the line in question is an added line.
 """
@@ -148,6 +153,8 @@ def ovs_checkpatch_parse(text):
 previous_file = ''
 scissors = re.compile(r'^[\w]*---[\w]*')
 hunks = re.compile('^(---|\+\+\+) (\S+)')
+hunk_differences = re.compile(
+r'^@@ ([1-9-+]+),([1-9-+]+) ([1-9-+]+),([1-9-+]+) @@')
 is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
@@ -196,6 +203,14 @@ def ovs_checkpatch_parse(text):
 current_file = newfile.group(2)
 print_file_name = current_file
 continue
+reset_line_number = hunk_differences.match(line)
+if reset_line_number:
+lineno = int(reset_line_number.group(3))
+if lineno < 0:
+lineno = -1 * lineno
+lineno -= 1
+if is_subtracted_line(line):
+lineno -= 1
 if not is_added_line(line):
 continue
 # Skip files which have /datapath in them, since they are
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 5/6] checkpatch: convert some of the warnings

2016-10-07 Thread Aaron Conole

These coding issues are not just things that shouldn't be done.  They are
styles which should never be submitted.

Signed-off-by: Aaron Conole 
---
 utilities/checkpatch.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index c8f21b0..c4361f0 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -216,12 +216,11 @@ def ovs_checkpatch_parse(text):
   lineno)
 if not if_and_for_whitespace_checks(line[1:]):
 print_line = True
-print_warning("Improper whitespace around control block",
+print_error("Improper whitespace around control block",
   lineno)
 if not if_and_for_end_with_bracket_check(line[1:]):
 print_line = True
-print_warning("Inappropriate bracing around statement",
-  lineno)
+print_error("Inappropriate bracing around statement", lineno)
 if print_line:
 print("\n%s\n" % line)
 if __errors or __warnings:
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 4/6] checkpatch: Print the line in question distinctly

2016-10-07 Thread Aaron Conole

This makes it easier to pick out of the warnings/errors.

Signed-off-by: Aaron Conole 
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 61678a9..c8f21b0 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -223,7 +223,7 @@ def ovs_checkpatch_parse(text):
 print_warning("Inappropriate bracing around statement",
   lineno)
 if print_line:
-print(line)
+print("\n%s\n" % line)
 if __errors or __warnings:
 return -1
 return 0
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/6] checkpatch: Announce the file where errors occur.

2016-10-07 Thread Aaron Conole

This makes finding the warning and error marks much easier.

Signed-off-by: Aaron Conole 
---
 utilities/checkpatch.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 17e5be4..ed53f32 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -21,10 +21,17 @@ import sys
 
 __errors = 0
 __warnings = 0
+print_file_name = None
 
+def print_file():
+global print_file_name
+if print_file_name:
+print("In file %s" % print_file_name)
+print_file_name = None
 
 def print_error(message, lineno=None):
 global __errors
+print_file()
 if lineno is not None:
 print("E(%d): %s" % (lineno, message))
 else:
@@ -35,6 +42,7 @@ def print_error(message, lineno=None):
 
 def print_warning(message, lineno=None):
 global __warnings
+print_file()
 if lineno:
 print("W(%d): %s" % (lineno, message))
 else:
@@ -131,6 +139,7 @@ def if_and_for_end_with_bracket_check(line):
 
 
 def ovs_checkpatch_parse(text):
+global print_file_name
 lineno = 0
 signatures = []
 co_authors = []
@@ -185,6 +194,7 @@ def ovs_checkpatch_parse(text):
 newfile = hunks.match(line)
 if newfile:
 current_file = newfile.group(2)
+print_file_name = current_file
 continue
 if not is_added_line(line):
 continue
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/6] checkpatch: Fix up the co-authors check

2016-10-07 Thread Aaron Conole

The signed-off and co-authors check that was committed is just plain
wrong.  The test should be more than 1 'Signed-off-by'.

Signed-off-by: Aaron Conole 
---
 utilities/checkpatch.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 754059a..17e5be4 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -167,13 +167,13 @@ def ovs_checkpatch_parse(text):
 if scissors.match(line):
 parse = parse + 1
 if not skip_signoff_check:
+if len(signatures) + len(co_authors) == 0:
+print_error("No authors?")
 if len(signatures) == 0:
 print_error("No signatures found.")
-if len(signatures) != 1 + len(co_authors):
+if len(signatures) > 1:
 print_error("Too many signoffs; "
 "are you missing Co-authored-by lines?")
-if not set(co_authors) <= set(signatures):
-print_error("Co-authored-by/Signed-off-by corruption")
 elif is_signature.match(line):
 m = is_signature.match(line)
 signatures.append(m.group(3))
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 0/6] Misc. Checkpatch cleanups

2016-10-07 Thread Aaron Conole

Two of these are 'bug fixes' to the checkpatch infrastructure (1/6, and 5/6),
while the rest are enhancements.  In particular, 6/6 hasn't been aggressively
tested.

These were developed while working on other patches, but are being submitted
here as an independent series.

Aaron Conole (6):
  checkpatch: Fix up the co-authors check
  checkpatch: Announce the file where errors occur.
  checkpatch: print the final warning / error count
  checkpatch: Print the line in question distinctly
  checkpatch: convert some of the warnings
  checkpatch: Print file line numbers

 utilities/checkpatch.py | 44 
 1 file changed, 36 insertions(+), 8 deletions(-)

-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v10] ovn-nbctl: Add LB commands.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 04:25, nickcooper-zhangtonghao <
nickcooper-zhangtong...@opencloud.tech> wrote:

> Thanks very much.
>

If it is of interest,  something similar can be done for NAT too.


>
> > On Oct 4, 2016, at 5:12 AM, Guru Shetty  wrote:
> >
> > I applied the patch. For future, when you version, please have some sort
> of versioning information that tells the difference between the versions
> below the "---" line.
>
> ___
> 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] [Backport Request] For branch-2.5, backport rhel-systemd integration

2016-10-07 Thread Flavio Leitner

On Thu, Sep 08, 2016 at 07:58:21PM +0100, Markos Chandras wrote:
> On 09/08/2016 05:50 PM, Aaron Conole wrote:
> > 
> >> It sounds like new feature territory, but you do make a case for it being 
> >> considered a set of fixes ...
> > 
> > I agree - it straddles a line.  I was hesitant to even ask, but in RHEL
> > we probably need to backport these anyway, so I made an assumption
> > (maybe poor) that other systemd distros might use the rhel scripts and
> > run into this class of issues also.  Good net-izen, and all that :)
> > 
> > -Aaron
> 
> Hi Aaron,
> 
> Thanks for the backport request. I am also interested in these fixes and
> it's something I could use in the SUSE package as well. For my point of
> view I see no blockers for those updating their 2.5.0 installations with
> these fixes in but I will do some testing on my end as well.

Most of the changes are transparent and should not cause any issues.

I say most of the changes because the problem is if someone is relying
on the openvswitch-nonetwork service state.  In that case, the service
doesn't exist anymore after the patchset, so it might break something.

I am not sure if we care about that because that service should be
considered internal to OVS (used only by ifcfg scripts).  The front end
is the openvswitch.service which remains available.

Regarding to be a new feature or bugfix, I think this is a bugfix for
two things at least.

1) The real service state is represented by openvswitch service.
Before the patchset there were situations where the OVS threads were
dead but the systemd service was up & running.  The patchset intends
to fix this issue.

2) The shutdown ordering. Before the patchset we could have OVS shutting
down before other networking services that depends on network.  When OVS
terminates, it breaks networking connectivity causing issues. The patchset
intends to fix the issue as well, though it needs to include the follow up
fix  http://openvswitch.org/pipermail/dev/2016-October/080426.html

In another words,
Acked-by: Flavio Leitner 

-- 
fbl

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] rhel-systemd: Delay shutting down the services

2016-10-07 Thread Aaron Conole

During testing it was found that systemd would consider the openvswitch
service as a part of networking component, but the dependent services of
ovs-vswitchd and ovsdb-server were not likewise considered.  This leads
to some strange race conditions, observed when using NFS over TCP, while
shutting down systems.

Fixes: 84ad12083491 ("rhel: Improved Systemd Integration")
Co-authored-by: Flavio Leitner 
Signed-off-by: Aaron Conole 
---
 rhel/usr_lib_systemd_system_openvswitch.service  | 2 ++
 rhel/usr_lib_systemd_system_ovs-vswitchd.service | 3 ++-
 rhel/usr_lib_systemd_system_ovsdb-server.service | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/rhel/usr_lib_systemd_system_openvswitch.service
b/rhel/usr_lib_systemd_system_openvswitch.service
index a44574b..e823c54 100644
--- a/rhel/usr_lib_systemd_system_openvswitch.service
+++ b/rhel/usr_lib_systemd_system_openvswitch.service
@@ -1,5 +1,7 @@
 [Unit]
 Description=Open vSwitch
+Before=network.target network.service
+After=network-pre.target
 PartOf=network.target
 BindsTo=ovsdb-server.service
 BindsTo=ovs-vswitchd.service
diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service
b/rhel/usr_lib_systemd_system_ovs-vswitchd.service
index d3d020a..01d9cee 100644
--- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service
+++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service
@@ -1,6 +1,7 @@
 [Unit]
 Description=Open vSwitch Forwarding Unit
-After=ovsdb-server.service
+After=ovsdb-server.service network-pre.target
+Before=network.target network.service
 Requires=ovsdb-server.service
 ReloadPropagatedFrom=ovsdb-server.service
 AssertPathIsReadWrite=/var/run/openvswitch/db.sock
diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
b/rhel/usr_lib_systemd_system_ovsdb-server.service
index 46e3e08..d3d7408 100644
--- a/rhel/usr_lib_systemd_system_ovsdb-server.service
+++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
@@ -1,6 +1,7 @@
 [Unit]
 Description=Open vSwitch Database Unit
-After=syslog.target
+After=syslog.target network-pre.target
+Before=network.target network.service
 ReloadPropagatedFrom=openvswitch.service
 PartOf=openvswitch.service
 
-- 
2.7.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] lex: Treat formfeeds as white space.

2016-10-07 Thread Ben Pfaff

Also vertical tabs, whatever those are.

Signed-off-by: Ben Pfaff 
---
 ovn/lib/lex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
index a05edfa..7eee3b6 100644
--- a/ovn/lib/lex.c
+++ b/ovn/lib/lex.c
@@ -589,7 +589,7 @@ next:
 token->type = LEX_T_END;
 return p;
 
-case ' ': case '\t': case '\n': case '\r':
+case ' ': case '\t': case '\n': case '\r': case '\v': case '\f':
 p++;
 goto next;
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/2] smap: New macro SMAP_CONST2 for an immutable map of 2 key-value pairs.

2016-10-07 Thread Ben Pfaff

Future commits will add a user.

Signed-off-by: Ben Pfaff 
---
 include/openvswitch/hmap.h | 10 +-
 lib/smap.h | 26 +-
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index ef272a6..8aea9c2 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -68,11 +68,11 @@ struct hmap {
 #define HMAP_INITIALIZER(HMAP) \
 { (struct hmap_node **const) &(HMAP)->one, NULL, 0, 0 }
 
-/* Initializer for an immutable struct hmap 'HMAP' that contains a single
- * 'NODE'. */
-#define HMAP_CONST1(HMAP, NODE) {   \
-CONST_CAST(struct hmap_node **, &(HMAP)->one), NODE, 0, 1 }
-#define HMAP_NODE_INIT(HASH) { HASH, NULL }
+/* Initializer for an immutable struct hmap 'HMAP' that contains 'N' nodes
+ * linked together starting at 'NODE'.  The hmap only has a single chain of
+ * hmap_nodes, so 'N' should be small. */
+#define HMAP_CONST(HMAP, N, NODE) { \
+CONST_CAST(struct hmap_node **, &(HMAP)->one), NODE, 0, N }
 
 /* Initialization. */
 void hmap_init(struct hmap *);
diff --git a/lib/smap.h b/lib/smap.h
index 49d31cb..37eac47 100644
--- a/lib/smap.h
+++ b/lib/smap.h
@@ -54,16 +54,24 @@ struct smap_node {
  *
  * An smap initialized this way must not be modified or destroyed.
  *
- * The 'KEY' argument is evaluated multiple times.
+ * The 'KEY', 'K1', 'K2' arguments are evaluated multiple times.
  */
-#define SMAP_CONST1(SMAP, KEY, VALUE) { \
-HMAP_CONST1(&(SMAP)->map,   \
-   (&(struct smap_node) SMAP_NODE_INIT(KEY, VALUE).node)) \
-}
-#define SMAP_NODE_INIT(KEY, VALUE) {\
-HMAP_NODE_INIT(hash_string(KEY, 0)),\
-   CONST_CAST(char *, KEY), \
-   CONST_CAST(char *, VALUE) }
+#define SMAP_CONST1(SMAP, KEY, VALUE) (const struct smap) { \
+HMAP_CONST(&(SMAP)->map, 1, SMAP_NODE(KEY, VALUE, NULL)) \
+}
+#define SMAP_CONST2(SMAP, K1, V1, K2, V2) (const struct smap) { \
+HMAP_CONST(&(SMAP)->map, 2, \
+   SMAP_NODE(K1, V1, SMAP_NODE(K2, V2, NULL)))  \
+}
+#define SMAP_NODE(KEY, VALUE, NEXT) \
+&(struct smap_node) {   \
+.node = {   \
+.hash = hash_string(KEY, 0),\
+.next = (NEXT), \
+},  \
+.key = CONST_CAST(char *, KEY), \
+.value = CONST_CAST(char *, VALUE), \
+}.node
 
 
 void smap_init(struct smap *);
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] ovn-trace: Include source file and line number reference in output.

2016-10-07 Thread Ben Pfaff

This should make it that much easier to track down the code that emitted
a particular flow.

Signed-off-by: Ben Pfaff 
---
 ovn/northd/ovn-northd.c   | 40 +++-
 ovn/ovn-sb.xml|  5 +
 ovn/utilities/ovn-trace.8.xml |  7 ---
 ovn/utilities/ovn-trace.c |  9 -
 4 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 2e79156..281dc62 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1635,6 +1635,7 @@ struct ovn_lflow {
 uint16_t priority;
 char *match;
 char *actions;
+const char *where;
 };
 
 static size_t
@@ -1658,30 +1659,36 @@ ovn_lflow_equal(const struct ovn_lflow *a, const struct 
ovn_lflow *b)
 
 static void
 ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
-  enum ovn_stage stage, uint16_t priority,
-  char *match, char *actions)
+   enum ovn_stage stage, uint16_t priority,
+   char *match, char *actions, const char *where)
 {
 lflow->od = od;
 lflow->stage = stage;
 lflow->priority = priority;
 lflow->match = match;
 lflow->actions = actions;
+lflow->where = where;
 }
 
 /* Adds a row with the specified contents to the Logical_Flow table. */
 static void
-ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
-  enum ovn_stage stage, uint16_t priority,
-  const char *match, const char *actions)
+ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
+ enum ovn_stage stage, uint16_t priority,
+ const char *match, const char *actions, const char *where)
 {
 ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od));
 
 struct ovn_lflow *lflow = xmalloc(sizeof *lflow);
 ovn_lflow_init(lflow, od, stage, priority,
-   xstrdup(match), xstrdup(actions));
+   xstrdup(match), xstrdup(actions), where);
 hmap_insert(lflow_map, >hmap_node, ovn_lflow_hash(lflow));
 }
 
+/* Adds a row with the specified contents to the Logical_Flow table. */
+#define ovn_lflow_add(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS) \
+ovn_lflow_add_at(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS,  \
+ OVS_SOURCE_LOCATOR)
+
 static struct ovn_lflow *
 ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od,
enum ovn_stage stage, uint16_t priority,
@@ -1689,7 +1696,8 @@ ovn_lflow_find(struct hmap *lflows, struct ovn_datapath 
*od,
 {
 struct ovn_lflow target;
 ovn_lflow_init(, od, stage, priority,
-   CONST_CAST(char *, match), CONST_CAST(char *, actions));
+   CONST_CAST(char *, match), CONST_CAST(char *, actions),
+   NULL);
 
 struct ovn_lflow *lflow;
 HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, ovn_lflow_hash(),
@@ -4337,8 +4345,22 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
 sbrec_logical_flow_set_match(sbflow, lflow->match);
 sbrec_logical_flow_set_actions(sbflow, lflow->actions);
 
-const struct smap ids = SMAP_CONST1(, "stage-name",
-ovn_stage_to_str(lflow->stage));
+/* Trim the source locator lflow->where, which looks something like
+ * "ovn/northd/ovn-northd.c:1234", down to just the part following the
+ * last slash, e.g. "ovn-northd.c:1234". */
+const char *slash = strrchr(lflow->where, '/');
+#if _WIN32
+const char *backslash = strrchr(lflow->where, '\\');
+if (!slash || backslash > slash) {
+slash = backslash;
+}
+#endif
+const char *where = slash ? slash + 1 : lflow->where;
+
+const struct smap ids = SMAP_CONST2(
+,
+"stage-name", ovn_stage_to_str(lflow->stage),
+"source", where);
 sbrec_logical_flow_set_external_ids(sbflow, );
 
 ovn_lflow_destroy(, lflow);
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 61b0f52..b484f59 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1455,6 +1455,11 @@ tcp.flags = RST;
   Human-readable name for this flow's stage in the pipeline.
 
 
+
+  Source file and line number of the code that added this flow to the
+  pipeline.
+
+
 
   The overall purpose of these columns is described under Common
   Columns at the beginning of this document.
diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml
index 747e130..92dcb17 100644
--- a/ovn/utilities/ovn-trace.8.xml
+++ b/ovn/utilities/ovn-trace.8.xml
@@ -114,9 +114,10 @@
   
 The detailed form of output is also the default form.  This form groups
 output into sections headed up by the ingress or egress pipeline being
-traversed.  Each pipeline lists each table that was visited (by number
-and name), the match expression and

[ovs-dev] [PATCH] ovn: Fix "trace" test to wait for synchronization of southbound ports.

2016-10-07 Thread Ben Pfaff

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/ovn.at b/tests/ovn.at
index 4dfb077..38b5af9 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1259,6 +1259,9 @@ ovn-nbctl ls-add lsw0
 ovn-sbctl chassis-add hv0 geneve 127.0.0.1
 for i in 1 2 3; do
 ovn-nbctl lsp-add lsw0 lp$i
+done
+ovn-nbctl --wait=sb sync
+for i in 1 2 3; do
 ovn-sbctl lsp-bind lp$i hv0
 if test $i = 1; then
 ovn-nbctl lsp-set-addresses lp$i "f0:00:00:00:00:0$i 192.168.0.$i" 
unknown
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall structure.

2016-10-07 Thread Bhanuprakash Bodireddy

By reordering the data elements in dpif_upcall structure, pad bytes can
be reduced and also a cache line.

Before: structure size:768, holes:1, sum padbytes:60, cachelines:12
After: structure size:656, holes:1, sum padbytes:4, cachelines:11

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/dpif.h | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/lib/dpif.h b/lib/dpif.h
index a7c5097..4a4bb3d 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum 
dpif_upcall_type);
 struct dpif_upcall {
 /* All types. */
 enum dpif_upcall_type type;
-struct dp_packet packet;   /* Packet data. */
-struct nlattr *key; /* Flow key. */
-size_t key_len; /* Length of 'key' in bytes. */
-ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
-struct nlattr *mru; /* Maximum receive unit. */
-struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
 
 /* DPIF_UC_ACTION only. */
-struct nlattr *userdata;/* Argument to OVS_ACTION_ATTR_USERSPACE. */
-struct nlattr *out_tun_key;/* Output tunnel key. */
 struct nlattr *actions;/* Argument to OVS_ACTION_ATTR_USERSPACE. */
+struct nlattr *out_tun_key;/* Output tunnel key. */
+struct nlattr *userdata;/* Argument to OVS_ACTION_ATTR_USERSPACE. */
+
+struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
+struct nlattr *mru; /* Maximum receive unit. */
+ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
+struct dp_packet packet;   /* Packet data. */
+struct nlattr *key; /* Flow key. */
+size_t key_len; /* Length of 'key' in bytes. */
 };
 
 /* A callback to notify higher layer of dpif about to be purged, so that
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 07/12] dpif-netdev: Cache align netdev_flow_keys.

2016-10-07 Thread Bhanuprakash Bodireddy

Aligning the 'keys' array seems to positively impact performance.

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/dpif-netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d0bb191..dfc9cbd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4157,7 +4157,7 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
 /* Sparse or MSVC doesn't like variable length array. */
 enum { PKT_ARRAY_SIZE = NETDEV_MAX_BURST };
 #endif
-struct netdev_flow_key keys[PKT_ARRAY_SIZE];
+struct netdev_flow_key keys[PKT_ARRAY_SIZE] __attribute__((aligned(64)));
 struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
 long long now = time_msec();
 size_t newcnt, n_batches, i;
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 12/12] timeval: Reorder elements in clock structure.

2016-10-07 Thread Bhanuprakash Bodireddy

By reordering the elements in clock structure, pad bytes can be reduced
and also a cache line is saved.

Before: structure size:136, holes:3, sum padbytes:18, cachelines:3
After: structure size:120, holes:1, sum padbytes:2, cachelines:2

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/timeval.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/timeval.c b/lib/timeval.c
index 0e8709a..ee755db 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -69,12 +69,12 @@ struct large_warp {
 
 struct clock {
 clockid_t id;   /* CLOCK_MONOTONIC or CLOCK_REALTIME. */
+atomic_bool slow_path; /* True if warped or stopped. */
+bool stopped OVS_GUARDED;  /* Disable real-time updates if true. */
 
 /* Features for use by unit tests.  Protected by 'mutex'. */
 struct ovs_mutex mutex;
-atomic_bool slow_path; /* True if warped or stopped. */
 struct timespec warp OVS_GUARDED;  /* Offset added for unit tests. */
-bool stopped OVS_GUARDED;  /* Disable real-time updates if true. */
 struct timespec cache OVS_GUARDED; /* Last time read from kernel. */
 struct large_warp large_warp OVS_GUARDED; /* Connection information waiting
  for warp response. */
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 11/12] netlink-socket: Reorder elements in nl_dump structure.

2016-10-07 Thread Bhanuprakash Bodireddy

By reordering the elements in nl_dump structure, pad bytes can be
reduced there by saving a cache line.

Before: structure size:72, holes:1, sum padbytes:4, cachelines:2
After: structure size:64, holes:0, sum padbytes:0, cachelines:1

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/netlink-socket.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
index f73fc7d..d3cc642 100644
--- a/lib/netlink-socket.h
+++ b/lib/netlink-socket.h
@@ -260,12 +260,12 @@ struct nl_dump {
 /* These members are immutable during the lifetime of the nl_dump. */
 struct nl_sock *sock;   /* Socket being dumped. */
 uint32_t nl_seq;/* Expected nlmsg_seq for replies. */
-
-/* 'mutex' protects 'status' and serializes access to 'sock'. */
-struct ovs_mutex mutex; /* Protects 'status', synchronizes recv(). */
 int status OVS_GUARDED; /* 0: dump in progress,
  * positive errno: dump completed with error,
  * EOF: dump completed successfully. */
+
+/* 'mutex' protects 'status' and serializes access to 'sock'. */
+struct ovs_mutex mutex; /* Protects 'status', synchronizes recv(). */
 };
 
 void nl_dump_start(struct nl_dump *, int protocol,
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 10/12] ovsdb: Reorder elements in ovsdb_table_schema structure.

2016-10-07 Thread Bhanuprakash Bodireddy

By reordering the elements in ovsdb_table_schema structure, pad bytes
can be reduced and also a cache line is saved.

Before: structure size:72, holes:2, sum padbytes:10, cachelines:2
After: structure size:64, holes:1, sum padbytes:2, cachelines:1

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 ovsdb/table.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ovsdb/table.h b/ovsdb/table.h
index f910d18..69dd649 100644
--- a/ovsdb/table.h
+++ b/ovsdb/table.h
@@ -28,9 +28,9 @@ struct uuid;
 struct ovsdb_table_schema {
 char *name;
 bool mutable;
-struct shash columns;   /* Contains "struct ovsdb_column *"s. */
-unsigned int max_rows;  /* Maximum number of rows. */
 bool is_root;   /* Part of garbage collection root set? */
+unsigned int max_rows;  /* Maximum number of rows. */
+struct shash columns;   /* Contains "struct ovsdb_column *"s. */
 struct ovsdb_column_set *indexes;
 size_t n_indexes;
 };
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 06/12] cmap: Remove prefetching in cmap_find_batch().

2016-10-07 Thread Bhanuprakash Bodireddy

prefetching the data in to the caches isn't improving the performance in
cmap_find_batch(). Moreover its found that there is slight improvement
in performance with out prefetching.

This patch removes prefetching from cmap_find_batch().

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/cmap.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/lib/cmap.c b/lib/cmap.c
index 8c7312d..4c34bda 100644
--- a/lib/cmap.c
+++ b/lib/cmap.c
@@ -397,7 +397,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,
 ULLONG_FOR_EACH_1(i, map) {
 h1s[i] = rehash(impl, hashes[i]);
 b1s[i] = >buckets[h1s[i] & impl->mask];
-OVS_PREFETCH(b1s[i]);
 }
 /* Lookups, Round 1. Only look up at the first bucket. */
 ULLONG_FOR_EACH_1(i, map) {
@@ -413,13 +412,11 @@ cmap_find_batch(const struct cmap *cmap, unsigned long 
map,
 if (!node) {
 /* Not found (yet); Prefetch the 2nd bucket. */
 b2s[i] = >buckets[other_hash(h1s[i]) & impl->mask];
-OVS_PREFETCH(b2s[i]);
 c1s[i] = c1; /* We may need to check this after Round 2. */
 continue;
 }
 /* Found. */
 ULLONG_SET0(map, i); /* Ignore this on round 2. */
-OVS_PREFETCH(node);
 nodes[i] = node;
 }
 /* Round 2. Look into the 2nd bucket, if needed. */
@@ -453,7 +450,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,
 continue;
 }
 found:
-OVS_PREFETCH(node);
 nodes[i] = node;
 }
 return result;
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 08/12] dpif-netdev: Reorder elements in dp_netdev_port structure.

2016-10-07 Thread Bhanuprakash Bodireddy

By reordering the data elements in dp_netdev_port structure, pad bytes
can be reduced and there by saving a cache line.

Before: structure size:136, holes:3, sum padbytes:15, cachelines:3
After: structure size:128, holes:1, sum padbytes:7, cachelines:2

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/dpif-netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index dfc9cbd..262f4de 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -284,10 +284,10 @@ struct dp_netdev_rxq {
 /* A port in a netdev-based datapath. */
 struct dp_netdev_port {
 odp_port_t port_no;
+unsigned n_rxq; /* Number of elements in 'rxq' */
 struct netdev *netdev;
 struct hmap_node node;  /* Node in dp_netdev's 'ports'. */
 struct netdev_saved_flags *sf;
-unsigned n_rxq; /* Number of elements in 'rxq' */
 struct dp_netdev_rxq *rxqs;
 bool dynamic_txqs;  /* If true XPS will be used. */
 unsigned *txq_used; /* Number of threads that uses each tx queue. 
*/
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches inside packet_batch_execute.

2016-10-07 Thread Bhanuprakash Bodireddy

There is a slight negative performance impact, by zeroing out the flow
batch pointers in dp_netdev_input__ ahead of packet_batch_execute(). The
issue has been observed with multiple batches test scenario.

This patch fixes the problem by removing the extra for loop and clear
the flow batches inside the packet_batch_per_flow_execute(). Also the
vtune analysis showed that the overall no. of instructions retired for
dp_netdev_input__ reduced by 1,273,800,000 with this patch.

Fixes: 603f2ce04d00 ("dpif-netdev: Clear flow batches before execute.")
Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/dpif-netdev.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c002dd3..d0bb191 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3878,6 +3878,7 @@ packet_batch_per_flow_execute(struct 
packet_batch_per_flow *batch,
 {
 struct dp_netdev_actions *actions;
 struct dp_netdev_flow *flow = batch->flow;
+flow->batch = NULL;
 
 dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
 batch->tcp_flags, now);
@@ -4173,10 +4174,6 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
 }
 
 for (i = 0; i < n_batches; i++) {
-batches[i].flow->batch = NULL;
-}
-
-for (i = 0; i < n_batches; i++) {
 packet_batch_per_flow_execute([i], pmd, now);
 }
 }
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 04/12] hash: Skip invoking mhash_add__() with zero input.

2016-10-07 Thread Bhanuprakash Bodireddy

mhash_add__() is expensive and should be only called with valid input.
This patch will validate the input before invoking the mhash_add__ and
there by saving some cpu cycles.

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/hash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/hash.h b/lib/hash.h
index 114a419..9bfebdb 100644
--- a/lib/hash.h
+++ b/lib/hash.h
@@ -70,7 +70,7 @@ static inline uint32_t mhash_add__(uint32_t hash, uint32_t 
data)
 
 static inline uint32_t mhash_add(uint32_t hash, uint32_t data)
 {
-hash = mhash_add__(hash, data);
+hash = data ? mhash_add__(hash, data): hash;
 hash = hash_rot(hash, 13);
 return hash * 5 + 0xe6546b64;
 }
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 03/12] flow: Skip invoking expensive count_1bits() with zero input.

2016-10-07 Thread Bhanuprakash Bodireddy

This patch checks if trash is non-zero and only then resets the flowmap
bit and increment the pointer by set bits as found in trash.

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/flow.h | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index 4eb19ae..8cfd243 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -609,11 +609,13 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
  * corresponding data chunks should be skipped accordingly. */
 map_t trash = *fmap & (rm1bit - 1);
 
-*fmap -= trash;
+if (trash) {
+*fmap -= trash;
 /* count_1bits() is fast for systems where speed matters (e.g.,
  * DPDK), so we don't try avoid using it.
  * Advance 'aux->values' to point to the value for 'rm1bit'. */
-aux->values += count_1bits(trash);
+aux->values += count_1bits(trash);
+}
 
 *value = *aux->values;
 } else {
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 02/12] flow: Add comments to mf_get_next_in_map()

2016-10-07 Thread Bhanuprakash Bodireddy

Signed-off-by: Bhanuprakash Bodireddy 
Signed-off-by: Antonio Fischetti 
---
 lib/flow.h | 23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index ea24e28..4eb19ae 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -570,15 +570,27 @@ struct mf_for_each_in_map_aux {
 const uint64_t *values;
 };
 
+/*
+ * Parse the bitmap mask of the subtable and output the next value
+ * from the search-key.
+ */
 static inline bool
 mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
uint64_t *value)
 {
-map_t *map, *fmap;
+map_t *map;/* map refers to the bitmap mask of the subtable. */
+map_t *fmap;   /* fmap refers to the bitmap of the search-key. */
 map_t rm1bit;
 
+/* The bitmap mask selects which value must be considered from the
+ * search-key. We process the corresponding 'unit' of size 64 bits.
+ * 'aux->unit' is an index to the current unit of 64 bits. */
 while (OVS_UNLIKELY(!*(map = >map.bits[aux->unit]))) {
-/* Skip remaining data in the previous unit. */
+/* No bits are enabled, so no search-key value will be output.
+ * In case some of the corresponding data chunks in the search-key
+ * bitmap are set, the data chunks must be skipped, as they are not
+ * considered by this mask. This is handled by incrementing aux->values
+ * accordingly. */
 aux->values += count_1bits(aux->fmap.bits[aux->unit]);
 if (++aux->unit == FLOWMAP_UNITS) {
 return false;
@@ -589,7 +601,12 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
 *map -= rm1bit;
 fmap = >fmap.bits[aux->unit];
 
+/* If the 8-byte value referred by 'rm1bit' is present in the
+ * search-key return 'value', otherwise return 0. */
 if (OVS_LIKELY(*fmap & rm1bit)) {
+/* The value is in the search-key, if the search-key contains other
+ * values preceeding the 'rm1bit' bit, we consider it trash and the
+ * corresponding data chunks should be skipped accordingly. */
 map_t trash = *fmap & (rm1bit - 1);
 
 *fmap -= trash;
@@ -600,6 +617,8 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
 
 *value = *aux->values;
 } else {
+/* The search-key does not contain a value that corresponds to
+ * rm1bit. */
 *value = 0;
 }
 return true;
-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 01/12] dpcls: Use 32 packet batches for lookups.

2016-10-07 Thread Bhanuprakash Bodireddy

This patch increases the number of packets processed in a batch during a
lookup from 16 to 32. Processing batches of 32 packets improves
performance and also one of the internal loops can be avoided here.

Signed-off-by: Antonio Fischetti 
Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/dpif-netdev.c | 109 +++---
 1 file changed, 46 insertions(+), 63 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6e09e44..c002dd3 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4975,23 +4975,20 @@ dpcls_lookup(struct dpcls *cls, const struct 
netdev_flow_key keys[],
  int *num_lookups_p)
 {
 /* The received 'cnt' miniflows are the search-keys that will be processed
- * in batches of 16 elements.  N_MAPS will contain the number of these
- * 16-elements batches.  i.e. for 'cnt' = 32, N_MAPS will be 2.  The batch
- * size 16 was experimentally found faster than 8 or 32. */
-typedef uint16_t map_type;
+ * to find a matching entry into the available subtables.
+ * The number of bits in map_type is equal to NETDEV_MAX_BURST. */
+typedef uint32_t map_type;
 #define MAP_BITS (sizeof(map_type) * CHAR_BIT)
 
-#if !defined(__CHECKER__) && !defined(_WIN32)
-const int N_MAPS = DIV_ROUND_UP(cnt, MAP_BITS);
-#else
-enum { N_MAPS = DIV_ROUND_UP(NETDEV_MAX_BURST, MAP_BITS) };
-#endif
-map_type maps[N_MAPS];
 struct dpcls_subtable *subtable;
 
-memset(maps, 0xff, sizeof maps);
-if (cnt % MAP_BITS) {
-maps[N_MAPS - 1] >>= MAP_BITS - cnt % MAP_BITS; /* Clear extra bits. */
+map_type keys_map = 0x;
+map_type found_map;
+uint32_t hashes[MAP_BITS];
+const struct cmap_node *nodes[MAP_BITS];
+
+if (OVS_UNLIKELY(cnt != NETDEV_MAX_BURST)) {
+keys_map >>= NETDEV_MAX_BURST - cnt; /* Clear extra bits. */
 }
 memset(rules, 0, cnt * sizeof *rules);
 
@@ -5007,59 +5004,45 @@ dpcls_lookup(struct dpcls *cls, const struct 
netdev_flow_key keys[],
 PVECTOR_FOR_EACH (subtable, >subtables) {
 const struct netdev_flow_key *mkeys = keys;
 struct dpcls_rule **mrules = rules;
-map_type remains = 0;
-int m;
-
-BUILD_ASSERT_DECL(sizeof remains == sizeof *maps);
-
-/* Loops on each batch of 16 search-keys. */
-for (m = 0; m < N_MAPS; m++, mkeys += MAP_BITS, mrules += MAP_BITS) {
-uint32_t hashes[MAP_BITS];
-const struct cmap_node *nodes[MAP_BITS];
-unsigned long map = maps[m];
-int i;
-
-if (!map) {
-continue; /* Skip empty maps. */
-}
-
-/* Compute hashes for the remaining keys.  Each search-key is
- * masked with the subtable's mask to avoid hashing the wildcarded
- * bits. */
-ULLONG_FOR_EACH_1(i, map) {
-hashes[i] = netdev_flow_key_hash_in_mask([i],
- >mask);
-}
-/* Lookup. */
-map = cmap_find_batch(>rules, map, hashes, nodes);
-/* Check results.  When the i-th bit of map is set, it means that a
- * set of nodes with a matching hash value was found for the i-th
- * search-key.  Due to possible hash collisions we need to check
- * which of the found rules, if any, really matches our masked
- * search-key. */
-ULLONG_FOR_EACH_1(i, map) {
-struct dpcls_rule *rule;
-
-CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
-if (OVS_LIKELY(dpcls_rule_matches_key(rule, [i]))) {
-mrules[i] = rule;
-/* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
- * within one second optimization interval  */
-subtable->hit_cnt++;
-lookups_match += subtable_pos;
-goto next;
-}
+int i;
+found_map = keys_map;
+
+/* Compute hashes for the remaining keys.  Each search-key is
+ * masked with the subtable's mask to avoid hashing the wildcarded
+ * bits. */
+ULLONG_FOR_EACH_1(i, keys_map) {
+hashes[i] = netdev_flow_key_hash_in_mask([i],
+ >mask);
+}
+/* Lookup. */
+found_map = cmap_find_batch(>rules, found_map, hashes,
+nodes);
+/* Check results.  When the i-th bit of found_map is set, it means
+ * that a set of nodes with a matching hash value was found for the
+ * i-th search-key.  Due to possible hash collisions we need to check
+ * which of the found rules, if any, really matches our masked
+ * search-key. */
+ULLONG_FOR_EACH_1(i,

[ovs-dev] [PATCH 00/12] Improve performance of OVS-DPDK classifier

2016-10-07 Thread Bhanuprakash Bodireddy

This patch series is aimed at improving the performance of OVS-DPDK
dpcls.

With few thousands flows installed, the EMC becomes inefficient due
to thrashing and the bottleneck moves to the dpcls. In EMC disabled
case, through VTune we found that significant performance degradation
is due to LLC thrashing, memory latency, machine clears and expensive
hash computation.

This first patch-set improves the dpcls performance by 15% (~1 Mpps)
when EMC is disabled and OVS-DPDK built with CFLAGS="-O2 -g".

Bhanuprakash Bodireddy (12):
  dpcls: Use 32 packet batches for lookups.
Comment: ~120k performance throughput improvement.

  flow: Add comments to mf_get_next_in_map()
Comment: Add comments to function.

  flow: Skip invoking expensive count_1bits() with zero input.
Comment: ~630k performance throughput improvement.

  hash: Skip invoking mhash_add__() with zero input.
Comment: ~150k performance throughput improvement.

  dpif-netdev: Clear flow batches inside packet_batch_execute.
Comment: ~50k performance throughput improvement with multiple batches 
test case.

  cmap: Remove prefetching in cmap_find_batch().
Comment: ~39k performance throughput improvement.

  dpif-netdev: Cache align netdev_flow_keys.
Comment: ~170k performance throughput improvement in EMC enabled case.

  dpif-netdev: Reorder elements in dp_netdev_port structure.
  dpif: Reorder elements in dpif_upcall structure.
  ovsdb: Reorder elements in ovsdb_table_schema structure.
  netlink-socket: Reorder elements in nl_dump structure.
  timeval: Reorder elements in clock structure.
Comment: Reorder memeber variables of the structures to reduce pad bytes
 and there by memory footprint.

 lib/cmap.c   |   4 --
 lib/dpif-netdev.c| 118 +--
 lib/dpif.h   |  17 
 lib/flow.h   |  29 +++--
 lib/hash.h   |   2 +-
 lib/netlink-socket.h |   6 +--
 lib/timeval.c|   4 +-
 ovsdb/table.h|   4 +-
 8 files changed, 91 insertions(+), 93 deletions(-)

-- 
2.4.11

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-nb: Document the syntax for an address set name.

2016-10-07 Thread Ben Pfaff

Also, it is not necessary to specify that the name must be unique because
the schema documentation generator does that for us.

Reported-by: Kevin Lin 
Reported-at: http://openvswitch.org/pipermail/dev/2016-October/080386.html
Signed-off-by: Ben Pfaff 
---
 AUTHORS| 1 +
 ovn/ovn-nb.xml | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/AUTHORS b/AUTHORS
index a30a5d8..46bd59e 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -371,6 +371,7 @@ John Hurley john.hur...@netronome.com
 John Reumannnofutznetwo...@gmail.com
 Kashyap Thimmaraju  kashyap.thimmar...@sec.t-labs.tu-berlin.de
 Keith Holleman  hollemani...@gmail.com
+Kevin Lin   kevin...@berkeley.edu
 K 華k940...@hotmail.com
 Kevin Mancuso   kevin.manc...@rackspace.com
 Kiran Shanbhog  ki...@vmware.com
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index e1b3136..a8943d8 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -691,7 +691,8 @@
 
 
 
-  A name for the address set.  This must be unique among all address sets.
+  A name for the address set.  Names are ASCII and must match
+  [a-zA-Z_.][a-zA-Z_.0-9]*.
 
 
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-northd: Drop redundant matching constraints in build_stateful().

2016-10-07 Thread Ben Pfaff

On Thu, Oct 06, 2016 at 04:19:59PM -0700, Justin Pettit wrote:
> 
> > On Oct 5, 2016, at 6:27 PM, Ben Pfaff  wrote:
> > 
> > ip4.dst implies ip, udp.dst implies udp, and tcp.dst implies tcp.
> > 
> > Signed-off-by: Ben Pfaff 
> 
> Acked-by: Justin Pettit 

Thanks, applied to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/7] ovn: Fix some races in IPAM connectivity test.

2016-10-07 Thread Ben Pfaff

On Fri, Oct 07, 2016 at 07:27:54AM -0700, Guru Shetty wrote:
> On 5 October 2016 at 18:26, Ben Pfaff  wrote:
> 
> > It can take a way for dynamic addresses to propagate through ovn-northd,
> > so wait for it to happen.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  tests/ovn.at | 13 -
> >  1 file changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 4214a00..9baa1df 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -4724,23 +4724,18 @@ ovn-nbctl lsp-add alice rp-alice -- set
> > Logical_Switch_Port rp-alice type=router
> >  # Create logical port foo1 in foo
> >  ovn-nbctl lsp-add foo foo1 \
> >  -- lsp-set-addresses foo1 "dynamic"
> > -AT_CHECK([ovn-nbctl get Logical-Switch-Port foo1 dynamic_addresses], [0],
> > - ["0a:00:00:00:00:01 192.168.1.2"
> > -])
> > +AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port foo1
> > dynamic_addresses='"0a:00:00:00:00:01 192.168.1.2"'], [0])
> >
> >  # Create logical port alice1 in alice
> >  ovn-nbctl lsp-add alice alice1 \
> >  -- lsp-set-addresses alice1 "dynamic"
> > -AT_CHECK([ovn-nbctl get Logical-Switch-Port alice1 dynamic_addresses],
> > [0],
> > - ["0a:00:00:00:00:02 192.168.2.2"
> > -])
> > +AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port alice1
> > dynamic_addresses='"0a:00:00:00:00:02 192.168.2.2"'])
> >
> >  # Create logical port foo2 in foo
> >  ovn-nbctl lsp-add foo foo2 \
> >  -- lsp-set-addresses foo2 "dynamic"
> > -AT_CHECK([ovn-nbctl get Logical-Switch-Port foo2 dynamic_addresses], [0],
> > - ["0a:00:00:00:00:03 192.168.1.3"
> > -])
> > +sleep 1
> >
> 
> Why a sleep 1?

I think that must have crept in as I was experimenting.  It should not
be necessary.  I've removed it now.

> 
> > +AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port foo2
> > dynamic_addresses='"0a:00:00:00:00:03 192.168.1.3"'])
> >
> >  # Create a hypervisor and create OVS ports corresponding to logical ports.
> >  net_add n1
> >
> Acked-by: Gurucharan Shetty 

Thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/7] ovn: Fix races in MAC_Binding deletion test.

2016-10-07 Thread Ben Pfaff

On Fri, Oct 07, 2016 at 07:24:29AM -0700, Guru Shetty wrote:
> On 5 October 2016 at 18:26, Ben Pfaff  wrote:
> 
> > The test assumed that ovn-northd could delete the MAC_Binding rows
> > instantly, but it may take a while.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  tests/ovn.at | 9 -
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 948716b..05e1349 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -5108,15 +5108,14 @@ dp_uuid=`ovn-sbctl find datapath | grep uuid | cut
> > -f2 -d ":" | cut -f2 -d " "`
> >  ovn-sbctl create MAC_Binding ip=10.0.0.1 datapath=$dp_uuid
> > logical_port=lp0 mac="mac1"
> >  ovn-sbctl create MAC_Binding ip=10.0.0.1 datapath=$dp_uuid
> > logical_port=lp1 mac="mac2"
> >  ovn-sbctl find MAC_Binding
> > -#Delete port lp0
> > +# Delete port lp0 and check that its MAC_Binding is deleted.
> >  ovn-nbctl lsp-del lp0
> >  ovn-sbctl find MAC_Binding
> >
> Is there is a use for above line?

It cannot affect the result of the test.  I guess that it is there so
that if the following OVS_WAIT_UNTIL (formerly AT_CHECK) fails, then the
person reading the log has an idea what was in the MAC_Binding table.
So I think that it is worthwhile enough to leave it in place.

> -AT_CHECK([ovn-sbctl find MAC_Binding logical_port=lp0], [0], [])
> > -#Delete ls0. This will verify that the mac_bindings are cleaned up when a
> > -#datapath is deleted without explicitly removing the the logical ports
> > +OVS_WAIT_UNTIL([test `ovn-sbctl find MAC_Binding logical_port=lp0 | wc
> > -l` = 0])
> > +# Delete logical switch ls0 and check that its MAC_Binding is deleted.
> >  ovn-nbctl ls-del ls0
> >  ovn-sbctl find MAC_Binding
> > -AT_CHECK([ovn-sbctl find MAC_Binding], [0], [])
> > +OVS_WAIT_UNTIL([test `ovn-sbctl find MAC_Binding | wc -l` = 0])
> >
> >  OVN_CLEANUP([hv1])
> >
> Acked-by: Gurucharan Shetty 

Thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 7/7] ovn: Remove weird or unneeded keywords from tests.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> AT_KEYWORDS are mostly there to make it easier to find the tests you're
> looking for.  One might, for example, mark tests as "positive" or
> "negative" so you can select the tests you want to run on that basis.
> They're also useful for cases where Autotest just isn't good at splitting
> words: for example, Autotest includes punctuation so that a test name
> that has a word followed by a comma or colon won't be selected using a
> keyword that lacks the comma or the colon.
>
> But a lot of OVN tests had keywords that just didn't seem helpful in one
> of these ways.  For example, it's hard to guess why running together
> words into a longer word would help someone select a test, and it's not
> helpful at all to repeat one of the words in the test name, since those
> words are keywords by default anyway.
>
> Therefore, this commit removes the keywords that don't seem helpful.
>
> Signed-off-by: Ben Pfaff 
>

Till today, I had no idea that you could give keywords from the test name.
Acked-by: Gurucharan Shetty 


> ---
>  tests/ovn.at | 24 ++--
>  1 file changed, 2 insertions(+), 22 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index c4a600a..cfc6a35 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1468,7 +1468,6 @@ AT_CLEANUP
>  # 2 locally attached networks (one flat, one vlan tagged over same device)
>  # 2 ports per HV on each network
>  AT_SETUP([ovn -- 2 HVs, 4 lports/HV, localnet ports])
> -AT_KEYWORDS([ovn-localnet])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -2375,7 +2374,6 @@ AT_CLEANUP
>
>  # 3 hypervisors, one logical switch, 3 logical ports per hypervisor
>  AT_SETUP([ovn -- portsecurity : 3 HVs, 1 LS, 3 lports/HV])
> -AT_KEYWORDS([portsecurity])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -2753,7 +2751,6 @@ OVN_CLEANUP([hv1],[hv2],[hv3])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- 2 HVs, 2 LS, 1 lport/LS, 2 peer LRs])
> -AT_KEYWORDS([ovnpeer])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -3121,7 +3118,6 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- 2 HVs, 3 LS, 1 lport/LS, 2 peer LRs, static routes])
> -AT_KEYWORDS([ovnstaticroutespeer])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -3275,7 +3271,6 @@ OVN_CLEANUP([hv1],[hv2])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- send gratuitous arp on localnet])
> -AT_KEYWORDS([ovn])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>  ovn-nbctl ls-add lsw0
> @@ -3317,7 +3312,6 @@ OVN_CLEANUP([hv])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- 2 HVs, 3 LRs connected via LS, static routes])
> -AT_KEYWORDS([ovnstaticroutes])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -3495,7 +3489,6 @@ OVN_CLEANUP([hv1],[hv2])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- dhcpv4 : 1 HV, 2 LS, 2 LSPs/LS])
> -AT_KEYWORDS([dhcpv4])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -3765,7 +3758,6 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- dhcpv6 : 1 HV, 2 LS, 5 LSPs])
> -AT_KEYWORDS([dhcpv6])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -4027,7 +4019,6 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- 2 HVs, 2 LRs connected via LS, gateway router])
> -AT_KEYWORDS([ovngatewayrouter])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -4353,7 +4344,6 @@ AT_CLEANUP
>  # make sure that the port state is properly set to up and back down
>  # when created and deleted.
>  AT_SETUP([ovn -- port state up and down])
> -AT_KEYWORDS([ovn])
>  ovn_start
>
>  ovn-nbctl ls-add ls1
> @@ -4379,7 +4369,7 @@ AT_CLEANUP
>  # make sure that the OF rules created to support a datapath are
> added/cleared
>  # when logical switch is created and removed.
>  AT_SETUP([ovn -- datapath rules added/removed])
> -AT_KEYWORDS([ovn datapath cleanup])
> +AT_KEYWORDS([cleanup])
>  ovn_start
>
>  net_add n1
> @@ -4443,7 +4433,6 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- nd_na ])
> -AT_KEYWORDS([ovn-nd_na])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -4516,7 +4505,6 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- address sets modification/removal smoke test])
> -AT_KEYWORDS([ovn-addr])
>  ovn_start
>
>  net_add n1
> @@ -4544,7 +4532,6 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- ipam])
> -AT_KEYWORDS([ovnipam])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -4701,7 +4688,6 @@ OVS_APP_EXIT_AND_WAIT([ovn-northd])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- ipam connectivity])
> -AT_KEYWORDS([ovnipamconnectivity])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -4830,7 +4816,7 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- ovs-vswitchd restart])
> -AT_KEYWORDS([vswitchd restart])
> +AT_KEYWORDS([vswitchd])
>  AT_SKIP_IF([test $HAVE_PYTHON = no])
>  ovn_start
>
> @@ -4925,7 +4911,6 @@ OVN_CLEANUP([hv1])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- send arp for

Re: [ovs-dev] [PATCH 6/7] ovn: Fix some races in ovn-controller-vtep tests.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> This fixes a few races for port bindings appearing and being bound to
> a chassis.  The ones changed to use "ovn-sbctl wait-until" were previously
> only waiting until a Port_Binding record appeared (created by ovn-northd),
> but not until the Port_Binding record's 'chassis' column was set (by
> ovn-controller).
>
> Signed-off-by: Ben Pfaff 
>
 Acked-by: Gurucharan Shetty 

> ---
>  tests/ovn-controller-vtep.at | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
> index 654c212..aeb1ec1 100644
> --- a/tests/ovn-controller-vtep.at
> +++ b/tests/ovn-controller-vtep.at
> @@ -190,7 +190,7 @@ OVN_CONTROLLER_VTEP_START
>  AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0 --
> bind-ls br-vtep p1 300 lswitch0])
>  # adds logical switch port in ovn-nb database, and sets the type and
> options.
>  OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep],
> [lswitch0])
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep
> br-vtep_lswitch0`"])
> +ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0
> chassis!='[[]]'
>  # should see one binding, associated to chassis of 'br-vtep'.
>  chassis_uuid=$(ovn-sbctl --columns=_uuid list Chassis br-vtep | cut -d
> ':' -f2 | tr -d ' ')
>  AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Port_Binding
> br-vtep_lswitch0 | cut -d ':' -f2 | tr -d ' '], [0], [dnl
> @@ -201,7 +201,7 @@ ${chassis_uuid}
>  AT_CHECK([vtep-ctl add-ls lswitch1 -- bind-ls br-vtep p0 200 lswitch1])
>  # adds logical switch port in ovn-nb database for lswitch1.
>  OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch1], [br-vtep],
> [lswitch1])
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding | grep --
> br-vtep_lswitch1`"])
> +ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch1
> chassis!='[[]]'
>  # This is allowed, but not recommended, to have two vlan_bindings (to
> different vtep logical switches)
>  # from one vtep gateway physical port in one ovn-nb logical swithch.
>  AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Port_Binding | cut -d
> ':' -f2 | tr -d ' ' | sort], [0], [dnl
> @@ -212,7 +212,7 @@ ${chassis_uuid}
>
>  # adds another logical switch port in ovn-nb database for lswitch0.
>  OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0_dup], [br-vtep],
> [lswitch0])
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding | grep --
> br-vtep_lswitch0_dup`"])
> +ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0_dup
> chassis!='[[]]'
>  # it is not allowed to have more than one ovn-nb logical port for the same
>  # vtep logical switch on a vtep gateway chassis, so should still see only
>  # two port_binding entries bound.
> @@ -255,7 +255,7 @@ OVN_CONTROLLER_VTEP_START
>  AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0])
>  # adds logical switch port in ovn-nb database, and sets the type and
> options.
>  OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep],
> [lswitch0])
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep
> br-vtep_lswitch0`"])
> +ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0
> chassis!='[[]]'
>
>  # adds another lswitch 'br-void' in ovn-nb database.
>  AT_CHECK([ovn-nbctl ls-add br-void])
> @@ -264,7 +264,7 @@ AT_CHECK([vtep-ctl add-ps br-vtep-void -- add-port
> br-vtep-void p0-void -- bind-
>  # adds a conflicting logical port (both br-vtep_lswitch0 and
> br-vtep-void_lswitch0
>  # are bound to the same logical switch, but they are on different
> datapath).
>  OVN_NB_ADD_VTEP_PORT([br-void], [br-vtep-void_lswitch0], [br-vtep-void],
> [lswitch0])
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep
> br-vtep-void_lswitch0`"])
> +ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0
>  OVS_WAIT_UNTIL([test -n "`grep WARN ovn-controller-vtep.log`"])
>  # confirms the warning log.
>  AT_CHECK([sed -n 's/^.*\(|WARN|.*\)$/\1/p' ovn-controller-vtep.log | sed
> 's/([[-_0-9a-z]][[-_0-9a-z]]*)/()/g;s/(with tunnel key
> [[0-9]][[0-9]]*)/()/g' | uniq], [0], [dnl
> @@ -346,6 +346,7 @@ OVN_CONTROLLER_VTEP_START
>  # 'ch0'.
>  AT_CHECK([ovn-nbctl lsp-add br-test vif0])
>  AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:02])
> +AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
>  AT_CHECK([ovn-sbctl chassis-add ch0 vxlan 1.2.3.5])
>  AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
>
> @@ -360,9 +361,9 @@ AT_CHECK([ovn-nbctl ls-add br-void])
>  # adds fake hv chassis 'ch1'.
>  AT_CHECK([ovn-nbctl lsp-add br-void vif1])
>  AT_CHECK([ovn-nbctl lsp-set-addresses vif1 f0:ab:cd:ef:01:02])
> +AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
>  AT_CHECK([ovn-sbctl chassis-add ch1 vxlan 1.2.3.6])
>  AT_CHECK([ovn-sbctl lsp-bind vif1 ch1])
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding | grep vif1`"])
>
>  # checks Ucast_Macs_Remote

Re: [ovs-dev] [PATCH 5/7] ovn: Wait for ovn-northd to catch up in "ovn-sbctl" test.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
>
Acked-by: Gurucharan Shetty 

> ---
>  tests/ovn-sbctl.at | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
> index 9393eef..26b05af 100644
> --- a/tests/ovn-sbctl.at
> +++ b/tests/ovn-sbctl.at
> @@ -87,6 +87,7 @@ AT_CHECK([ovn-nbctl ls-add br-test])
>  AT_CHECK([ovn-nbctl lsp-add br-test vif0])
>  AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:02])
>  AT_CHECK([ovn-sbctl chassis-add ch0 stt 1.2.3.5])
> +AT_CHECK([ovn-nbctl --wait=sb sync])
>  AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
>
>  AT_CHECK([ovn-sbctl show], [0], [dnl
> @@ -98,7 +99,7 @@ Chassis "ch0"
>  ])
>
>  # adds another 'vif1'
> -AT_CHECK([ovn-nbctl lsp-add br-test vif1])
> +AT_CHECK([ovn-nbctl --wait=sb lsp-add br-test vif1])
>  AT_CHECK([ovn-nbctl lsp-set-addresses vif1 f0:ab:cd:ef:01:03])
>  AT_CHECK([ovn-sbctl lsp-bind vif1 ch0])
>
> @@ -113,6 +114,7 @@ Chassis "ch0"
>
>  # deletes 'vif1'
>  AT_CHECK([ovn-nbctl lsp-del vif1])
> +AT_CHECK([ovn-nbctl --wait=sb sync])
>
>  AT_CHECK([ovn-sbctl show], [0], [dnl
>  Chassis "ch0"
> @@ -130,12 +132,12 @@ chassis : ${uuid}
>  ])
>
>  # test the passing down of logical port type and options.
> -AT_CHECK([ovn-nbctl lsp-add br-test vtep0])
> +AT_CHECK([ovn-nbctl --wait=sb lsp-add br-test vtep0])
>  AT_CHECK([ovn-nbctl lsp-set-type vtep0 vtep])
>  AT_CHECK([ovn-nbctl lsp-set-options vtep0 vtep_physical_switch=p0
> vtep_logical_switch=l0])
>
> -OVS_WAIT_UNTIL([test -n "`ovn-sbctl --columns=logical_port list
> Port_Binding | grep vtep0`" ])
> -AT_CHECK_UNQUOTED([ovn-sbctl --columns=logical_port,mac,type,options
> list Port_Binding vtep0], [0], [dnl
> +AT_CHECK([ovn-sbctl --timeout=10 wait-until Port_Binding vtep0
> options!={}])
> +AT_CHECK([ovn-sbctl --columns=logical_port,mac,type,options list
> Port_Binding vtep0], [0], [dnl
>  logical_port: "vtep0"
>  mac : [[]]
>  type: vtep
> --
> 2.1.3
>
> ___
> 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/7] ovn: Fix race in "ovn -- ipam" test.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> After setting the subnet, ovn-northd needs to process the changes before
> setting the dynamic addresses.
>
> Signed-off-by: Ben Pfaff 
>
Acked-by: Gurucharan Shetty 

> ---
>  tests/ovn.at | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9baa1df..c4a600a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -4552,7 +4552,7 @@ ovn_start
>  # subnet which should result in an address being allocated for the port.
>  ovn-nbctl ls-add sw0
>  ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
> -ovn-nbctl add Logical-Switch sw0 other_config subnet=192.168.1.0/24
> +ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=
> 192.168.1.0/24
>  AT_CHECK([ovn-nbctl get Logical-Switch-Port p0 dynamic_addresses], [0],
>  ["0a:00:00:00:00:01 192.168.1.2"
>  ])
> --
> 2.1.3
>
> ___
> 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 3/7] ovn: Fix some races in IPAM connectivity test.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> It can take a way for dynamic addresses to propagate through ovn-northd,
> so wait for it to happen.
>
> Signed-off-by: Ben Pfaff 
> ---
>  tests/ovn.at | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4214a00..9baa1df 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -4724,23 +4724,18 @@ ovn-nbctl lsp-add alice rp-alice -- set
> Logical_Switch_Port rp-alice type=router
>  # Create logical port foo1 in foo
>  ovn-nbctl lsp-add foo foo1 \
>  -- lsp-set-addresses foo1 "dynamic"
> -AT_CHECK([ovn-nbctl get Logical-Switch-Port foo1 dynamic_addresses], [0],
> - ["0a:00:00:00:00:01 192.168.1.2"
> -])
> +AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port foo1
> dynamic_addresses='"0a:00:00:00:00:01 192.168.1.2"'], [0])
>
>  # Create logical port alice1 in alice
>  ovn-nbctl lsp-add alice alice1 \
>  -- lsp-set-addresses alice1 "dynamic"
> -AT_CHECK([ovn-nbctl get Logical-Switch-Port alice1 dynamic_addresses],
> [0],
> - ["0a:00:00:00:00:02 192.168.2.2"
> -])
> +AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port alice1
> dynamic_addresses='"0a:00:00:00:00:02 192.168.2.2"'])
>
>  # Create logical port foo2 in foo
>  ovn-nbctl lsp-add foo foo2 \
>  -- lsp-set-addresses foo2 "dynamic"
> -AT_CHECK([ovn-nbctl get Logical-Switch-Port foo2 dynamic_addresses], [0],
> - ["0a:00:00:00:00:03 192.168.1.3"
> -])
> +sleep 1
>

Why a sleep 1?


> +AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port foo2
> dynamic_addresses='"0a:00:00:00:00:03 192.168.1.3"'])
>
>  # Create a hypervisor and create OVS ports corresponding to logical ports.
>  net_add n1
>
Acked-by: Gurucharan Shetty 


> --
> 2.1.3
>
> ___
> 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 2/7] ovn: Fix bugs in port security test.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> A number of instances of "{i}" in this test should have been "${i}".
>
> Signed-off-by: Ben Pfaff 
>
Acked-by: Gurucharan Shetty 

> ---
>  tests/ovn.at | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 05e1349..4214a00 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -2626,13 +2626,13 @@ sip=`ip_to_hex 192 168 0 13`
>  tip=`ip_to_hex 192 168 0 23`
>  test_ip 13 f113 f023 $sip $tip
>
> -# ipv6 packet should be received by lp[123]3 with mac f000{i}{i}3
> -# and ip6.dst as fe80::ea2a:eaff:fe28:0{i}{i}3.
> +# ipv6 packet should be received by lp[123]3 with mac f${i}${i}3
> +# and ip6.dst as fe80::ea2a:eaff:fe28:0${i}${i}3.
>  # lp11 can send ipv6 traffic as there is no port security
>  sip=ee80
>  for i in 1 2 3; do
> -tip=fe80ea2aeafffe2800{i}3
> -test_ipv6 11 f011 f0{i}${i}3 $sip $tip {i}3
> +tip=fe80ea2aeafffe2800${i}3
> +test_ipv6 11 f011 f${i}${i}3 $sip $tip ${i}3
>  done
>
>
> @@ -2645,8 +2645,8 @@ sip=ee80
>  tip=fe80ea2aeafffe280023
>  test_ipv6 11 f011 f333 $sip $tip
>
> -# ipv6 packet should be allowed for lp[123]3 with mac f000{i}{i}3
> -# and ip6.src fe80::ea2a:eaff:fe28:0{i}{i}3 and ip6.src ::.
> +# ipv6 packet should be allowed for lp[123]3 with mac f000${i}${i}3
> +# and ip6.src fe80::ea2a:eaff:fe28:0${i}${i}3 and ip6.src ::.
>  # and should be dropped for any other ip6.src
>  # lp21 can receive ipv6 traffic as there is no port security
>
> --
> 2.1.3
>
> ___
> 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 1/7] ovn: Fix races in MAC_Binding deletion test.

2016-10-07 Thread Guru Shetty

On 5 October 2016 at 18:26, Ben Pfaff  wrote:

> The test assumed that ovn-northd could delete the MAC_Binding rows
> instantly, but it may take a while.
>
> Signed-off-by: Ben Pfaff 
> ---
>  tests/ovn.at | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 948716b..05e1349 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5108,15 +5108,14 @@ dp_uuid=`ovn-sbctl find datapath | grep uuid | cut
> -f2 -d ":" | cut -f2 -d " "`
>  ovn-sbctl create MAC_Binding ip=10.0.0.1 datapath=$dp_uuid
> logical_port=lp0 mac="mac1"
>  ovn-sbctl create MAC_Binding ip=10.0.0.1 datapath=$dp_uuid
> logical_port=lp1 mac="mac2"
>  ovn-sbctl find MAC_Binding
> -#Delete port lp0
> +# Delete port lp0 and check that its MAC_Binding is deleted.
>  ovn-nbctl lsp-del lp0
>  ovn-sbctl find MAC_Binding
>
Is there is a use for above line?

-AT_CHECK([ovn-sbctl find MAC_Binding logical_port=lp0], [0], [])
> -#Delete ls0. This will verify that the mac_bindings are cleaned up when a
> -#datapath is deleted without explicitly removing the the logical ports
> +OVS_WAIT_UNTIL([test `ovn-sbctl find MAC_Binding logical_port=lp0 | wc
> -l` = 0])
> +# Delete logical switch ls0 and check that its MAC_Binding is deleted.
>  ovn-nbctl ls-del ls0
>  ovn-sbctl find MAC_Binding
> -AT_CHECK([ovn-sbctl find MAC_Binding], [0], [])
> +OVS_WAIT_UNTIL([test `ovn-sbctl find MAC_Binding | wc -l` = 0])
>
>  OVN_CLEANUP([hv1])
>
Acked-by: Gurucharan Shetty 


>
> --
> 2.1.3
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

66 matches

Mail list logo