Re: [ovs-dev] [PATCH RFC] dpif-netdev: Add Cuckoo Distributor to Accelerate Megaflow Search

[Darrell Ball]

On 4/14/17, 6:10 PM, "Wang, Yipeng1"  wrote:

Thank you Darrell for the comments. Please take a look at my reply inlined.



> -Original Message-

> From: Darrell Ball [mailto:db...@vmware.com]

> Sent: Thursday, April 13, 2017 10:36 PM

> To: Wang, Yipeng1 ; d...@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH RFC] dpif-netdev: Add Cuckoo Distributor to

> Accelerate Megaflow Search

> 

> 

> 

> On 4/6/17, 2:48 PM, "ovs-dev-boun...@openvswitch.org on behalf of

> yipeng1.w...@intel.com"  yipeng1.w...@intel.com> wrote:

> 

> From: Yipeng Wang 

> 

> The Datapath Classifier uses tuple space search for flow 
classification.

> The rules are arranged into a set of tuples/subtables (each with a

> distinct mask).  Each subtable is implemented as a hash table and 
lookup

> is done with flow keys formed by selecting the bits from the packet 
header

> based on each subtable's mask. Tuple space search will sequentially 
search

> each subtable until a match is found. With a large number of 
subtables, a

> sequential search of the subtables could consume a lot of CPU cycles. 
In

> a testbench with a uniform traffic pattern equally distributed across 
20

> subtables, we measured that up to 65% of total execution time is 
attributed

> to the megaflow cache lookup.

> 

> This patch presents the idea of the two-layer hierarchical lookup, 
where a

> low overhead first level of indirection is accessed first, we call 
this

> level cuckoo distributor (CD). If a flow key has been inserted in the 
flow

> table the first level will indicate with high probability that which

> subtable to look into. A lookup is performed on the second level (the

> target subtable) to retrieve the result. If the key doesn’t have a 
match,

> then we revert back to the sequential search of subtables.

> 

> This patch can improve the already existing Subtable Ranking when 
traffic

> data has high entropy. Subtable Ranking helps minimize the number of

> traversed subtables when most of the traffic hit the same subtable.

> However, in the case of high entropy traffic such as traffic coming 
from

> a physical port, multiple subtables could be hit with a similar 
frequency.

> In this case the average subtable lookups per hit would be much 
greater

> than 1. In addition, CD can adaptively turn off when it finds the 
traffic

> mostly hit one subtable. Thus, CD will not be an overhead when 
Subtable

> Ranking works well.

> 

> Scheme:

> 

>  ---

> |  CD   |

>  ---

>\

> \

>  -  - -

> |sub  ||sub  |...|sub  |

> |table||table|   |table|

>  -  - -

> 

> Evaluation:

> 

> We create set of rules with various src IP. We feed traffic 
containing 1

> million flows with various src IP and dst IP. All the flows hit 
10/20/30

> rules creating 10/20/30 subtables.

> 

> The table below shows the preliminary continuous testing results 
(full line

> speed test) we collected with a uni-directional port-to-port setup. 
The

> machine we tested on is a Xeon E5 server running with 2.2GHz cores. 
OvS

> runs with 1 PMD. We use Spirent as the hardware traffic generator.

> 

> no.subtable: 10  20  30

> cd-ovs   3895961 3170530 2968555

> orig-ovs 2683455 1646227 1240501

> speedup  1.45x   1.92x   2.39x

> 

> 

> I have a few initial comments.

> 1) Can you present the numbers with and without __AVX2__ “enabled”.'

[Wang, Yipeng] We mainly test with AVX2 to find the upper-bound performance 
speedup of the design.  Throughput-wise, we have not optimized for the scalar 
version thus we did not present the results. If people are interested in this 
patch, we will update the implementation to consider the performance for both 
AVX and scalar in Version 2 and report the results. We may design different 
structure (mainly different entry count per bucket) for scalar and AVX to 
optimize the performance.


[Darrell] This seem interesting.
It would be nice to hear from others.
  

Re: [ovs-dev] [PATCH] compat: Fix build error in kernel 4.10.0

[Guoshuai Li]

I try remove "nf_defrag_ipv{4|6}_enabl",  and then test by lsmod:

stack@devstack:~$ lsmod | grep defrag
nf_defrag_ipv6 36864  1 nf_conntrack_ipv6
nf_defrag_ipv4 16384  1 nf_conntrack_ipv4

stack@devstack:~$ lsmod | grep conntrack
nf_conntrack_ipv6  20480  1
nf_defrag_ipv6 36864  1 nf_conntrack_ipv6
nf_conntrack_ipv4  16384  5
nf_defrag_ipv4 16384  1 nf_conntrack_ipv4


without remove, nf_defrag_ipv6 is used by openvswitch:

stack@devstack:~/neutron$ lsmod | grep defrag
nf_defrag_ipv6 36864  2 openvswitch,nf_conntrack_ipv6
nf_defrag_ipv4 16384  1 nf_conntrack_ipv4


Normally, defrag will be loaded, but is not sure whether it is absolute.



In general, we try not to rely on kernel version number. In this case,
does it make sense to look at the signature of
nf_defrag_ipv{4|6}_enable instead?


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] compat: Fix build error in kernel 4.10.0

[Guoshuai Li]

Thanks all, I'm considering rolling the following incremental into
this patch, does this make sense?

looks good to me ~~

diff --git a/datapath/linux/compat/ip_fragment.c
b/datapath/linux/compat/ip_fragment.c
index efa86fcfae1b..de08f6c6744e 100644
--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -732,9 +732,7 @@ int rpl_ip_defrag(struct net *net, struct sk_buff
*skb, u32 user)
#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,10,0)
static int __net_init ipv4_frags_init_net(struct net *net)
{
-   nf_defrag_ipv4_enable(net);
-
-   return 0;
+   return nf_defrag_ipv4_enable(net);
}
#endif

diff --git a/datapath/linux/compat/nf_conntrack_reasm.c
b/datapath/linux/compat/nf_conntrack_reasm.c
index cb6da6c64e05..5832fead800e 100644
--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -561,9 +561,7 @@ out_unlock:
#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,10,0)
static int nf_ct_net_init(struct net *net)
{
-   nf_defrag_ipv6_enable(net);
-
-   return 0;
+   return nf_defrag_ipv6_enable(net);
}
#endif


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] revalidator: Improve logging for transition_ukey().

[Jarno Rajahalme]

> On Apr 26, 2017, at 6:03 PM, Joe Stringer  wrote:
> 
> There are a few cases where more introspection into ukey transitions
> would be relevant for logging or assertion. Track the SOURCE_LOCATOR and
> thread id when states are transitioned and use these for logging.
> 
> Suggested-by: Jarno Rajahalme 
> Signed-off-by: Joe Stringer 
> ---
> ofproto/ofproto-dpif-upcall.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index ccf15a3c80b3..8aff613161d9 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -281,6 +281,10 @@ struct udpif_key {
> uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
> enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
> 
> +/* 'state' debug information. */
> +unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
> +const char *state_where OVS_GUARDED;  /* transition_ukey() locator. 
> */
> +
> /* Datapath flow actions as nlattrs.  Protected by RCU.  Read with
>  * ukey_get_actions(), and write with ukey_set_actions(). */
> OVSRCU_TYPE(struct ofpbuf *) actions;
> @@ -1484,6 +1488,8 @@ ukey_create__(const struct nlattr *key, size_t key_len,
> ukey->dump_seq = dump_seq;
> ukey->reval_seq = reval_seq;
> ukey->state = UKEY_CREATED;
> +ukey->state_thread = ovsthread_id_self();
> +ukey->state_where = OVS_SOURCE_LOCATOR;
> ukey->created = time_msec();
> memset(>stats, 0, sizeof ukey->stats);
> ukey->stats.used = used;
> @@ -1674,7 +1680,11 @@ static void
> transition_ukey(struct udpif_key *ukey, enum ukey_state dst)
> OVS_REQUIRES(ukey->mutex)
> {
> -ovs_assert(dst >= ukey->state);
> +if (dst >= ukey->state) {
> +VLOG_ABORT("Invalid ukey transition %d->%d (last transitioned from "
> +   "thread %u at %s)", ukey->state, dst, ukey->state_thread,
> +   ukey->state_where);
> +}
> if (ukey->state == dst && dst == UKEY_OPERATIONAL) {
> return;
> }
> @@ -1709,6 +1719,8 @@ transition_ukey(struct udpif_key *ukey, enum ukey_state 
> dst)
>  ds_cstr(), ukey->state, dst);
> ds_destroy();
> }
> +ukey->state_thread = ovsthread_id_self();
> +ukey->state_where = OVS_SOURCE_LOCATOR;

You’ll want to evaluate OVS_SOURCE_LOCATOR at the caller of the 
transition_ukey() instead. Top do that you’ll need to add a “const char *where” 
argument, and then make a macro that uses OVS_SOURCE_LOCATOR as that additional 
argument, and make the callers use the new macro instead of calling the 
function directly.

As is it will always report the same line (right here).

  Jarno

> }
> 
> static bool
> @@ -2327,6 +2339,9 @@ revalidate(struct revalidator *revalidator)
> /* The flow is now confirmed to be in the datapath. */
> transition_ukey(ukey, UKEY_OPERATIONAL);
> } else {
> +VLOG_INFO("Unexpected ukey transition from state %d "
> +  "(last transitioned from thread %u at %s)",
> +  ukey->state, ukey->state_thread, 
> ukey->state_where);
> ovs_mutex_unlock(>mutex);
> continue;
> }
> -- 
> 2.11.1
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] revalidator: Improve logging for transition_ukey().

[Ben Pfaff]

On Wed, Apr 26, 2017 at 06:03:12PM -0700, Joe Stringer wrote:
> There are a few cases where more introspection into ukey transitions
> would be relevant for logging or assertion. Track the SOURCE_LOCATOR and
> thread id when states are transitioned and use these for logging.
> 
> Suggested-by: Jarno Rajahalme 
> Signed-off-by: Joe Stringer 

Very nice.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] revalidator: Avoid assert in transition_ukey().

[Ben Pfaff]

On Wed, Apr 26, 2017 at 06:03:11PM -0700, Joe Stringer wrote:
> There is a case where a flow is dumped from the kernel after the ukey is
> already transitioned into an EVICTING/EVICTED/DELETED state, and the
> revalidator thread attempts to shift that into UKEY_OPERATIONAL because
> it was able to dump the flow from the datapath. This resulted in
> triggering the assert in transition_ukey(). Detect this condition and
> skip handling the flow (as it's already on its way out).
> 
> Users report:
> > Program terminated with signal SIGABRT, Aborted.
> > raise () from /lib/x86_64-linux-gnu/libc.so.6
> > raise () from /lib/x86_64-linux-gnu/libc.so.6
> > abort () from /lib/x86_64-linux-gnu/libc.so.6
> > ovs_abort_valist
> > vlog_abort_valist
> > vlog_abort
> > ovs_assert_failure
> > transition_ukey (ukey=, dst=)
> > at ofproto/ofproto-dpif-upcall.c:1674
> > revalidate (revalidator=0x1cb36c8) at ofproto/ofproto-dpif-upcall.c:2324
> > udpif_revalidator (arg=0x1cb36c8) at ofproto/ofproto-dpif-upcall.c:901
> > ovsthread_wrapper (aux_=) at lib/ovs-thread.c:348
> > start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
> > clone () from /lib/x86_64-linux-gnu/libc.so.6
> 
> VMware-BZ: #1857694
> Signed-off-by: Joe Stringer 

Thank you for figuring this out.  I doubt it was easy.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] compat: Fix build error in kernel 4.10.0

[Joe Stringer]

On 25 April 2017 at 10:00, Greg Rose  wrote:
> On Tue, 2017-04-25 at 23:26 +0800, Guoshuai Li wrote:
>> In kernel 4.10.0, the function "nf_defrag_ipv6_enable" need
>> parameters "struct net *", so we need call it for each namespace init
>> to load netfilter fragment kmod.
>>
>> Reported-by: Raymond Burkholder 
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331411.html
>> Signed-off-by: Guoshuai Li 
>
> Compile tested and looks OK to me.
>
> Reviewed-by: Greg Rose 

Thanks all, I'm considering rolling the following incremental into
this patch, does this make sense?

diff --git a/datapath/linux/compat/ip_fragment.c
b/datapath/linux/compat/ip_fragment.c
index efa86fcfae1b..de08f6c6744e 100644
--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -732,9 +732,7 @@ int rpl_ip_defrag(struct net *net, struct sk_buff
*skb, u32 user)
#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,10,0)
static int __net_init ipv4_frags_init_net(struct net *net)
{
-   nf_defrag_ipv4_enable(net);
-
-   return 0;
+   return nf_defrag_ipv4_enable(net);
}
#endif

diff --git a/datapath/linux/compat/nf_conntrack_reasm.c
b/datapath/linux/compat/nf_conntrack_reasm.c
index cb6da6c64e05..5832fead800e 100644
--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -561,9 +561,7 @@ out_unlock:
#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,10,0)
static int nf_ct_net_init(struct net *net)
{
-   nf_defrag_ipv6_enable(net);
-
-   return 0;
+   return nf_defrag_ipv6_enable(net);
}
#endif
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/2] revalidator: Improve logging for transition_ukey().

[Joe Stringer]

There are a few cases where more introspection into ukey transitions
would be relevant for logging or assertion. Track the SOURCE_LOCATOR and
thread id when states are transitioned and use these for logging.

Suggested-by: Jarno Rajahalme 
Signed-off-by: Joe Stringer 
---
 ofproto/ofproto-dpif-upcall.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index ccf15a3c80b3..8aff613161d9 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -281,6 +281,10 @@ struct udpif_key {
 uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
 enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
 
+/* 'state' debug information. */
+unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
+const char *state_where OVS_GUARDED;  /* transition_ukey() locator. */
+
 /* Datapath flow actions as nlattrs.  Protected by RCU.  Read with
  * ukey_get_actions(), and write with ukey_set_actions(). */
 OVSRCU_TYPE(struct ofpbuf *) actions;
@@ -1484,6 +1488,8 @@ ukey_create__(const struct nlattr *key, size_t key_len,
 ukey->dump_seq = dump_seq;
 ukey->reval_seq = reval_seq;
 ukey->state = UKEY_CREATED;
+ukey->state_thread = ovsthread_id_self();
+ukey->state_where = OVS_SOURCE_LOCATOR;
 ukey->created = time_msec();
 memset(>stats, 0, sizeof ukey->stats);
 ukey->stats.used = used;
@@ -1674,7 +1680,11 @@ static void
 transition_ukey(struct udpif_key *ukey, enum ukey_state dst)
 OVS_REQUIRES(ukey->mutex)
 {
-ovs_assert(dst >= ukey->state);
+if (dst >= ukey->state) {
+VLOG_ABORT("Invalid ukey transition %d->%d (last transitioned from "
+   "thread %u at %s)", ukey->state, dst, ukey->state_thread,
+   ukey->state_where);
+}
 if (ukey->state == dst && dst == UKEY_OPERATIONAL) {
 return;
 }
@@ -1709,6 +1719,8 @@ transition_ukey(struct udpif_key *ukey, enum ukey_state 
dst)
  ds_cstr(), ukey->state, dst);
 ds_destroy();
 }
+ukey->state_thread = ovsthread_id_self();
+ukey->state_where = OVS_SOURCE_LOCATOR;
 }
 
 static bool
@@ -2327,6 +2339,9 @@ revalidate(struct revalidator *revalidator)
 /* The flow is now confirmed to be in the datapath. */
 transition_ukey(ukey, UKEY_OPERATIONAL);
 } else {
+VLOG_INFO("Unexpected ukey transition from state %d "
+  "(last transitioned from thread %u at %s)",
+  ukey->state, ukey->state_thread, ukey->state_where);
 ovs_mutex_unlock(>mutex);
 continue;
 }
-- 
2.11.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/2] revalidator: Avoid assert in transition_ukey().

[Joe Stringer]

There is a case where a flow is dumped from the kernel after the ukey is
already transitioned into an EVICTING/EVICTED/DELETED state, and the
revalidator thread attempts to shift that into UKEY_OPERATIONAL because
it was able to dump the flow from the datapath. This resulted in
triggering the assert in transition_ukey(). Detect this condition and
skip handling the flow (as it's already on its way out).

Users report:
> Program terminated with signal SIGABRT, Aborted.
> raise () from /lib/x86_64-linux-gnu/libc.so.6
> raise () from /lib/x86_64-linux-gnu/libc.so.6
> abort () from /lib/x86_64-linux-gnu/libc.so.6
> ovs_abort_valist
> vlog_abort_valist
> vlog_abort
> ovs_assert_failure
> transition_ukey (ukey=, dst=)
> at ofproto/ofproto-dpif-upcall.c:1674
> revalidate (revalidator=0x1cb36c8) at ofproto/ofproto-dpif-upcall.c:2324
> udpif_revalidator (arg=0x1cb36c8) at ofproto/ofproto-dpif-upcall.c:901
> ovsthread_wrapper (aux_=) at lib/ovs-thread.c:348
> start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
> clone () from /lib/x86_64-linux-gnu/libc.so.6

VMware-BZ: #1857694
Signed-off-by: Joe Stringer 
---
 ofproto/ofproto-dpif-upcall.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 3b28f9a22939..ccf15a3c80b3 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2323,8 +2323,13 @@ revalidate(struct revalidator *revalidator)
 continue;
 }
 
-/* The flow is now confirmed to be in the datapath. */
-transition_ukey(ukey, UKEY_OPERATIONAL);
+if (ukey->state <= UKEY_OPERATIONAL) {
+/* The flow is now confirmed to be in the datapath. */
+transition_ukey(ukey, UKEY_OPERATIONAL);
+} else {
+ovs_mutex_unlock(>mutex);
+continue;
+}
 
 if (!used) {
 used = ukey->created;
-- 
2.11.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.7 0/4] Backport of variable length metaflow field fixes.

[Ben Pfaff]

On Wed, Apr 26, 2017 at 04:18:47PM -0700, Joe Stringer wrote:
> On 21 April 2017 at 09:11, Ben Pfaff  wrote:
> > On Thu, Apr 20, 2017 at 06:58:18PM -0700, Joe Stringer wrote:
> >> On 17 March 2017 at 14:30, Joe Stringer  wrote:
> >> > On 15 March 2017 at 16:01, Joe Stringer  wrote:
> >> >> Commit 04f48a68c428 ("ofp-actions: Fix variable length meta-flow 
> >> >> OXMs."), on
> >> >> branch-2.7 as 9554b03d6ab7, attempted to address incorrect encode and 
> >> >> decode of
> >> >> variable length metaflow fields where the OXM/NXM encoding of the 
> >> >> variable
> >> >> length fields would incorrectly serialize the length. The patch 
> >> >> addresses this
> >> >> by introducing a new per-bridge structure that adds additional metaflow 
> >> >> fields
> >> >> for the variable-length fields on demand when the TLVs are configured 
> >> >> by a
> >> >> controller.
> >> >>
> >> >> Unfortunately, in the original patch there was nothing ensuring that 
> >> >> flows
> >> >> referring to variable length fields would retain valid field references 
> >> >> when
> >> >> controllers reconfigure the TLVs. In practice, this could lead to a 
> >> >> crash of
> >> >> ovs-vswitchd by configuring a TLV field, adding a flow which refers to 
> >> >> it,
> >> >> removing the TLV field, then running some traffic that hit the 
> >> >> configured flow.
> >> >>
> >> >> This series looks to remedy the situation by reference counting the 
> >> >> variable
> >> >> length fields and preventing a controller from reconfiguring TLV fields 
> >> >> when
> >> >> there are active flows whose match or actions refer to the field.
> >> >>
> >> >> This series was applied to master, but given the size of the change and 
> >> >> the
> >> >> minor changes necessary to apply to branch-2.7, I would feel more 
> >> >> confident in
> >> >> backporting it if there was an extra round of review to ensure that 
> >> >> nothing was
> >> >> missed when this series was first applied to master.
> >> >
> >> > One further concern I have with this series is that while it allows us
> >> > to fix bugs in OVS 2.7, it would change some files in
> >> > include/openvswitch/, which I believe indirectly implies that it could
> >> > break the libopenvswitch ABI, which we try not to do within a release
> >> > series:
> >> >
> >> > http://docs.openvswitch.org/en/latest/internals/contributing/libopenvswitch-abi/
> >>
> >> Reporting back, using abipkgdiff from
> >> libabigail[https://sourceware.org/libabigail], I was able to identify
> >> the following ABI breakages from v2.7.0 to branch-2.7 with these
> >> patches applied, details below.
> >>
> >> A bunch of these are libraries only exported through headers in lib/,
> >> which I believe is not considered 'stable' ABI.
> >>
> >> However, there are several that are exported in include/openvswitch:
> >>
> >> ofpacts_pull_openflow_instructions
> >> ofperr_encode_hello
> >> ofputil_decode_flow_stats_request
> >> ofputil_decode_packet_in
> >> ofputil_decode_packet_in_private
> >> ofputil_encode_bundle_msgs
> >> ofputil_pull_ofp11_match
> >>
> >> Now, the shared library is currently something like
> >> 'libopenvswitch-2.so.7.0.0'  (or, when we prepare 2.7.1,
> >> 'libopenvswitch-2.so.7.0.1' unless other changes are made). The
> >> libtool ABI numbering does not appear to have any influence on the
> >> naming of the shared library. It is (1,0,0) for current, revision and
> >> age. I'm suspecting that the right answer to this is to bump current
> >> and age as per 
> >> [https://lists.gnu.org/archive/html/libtool/2009-08/msg00034.html].
> >> When I do this, the ABI appears completely different according to
> >> abipkgdiff due to the different 'current' number.
> >>
> >> I'm not sure if we are supposed to make the ABI versioning number
> >> consistent with our shared library naming, or if it is reasonable for
> >> each OVS release series (eg, 2.7.x) to have independent libtool
> >> versioning numbers.
> >
> > Thank you for looking into this.
> >
> > It sounds like for 2.7.1 we should change the library name from
> > libopenvswitch-2 to libopenvswitch-2.7.1.  For the long term, it sounds
> > like maybe we need to include an extra component of the version in the
> > library name, so that we'd end up with libopenvswitch-X.Y.so.1.0.Z by
> > default.
> >
> > What are your thoughts?
> 
> Thanks for the thoughts Ben, I took this suggestion and proposed it here:
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331503.html
> 
> I'm not sure that I see why 2.7.1 release would require the full
> "2.7.1" number to be included in the name. If we rename it, I think
> that it should become effectively a different library so we could
> reset the libtool "current" number safely.

I think you're right, thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.7 0/4] Backport of variable length metaflow field fixes.

[Joe Stringer]

On 21 April 2017 at 09:11, Ben Pfaff  wrote:
> On Thu, Apr 20, 2017 at 06:58:18PM -0700, Joe Stringer wrote:
>> On 17 March 2017 at 14:30, Joe Stringer  wrote:
>> > On 15 March 2017 at 16:01, Joe Stringer  wrote:
>> >> Commit 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs."), 
>> >> on
>> >> branch-2.7 as 9554b03d6ab7, attempted to address incorrect encode and 
>> >> decode of
>> >> variable length metaflow fields where the OXM/NXM encoding of the variable
>> >> length fields would incorrectly serialize the length. The patch addresses 
>> >> this
>> >> by introducing a new per-bridge structure that adds additional metaflow 
>> >> fields
>> >> for the variable-length fields on demand when the TLVs are configured by a
>> >> controller.
>> >>
>> >> Unfortunately, in the original patch there was nothing ensuring that flows
>> >> referring to variable length fields would retain valid field references 
>> >> when
>> >> controllers reconfigure the TLVs. In practice, this could lead to a crash 
>> >> of
>> >> ovs-vswitchd by configuring a TLV field, adding a flow which refers to it,
>> >> removing the TLV field, then running some traffic that hit the configured 
>> >> flow.
>> >>
>> >> This series looks to remedy the situation by reference counting the 
>> >> variable
>> >> length fields and preventing a controller from reconfiguring TLV fields 
>> >> when
>> >> there are active flows whose match or actions refer to the field.
>> >>
>> >> This series was applied to master, but given the size of the change and 
>> >> the
>> >> minor changes necessary to apply to branch-2.7, I would feel more 
>> >> confident in
>> >> backporting it if there was an extra round of review to ensure that 
>> >> nothing was
>> >> missed when this series was first applied to master.
>> >
>> > One further concern I have with this series is that while it allows us
>> > to fix bugs in OVS 2.7, it would change some files in
>> > include/openvswitch/, which I believe indirectly implies that it could
>> > break the libopenvswitch ABI, which we try not to do within a release
>> > series:
>> >
>> > http://docs.openvswitch.org/en/latest/internals/contributing/libopenvswitch-abi/
>>
>> Reporting back, using abipkgdiff from
>> libabigail[https://sourceware.org/libabigail], I was able to identify
>> the following ABI breakages from v2.7.0 to branch-2.7 with these
>> patches applied, details below.
>>
>> A bunch of these are libraries only exported through headers in lib/,
>> which I believe is not considered 'stable' ABI.
>>
>> However, there are several that are exported in include/openvswitch:
>>
>> ofpacts_pull_openflow_instructions
>> ofperr_encode_hello
>> ofputil_decode_flow_stats_request
>> ofputil_decode_packet_in
>> ofputil_decode_packet_in_private
>> ofputil_encode_bundle_msgs
>> ofputil_pull_ofp11_match
>>
>> Now, the shared library is currently something like
>> 'libopenvswitch-2.so.7.0.0'  (or, when we prepare 2.7.1,
>> 'libopenvswitch-2.so.7.0.1' unless other changes are made). The
>> libtool ABI numbering does not appear to have any influence on the
>> naming of the shared library. It is (1,0,0) for current, revision and
>> age. I'm suspecting that the right answer to this is to bump current
>> and age as per 
>> [https://lists.gnu.org/archive/html/libtool/2009-08/msg00034.html].
>> When I do this, the ABI appears completely different according to
>> abipkgdiff due to the different 'current' number.
>>
>> I'm not sure if we are supposed to make the ABI versioning number
>> consistent with our shared library naming, or if it is reasonable for
>> each OVS release series (eg, 2.7.x) to have independent libtool
>> versioning numbers.
>
> Thank you for looking into this.
>
> It sounds like for 2.7.1 we should change the library name from
> libopenvswitch-2 to libopenvswitch-2.7.1.  For the long term, it sounds
> like maybe we need to include an extra component of the version in the
> library name, so that we'd end up with libopenvswitch-X.Y.so.1.0.Z by
> default.
>
> What are your thoughts?

Thanks for the thoughts Ben, I took this suggestion and proposed it here:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331503.html

I'm not sure that I see why 2.7.1 release would require the full
"2.7.1" number to be included in the name. If we rename it, I think
that it should become effectively a different library so we could
reset the libtool "current" number safely.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [RFC 1/2] libopenvswitch: Rename to libfoo-X.Y.

[Joe Stringer]

The current intent for Open vSwitch is to maintain libopenvswitch ABI
stability for minor versions, for example each release within the 2.7.z
series. According to the following documentation, no changes to exported
headers should be made.

http://docs.openvswitch.org/en/latest/internals/contributing/libopenvswitch-abi/

However, it is occasionally necessary to make changes to
{include/openvswitch,lib}/*.h headers to fix issues within a given
release series. The current libtool tagging mechanism in the build
system does not allow for this without creating a conflict between the
libtool 'current' version and the next minor release of OVS.

This patch modifies libopenvswitch build to include the MAJOR.MINOR
release version in the libX name, and include the libtool CURRENT and
OVS MICRO release in the libtool versioning tags to indicate library
stability. The resulting format is "libfoo-X.Y.so.CURRENT.0.Z" for OVS
release "X.Y.Z".

Developers should still attempt to avoid introducing ABI-breaking changes
within a particular OVS-X.Y release series, but if this is not possible
this patch introduced a mechanism to allow an ABI-breaking fix to be
introduced. In such a case, developers may update the libtool CURRENT
version to indicate this breakage to library users.

Signed-off-by: Joe Stringer 
---
 .../internals/contributing/libopenvswitch-abi.rst| 16 +---
 m4/openvswitch.m4|  6 +++---
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/internals/contributing/libopenvswitch-abi.rst 
b/Documentation/internals/contributing/libopenvswitch-abi.rst
index 845d90898859..8fa24db964cd 100644
--- a/Documentation/internals/contributing/libopenvswitch-abi.rst
+++ b/Documentation/internals/contributing/libopenvswitch-abi.rst
@@ -87,16 +87,18 @@ ABI Policy
 --
 
 Open vSwitch will export the ABI version at the time of release, such that the
-library name will be the major version, and the rest of the release version
-information will be conveyed with a libtool interface version.
+library name will be the major.minor version, and the rest of the release
+version information will be conveyed with a libtool interface version.
 
 The intent is for Open vSwitch to maintain an ABI stability for each minor
 revision only (so that Open vSwitch release 2.5 carries a guarantee for all
-2.5.ZZ micro-releases).  This means that any porting effort to stable branches
-must take not to disrupt the existing ABI.  Each new 'minor-level' release
-bumps the libtool 'current' version, which informs the linker of a backwards
-incompatible interface, signaling that libraries exposed by Open vSwitch 2.6
-will not maintain ABI stability with Open vSwitch 2.5.
+2.5.ZZ micro-releases). This means that any porting effort to stable branches
+must take not to disrupt the existing ABI.
+
+In the event that a bug must be fixed in a backwards-incompatible way,
+developers must bump the libtool 'current' version to inform the linker of the
+ABI breakage. This will signal that libraries exposed by the subsequent release
+will not maintain ABI stability with the previous version.
 
 Coding
 ---
diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
index 48892f9f720e..648750ab5ad9 100644
--- a/m4/openvswitch.m4
+++ b/m4/openvswitch.m4
@@ -614,9 +614,9 @@ AC_DEFUN([OVS_LIBTOOL_VERSIONS],
   OVS_MAJOR=`echo "$PACKAGE_VERSION" | sed -e 's/[[.]].*//'`
   OVS_MINOR=`echo "$PACKAGE_VERSION" | sed -e "s/^$OVS_MAJOR//" -e 's/^.//' -e 
's/[[.]].*//'`
   OVS_MICRO=`echo "$PACKAGE_VERSION" | sed -e "s/^$OVS_MAJOR.$OVS_MINOR//" -e 
's/^.//' -e 's/[[^0-9]].*//'`
-  OVS_LT_RELINFO="-release $OVS_MAJOR"
-  OVS_LT_VERINFO="-version-info $OVS_MINOR:$OVS_MICRO"
+  OVS_LT_RELINFO="-release $OVS_MAJOR.$OVS_MINOR"
+  OVS_LT_VERINFO="-version-info $LT_CURRENT:$OVS_MICRO"
   OVS_LTINFO="$OVS_LT_RELINFO $OVS_LT_VERINFO"
-  AC_MSG_RESULT([libX-$OVS_MAJOR.so.$OVS_MINOR.0.$OVS_MICRO)])
+  AC_MSG_RESULT([libX-$OVS_MAJOR.$OVS_MINOR.so.$LT_CURRENT.0.$OVS_MICRO)])
   AC_SUBST(OVS_LTINFO)
 ])
-- 
2.11.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [RFC 0/2] Rename libopenvswitch to include MAJOR.MINOR number.

[Joe Stringer]

I have recently been looking into backporting a fix for variable-length
metadata fields from master to branch-2.7. Unfortunately, the fix makes several
changes to libopenvswitch APIs, in particular some files under
include/openvswitch/*.h and lib/*.h. From the perspective of the
openvswitch-dev packages, these are visible in
/usr/include/openvswitch/{,lib/}*.h.

This is the series:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329811.html

To backport these changes, they would introduce a breakage to the
libopenvswitch ABI, in contrary to the policy outlined here:

http://docs.openvswitch.org/en/latest/internals/contributing/libopenvswitch-abi/

Normally, library developers may advertise ABI-breaking changes by including
libtool tags in their .so files, where the "current" number indicates that the
interface changed in a backwards-incompatible manner. OVS currently maintains
this "current" number as the same as the minor release number in an OVS
release, for example "Y" in the OVS X.Y.Z release.

We have a few options:
* Don't backport the fix. The issue will exist, unfixed, in v2.7.
* Backport the fix and ignore the ABI breakage. This is not nice for users, as
  they may update their OVS packages and then begin to observe weird behaviour
  in programs that dynamically link against those libraries.
* Backport the fix and modify current, for example for 2.7 to use "8" as the
  "current" number. This would however conflict with the upcoming 2.8 release,
  so the "current" number would need to be modified to something else for that
  release. Result for users of libopenvswitch would be that the programs fail
  to dynamically load, because the libtool library version indicates that the
  newly updated OVS library is incompatible with the version that their
  application was compiled against. This is nicer than the previous option.
* Attempt to find an alternative workaround, for instance by developing a new
  set of patches which disable functionality that triggers the bug, or by
  attempting to fix the underlying issue in a fundamentally different manner
  from the way it is fixed on master. This includes risk as the changes will
  not receive pre-release testing through developers running the code from the
  master branch.

In the past, developers have not paid particularly close attention to changes
to lib/*.h header files. Bugfixes which change these have been applied to
master (this is OK), then backported to branches 2.6 and 2.7 without attempting
to avoid changes to the header files (this is in contrary to the policy above).
As such, there has been ABI breakage between v2.6.0...branch-2.6, and
v2.7.0...branch-2.7.

In this particular case it appears that if the issues are to be fixed on v2.7,
then changes to include/openvswitch/*.h are necessary, and therefore we should
export this ABI breakage through the libtool "current" number. There is
currently no method to do this cleanly, as the libtool "current" number is tied
to the OVS minor release number.

This series proposes to change the name of the libopenvswitch-X.so.Y.0.Z
library to become libopenvswitch-X.Y.so.CURRENT.0.Z, where X, Y, and Z
correspond to the OVS release number, for example 2.7.0; and CURRENT is the
libtool current number, which would begin as 0 for every libopenvswitch-X.Y
release, and would be incremented whenever backwards-incompatible changes are
introduced. The policy would remain that we make first efforts to avoid
changing the "current" libtool number within an OVS release series; and if this
is not viable or possible, developers may submit ABI-breaking changes along
with an update to this "current" libtool number.

If this is reasonable, I would also seek to backport this to branch-2.7.

I don't have a good understanding of the impact of such a change on
libopenvswitch users, so I would welcome anyone building applications upon this
to voice their feedback.

TL;DR: Is it reasonable to change the libopenvswitch library name to include
MAJOR.MINOR release number?

Joe Stringer (2):
  libopenvswitch: Rename to libfoo-X.Y.
  configure: Reset libtool CURRENT version.

 .../internals/contributing/libopenvswitch-abi.rst| 16 +---
 configure.ac |  2 +-
 m4/openvswitch.m4|  6 +++---
 3 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.11.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 6/6] Documentation: Update DPDK doc with Keepalive feature.

[Bhanuprakash Bodireddy]

OvS DPDK Keepalive feature is aimed at achieving Fastpath Service Assurance
in OVS-DPDK deployments. It adds support for monitoring the packet
processing cores(PMD thread cores) by dispatching heartbeats at regular
intervals. Incase of heartbeat misses the failure shall be detected &
reported to higher level fault management systems/frameworks.

The implementation uses POSIX shared memory object for storing the
events that will be periodically read by monitoring framework. keep-alive
feature can be enabled through below OVSDB settings.

keepalive=true
   - Keepalive feature is disabled by default.

keepalive-interval="50"
   - Timer interval in milliseconds for monitoring the packet
 processing cores.

keepalive-shm-name="/dpdk_keepalive_shm_name"
   - Shared memory block name where the events shall be updated.

When KA is enabled, 'ovs-keepalive' thread shall be spawned that wakes
up at regular intervals to update the timestamp and status of pmd cores
in shared memory region.

An external monitoring framework like collectd(with dpdk plugin support)
can read the core status updates from shared memory. When the core state
is updated, the collectd shall relay the status to ceilometer service
running in the controller. Below is the high level overview of deployment
model.

   Compute NodeControllerCompute Node

Collectd  <--> Ceilometer <>   Collectd

OvS DPDK   OvS DPDK

   +-+
   | VM  |
   +--+--+
  \---+---/
  |
   +--+---+   ++--+ +--+---+
   | OVS  |-> |dpdkevents plugin  | --> |   collectd   |
   +--+---+   ++--+ +--+---+
   |
 +--+-+ +---++ |
 | Ceilometer | <-- | collectd ceilometer plugin |  <---
 +--+-+ +---++

Performance impact:
No noticeable performance or latency impact is observed with
KA feature enabled. The tests were run with 100ms KA interval
and latency is (Min:134,710ns, Avg:173,005ns, Max:1,504,670ns)
for Phy2Phy loopback test case with 100 unique streams.

Signed-off-by: Bhanuprakash Bodireddy 
---
 Documentation/howto/dpdk.rst | 93 
 1 file changed, 93 insertions(+)

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index dc63f7d..e482166 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -400,6 +400,99 @@ If ``N`` is set to 1, an insertion will be performed for 
every flow. If set to
 
 For more information on the EMC refer to :doc:`/intro/install/dpdk` .
 
+.. _dpdk_keepalive:
+
+KeepAlive
+-
+
+OvS KeepAlive(KA) feature is disabled by default. To enable KA feature::
+
+$ ovs-vsctl --no-wait set Open_vSwitch . other_config:keepalive=true
+
+The default timer interval for monitoring packet processing cores is 100ms.
+To set a different timer value, run::
+
+$ ovs-vsctl --no-wait set Open_vSwitch . \
+other_config:keepalive-interval="50"
+
+The events comprise of core states and the last seen timestamps. The events
+are written in to shared memory region ``/dev/shm/dpdk_keepalive_shm_name``.
+To write in to a different shared memory region, run::
+
+$ ovs-vsctl --no-wait set Open_vSwitch . \
+other_config:keepalive-shm-name="/"
+
+The events in the shared memory block can be read by external monitoring
+framework (or) applications. `collectd `__ has built-in
+support for DPDK and provides a `dpdkevents` plugin that can be enabled to
+relay the datapath core status to OpenStack service `Ceilometer
+`__.
+
+To install and configure `collectd`, run::
+
+# Clone collectd from Git repository
+$ git clone https://github.com/collectd/collectd.git
+
+# configure and install collectd
+$ cd collectd
+$ ./build.sh
+$ ./configure --enable-syslog --enable-logfile --with-libdpdk=/usr
+$ make
+$ make install
+
+`collectd` is installed in ``/opt/collectd`` by default. Edit the configuration
+file in ``/opt/collectd/etc/collectd.conf`` to enable logfile, dpdkevents
+and csv plugin::
+
+   LoadPlugin logfile
+   
+   LogLevel debug
+   File "/var/log/collectd/collectd.log"
+   Timestamp true
+   PrintSeverity false
+   
+
+   
+   LogLevel info
+   
+
+Enable `dpdkevents` plugin and update the plugindetails as below::
+
+   LoadPlugin dpdkevents
+
+   
+ 
+   Coremask "0x2"
+   MemoryChannels "4"
+   ProcessType "secondary"
+   FilePrefix "rte"
+ 
+ 
+   SendEventsOnUpdate true
+   LCoreMask "0xf"
+   KeepAliveShmName "/dpdk_keepalive_shm_name"
+   SendNotification True
+  
+   
+
+``LCoreMask`` should be set to the PMD cores that were earlier 

[ovs-dev] [PATCH v2 5/6] vswitch.xml: Add keepalive support.

[Bhanuprakash Bodireddy]

Add support for keepalive functionality to DPDK datapath. By default,
the keepalive is OFF and can be enabled/disabled either at start time.

For eg:
  To enable keepalive feature.
  'ovs-vsctl --no-wait set Open_vSwitch . other_config:keepalive=true'

  To set timer interval of 50ms for monitoring packet processing cores;
  'ovs-vsctl --no-wait set Open_vSwitch . \
  other_config:keepalive-interval="50"

  To set shared memory block name where the events shall be updated
  'ovs-vsctl --no-wait set Open_vSwitch .
   other_config:keepalive-shm-name="/dpdk_keepalive_shm_name"'

Signed-off-by: Bhanuprakash Bodireddy 
---
 vswitchd/vswitch.xml | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 02980b1..458148c 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -283,6 +283,44 @@
 
   
 
+  
+
+  Set this value to true to enable keepalive feature.
+
+
+  The default value is false. Changing this value requires
+  restarting the daemon.
+
+
+  If this value is false at startup, keepalive thread
+  shall not be spawned.
+
+  
+
+  
+
+  Specifies the keepalive interval value.
+
+
+  If not specified, this will be set to 100 milliseconds (default
+  value). Changing this value requires restarting the daemon.
+
+  
+
+  
+
+  Specifies the keepalive shared memory block name.
+
+
+  If not specified, shared memory block named "keepalive_shm_name"
+  (default name) is created. Changing this value requires restarting
+  the daemon.
+
+  
+
   
 
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 4/6] netdev-dpdk: Add support for keepalive functionality.

[Bhanuprakash Bodireddy]

This commit initializes the keepalive subsystem and spawns keepalive
thread that wakes up at regular intervals to update the timestamp and
status of pmd cores in shared memory.

This patch implements POSIX shared memory object for storing the events
that will be read by monitoring framework. Also implements APIs to read
the keepalive settings from OVSDB.

Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/netdev-dpdk.c | 220 +-
 1 file changed, 219 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ddc651b..b9c56ae 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -22,6 +22,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include 
 #include 
@@ -32,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "dirs.h"
 #include "dp-packet.h"
@@ -48,8 +51,9 @@
 #include "ovs-numa.h"
 #include "ovs-thread.h"
 #include "ovs-rcu.h"
-#include "packets.h"
 #include "openvswitch/shash.h"
+#include "packets.h"
+#include "process.h"
 #include "smap.h"
 #include "sset.h"
 #include "unaligned.h"
@@ -402,6 +406,37 @@ struct netdev_rxq_dpdk {
 int port_id;
 };
 
+/*
+ * OVS Shared Memory structure
+ *
+ * keepalive states explained:
+ *
+ *   State   Meaning
+ *--
+ * RTE_KA_STATE_ALIVE Core is Alive
+ * RTE_KA_STATE_MISSING   First heartbeat miss
+ * RTE_KA_STATE_DEAD  Second heartbeat miss
+ * RTE_KA_STATE_GONE  Three or more heatbeat misses
+ * RTE_KA_STATE_DOZINGIdle state
+ * RTE_KA_STATE_SLEEP Sleep state
+ *
+ * The information in the shared memory block will be read by collectd.
+ * */
+struct dpdk_keepalive_shm {
+/*
+ * Relayed status of each core.
+ * UNUSED[0], ALIVE[1], DEAD[2], GONE[3], MISSING[4], DOZING[5], SLEEP[6]
+ **/
+enum rte_keepalive_state core_state[RTE_KEEPALIVE_MAXCORES];
+
+/* Last seen timestamp of the core */
+uint64_t core_last_seen_times[RTE_KEEPALIVE_MAXCORES];
+
+/* Store pmd thread tid */
+pid_t thread_id[RTE_KEEPALIVE_MAXCORES];
+};
+
+static struct dpdk_keepalive_shm *ka_shm;
 static int netdev_dpdk_class_init(void);
 static int netdev_dpdk_vhost_class_init(void);
 
@@ -586,6 +621,183 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
 return 0;
 }
 
+void
+dpdk_ka_get_tid(unsigned core_idx)
+{
+uint32_t tid = rte_sys_gettid();
+
+if (dpdk_is_ka_enabled() && ka_shm) {
+ka_shm->thread_id[core_idx] = tid;
+}
+}
+
+/* Callback function invoked on heartbeat miss.  Verify if it is genuine
+ * heartbeat miss or a false positive and log the message accordingly.
+ */
+static void
+dpdk_failcore_cb(void *ptr_data, const int core_id)
+{
+struct dpdk_keepalive_shm *ka_shm = (struct dpdk_keepalive_shm *)ptr_data;
+
+if (ka_shm) {
+int pstate;
+uint32_t tid = ka_shm->thread_id[core_id];
+int err = get_process_status(tid, );
+
+if (!err) {
+switch (pstate) {
+
+case ACTIVE_STATE:
+VLOG_INFO_RL(,"False positive, pmd tid[%"PRIu32"] alive\n",
+  tid);
+break;
+case STOPPED_STATE:
+case TRACED_STATE:
+case DEFUNC_STATE:
+case UNINTERRUPTIBLE_SLEEP_STATE:
+VLOG_WARN_RL(,
+"PMD tid[%"PRIu32"] on core[%d] is unresponsive\n",
+tid, core_id);
+break;
+default:
+VLOG_DBG("%s: The process state: %d\n", __FUNCTION__, pstate);
+OVS_NOT_REACHED();
+}
+}
+}
+}
+
+/* Relay the core state to external monitoring application(collectd).
+ *
+ * This function shall be invoked periodically to write the core status and
+ * last seen timestamp of the cores in to shared memory block. The information
+ * shall be read by external monitoring framework periodically for change in
+ * core state and will be relayed appropriately to OpenStack service.
+ *
+ */
+static void
+dpdk_ka_relay_core_state(void *ptr_data, const int core_id,
+   const enum rte_keepalive_state core_state, uint64_t last_alive)
+{
+struct dpdk_keepalive_shm *ka_shm = (struct dpdk_keepalive_shm *)ptr_data;
+
+if (!ka_shm) {
+VLOG_ERR_RL(, "KeepAlive: Invalid shared memory block\n");
+return;
+}
+
+VLOG_DBG_RL(,
+   "TS(%lu):CORE%d, old state:%d, current_state:%d\n",
+   (unsigned long)time(NULL),core_id,ka_shm->core_state[core_id],
+   core_state);
+
+switch (core_state) {
+case RTE_KA_STATE_ALIVE:
+case RTE_KA_STATE_MISSING:
+ka_shm->core_state[core_id] = RTE_KA_STATE_ALIVE;
+ka_shm->core_last_seen_times[core_id] = last_alive;
+break;
+case RTE_KA_STATE_DOZING:
+case RTE_KA_STATE_SLEEP:
+case RTE_KA_STATE_DEAD:
+case 

[ovs-dev] [PATCH v2 3/6] dpif-netdev: Register packet processing cores for keepalive.

[Bhanuprakash Bodireddy]

This commit registers the packet processing cores for keepalive
monitoring. Also the pmd threads respond to heartbeats by marking
themselves alive. When the pmd thread is teared down due to datapath
reconfiguration the core state is marked as 'sleep'.

Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/dpif-netdev.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 41d0836..5327703 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3249,6 +3249,14 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) 
OVS_REQUIRES(dp->port_mutex)
 }
 
 static void
+ka_register_core(unsigned core_id OVS_UNUSED)
+{
+#ifdef DPDK_NETDEV
+dpdk_ka_register_core(core_id);
+#endif
+}
+
+static void
 reconfigure_pmd_threads(struct dp_netdev *dp)
 OVS_REQUIRES(dp->port_mutex)
 {
@@ -3296,6 +3304,9 @@ reconfigure_pmd_threads(struct dp_netdev *dp)
 dp_netdev_configure_pmd(pmd, dp, core->core_id, core->numa_id);
 
 pmd->thread = ovs_thread_create("pmd", pmd_thread_main, pmd);
+
+/* Register core for KeepAlive detection. */
+ka_register_core(core->core_id);
 }
 
 /* Log the number of pmd threads per numa node. */
@@ -3664,6 +3675,9 @@ pmd_thread_main(void *f_)
 ovs_numa_thread_setaffinity_core(pmd->core_id);
 dpdk_set_lcore_id(pmd->core_id);
 poll_cnt = pmd_load_queues_and_ports(pmd, _list);
+
+/* Store the pmd thread_id in shared memory. */
+dpdk_ka_get_tid(pmd->core_id);
 reload:
 emc_cache_init(>flow_cache);
 
@@ -3688,6 +3702,9 @@ reload:
poll_list[i].port_no);
 }
 
+/* Mark packet processing core alive if KeepAlive is enabled. */
+ka_mark_core_alive();
+
 if (lc++ > 1024) {
 bool reload;
 
@@ -3718,6 +3735,8 @@ reload:
 goto reload;
 }
 
+ka_mark_core_sleep();
+
 free(poll_list);
 pmd_free_cached_ports(pmd);
 return NULL;
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/6] dpdk: Add helper functions for DPDK keepalive.

[Bhanuprakash Bodireddy]

Introduce helper functions in 'dpdk' module that are needed for
keepalive functionality. Also add dummy functions in 'dpdk-stub' module
that are needed when DPDK is not available.

Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/dpdk-stub.c   | 30 ++
 lib/dpdk.c| 92 +++
 lib/dpdk.h| 11 ++-
 lib/netdev-dpdk.h |  4 +++
 4 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c
index daef729..2392273 100644
--- a/lib/dpdk-stub.c
+++ b/lib/dpdk-stub.c
@@ -48,3 +48,33 @@ dpdk_get_vhost_sock_dir(void)
 {
 return NULL;
 }
+
+void
+dpdk_ka_register_core(unsigned core_id OVS_UNUSED)
+{
+/* Nothing */
+}
+
+void
+dpdk_ka_get_tid(unsigned core_idx OVS_UNUSED)
+{
+/* Nothing */
+}
+
+bool
+dpdk_is_ka_enabled(void)
+{
+return false;
+}
+
+void
+ka_mark_core_alive(void)
+{
+/* Nothing */
+}
+
+void
+ka_mark_core_sleep(void)
+{
+/* Nothing */
+}
diff --git a/lib/dpdk.c b/lib/dpdk.c
index 8da6c32..d673e48 100644
--- a/lib/dpdk.c
+++ b/lib/dpdk.c
@@ -15,6 +15,7 @@
  */
 
 #include 
+#include 
 #include "dpdk.h"
 
 #include 
@@ -22,6 +23,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #ifdef DPDK_PDUMP
@@ -42,6 +44,10 @@ static FILE *log_stream = NULL;   /* Stream for DPDK log 
redirection */
 
 static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
 
+bool keepalive_enable = false;/* KeepAlive disabled by default */
+static uint32_t keepalive_timer_interval;  /* keepalive timer interval */
+static const char *keepalive_shm_blk = NULL;
+
 static int
 process_vhost_flags(char *flag, const char *default_val, int size,
 const struct smap *ovs_other_config,
@@ -67,6 +73,36 @@ process_vhost_flags(char *flag, const char *default_val, int 
size,
 return changed;
 }
 
+/* Retrieve and return the keepalive timer interval from OVSDB. */
+static uint32_t
+get_ka_timer_interval(const struct smap *ovs_other_config)
+{
+#define OVS_KEEPALIVE_TIMEOUT 100/* Default timeout set to 100ms */
+uint32_t ka_interval;
+
+/* Timer granularity in milliseconds
+ * Defaults to OVS_KEEPALIVE_TIMEOUT(ms) if not set */
+ka_interval = smap_get_int(ovs_other_config, "keepalive-interval",
+  OVS_KEEPALIVE_TIMEOUT);
+
+VLOG_INFO("KeepAlive timer interval set to %"PRIu32" (ms)\n", ka_interval);
+return ka_interval;
+}
+
+static const char *
+get_ka_shm_block(const struct smap *ovs_other_config)
+{
+/* Shared mem block. */
+#define OVS_KEEPALIVE_SHM_NAME /dpdk_keepalive_shm_name
+keepalive_shm_blk = smap_get(ovs_other_config, "keepalive-shm-name");
+if (!keepalive_shm_blk) {
+keepalive_shm_blk = OVS_STRINGIZE(OVS_KEEPALIVE_SHM_NAME);
+}
+
+VLOG_INFO("KeepAlive shared memory block: %s\n", keepalive_shm_blk);
+return keepalive_shm_blk;
+}
+
 static char **
 grow_argv(char ***argv, size_t cur_siz, size_t grow_by)
 {
@@ -432,6 +468,14 @@ dpdk_init__(const struct smap *ovs_other_config)
 /* We are called from the main thread here */
 RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID;
 
+if (smap_get_bool(ovs_other_config, "keepalive", false)) {
+keepalive_enable = true;
+VLOG_INFO("OvS-DPDK KeepAlive enabled\n");
+
+keepalive_timer_interval = get_ka_timer_interval(ovs_other_config);
+keepalive_shm_blk = get_ka_shm_block(ovs_other_config);
+}
+
 #ifdef DPDK_PDUMP
 VLOG_INFO("DPDK pdump packet capture enabled");
 err = rte_pdump_init(ovs_rundir());
@@ -489,3 +533,51 @@ dpdk_set_lcore_id(unsigned cpu)
 ovs_assert(cpu != NON_PMD_CORE_ID);
 RTE_PER_LCORE(_lcore_id) = cpu;
 }
+
+/* Return 'true' if KA enabled, otherwise 'false'. */
+inline bool
+dpdk_is_ka_enabled(void)
+{
+return keepalive_enable;
+}
+
+/* Return the Keepalive timer interval. */
+uint32_t
+dpdk_get_ka_interval(void)
+{
+return keepalive_timer_interval;
+}
+
+/* Return the Keepalive shared memory block name. */
+const char *
+dpdk_get_ka_shm(void)
+{
+return keepalive_shm_blk;
+}
+
+/* Register Packet processing core 'core_id' for liveness checks. */
+void
+dpdk_ka_register_core(unsigned core_id)
+{
+if (dpdk_is_ka_enabled()) {
+rte_keepalive_register_core(rte_global_keepalive_info, core_id);
+}
+}
+
+/* Mark Packet processing core alive. */
+void
+ka_mark_core_alive(void)
+{
+if (dpdk_is_ka_enabled()) {
+rte_keepalive_mark_alive(rte_global_keepalive_info);
+}
+}
+
+/* Mark packet processing core as idle. */
+void
+ka_mark_core_sleep(void)
+{
+if (dpdk_is_ka_enabled()) {
+rte_keepalive_mark_sleep(rte_global_keepalive_info);
+}
+}
diff --git a/lib/dpdk.h b/lib/dpdk.h
index 673a1f1..913c9f9 100644
--- a/lib/dpdk.h
+++ b/lib/dpdk.h
@@ -26,6 +26,9 @@
 
 #else
 
+#include 
+#include 
+
 #define NON_PMD_CORE_ID UINT32_MAX
 
 #endif /* DPDK_NETDEV */
@@ -35,5 +38,11 @@ struct 

[ovs-dev] [PATCH v2 2/6] process: Retrieve process status.

[Bhanuprakash Bodireddy]

Implement function to retrieve the process status. This will be used by
Keepalive monitoring thread for detecting false alarms.

Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/process.c | 60 +++
 lib/process.h | 10 ++
 2 files changed, 70 insertions(+)

diff --git a/lib/process.c b/lib/process.c
index e9d0ba9..e35e643 100644
--- a/lib/process.c
+++ b/lib/process.c
@@ -50,6 +50,20 @@ struct process {
 int status;
 };
 
+struct pstate2Num {
+char *tidState;
+int num;
+};
+
+const struct pstate2Num pstate_map[] = {
+{ "S", STOPPED_STATE },
+{ "R", ACTIVE_STATE },
+{ "t", TRACED_STATE },
+{ "Z", DEFUNC_STATE },
+{ "D", UNINTERRUPTIBLE_SLEEP_STATE },
+{ "NULL", UNUSED_STATE },
+};
+
 /* Pipe used to signal child termination. */
 static int fds[2];
 
@@ -390,6 +404,52 @@ process_run(void)
 #endif
 }
 
+int
+get_process_status(int tid, int *pstate)
+{
+#ifdef __linux__
+static char process_name[20];
+FILE *stream;
+char line[75];
+char Name[15], value[5], status[20];
+int i, ln;
+
+snprintf(process_name, sizeof(process_name),
+ "/proc/%d/status", tid);
+stream = fopen(process_name, "r");
+if (stream == NULL) {
+VLOG_WARN_ONCE("%s: open failed: %s", process_name,
+ovs_strerror(errno));
+return errno;
+}
+
+ln=0;
+while (fgets(line, sizeof line, stream)) {
+if (!ovs_scan(line,
+  "%6s %2s %14s\n",
+   Name, value, status)) {
+VLOG_WARN_ONCE("%s: could not parse line %d: %s",
+process_name, ln, line);
+continue;
+}
+if (!strcmp(Name, "State:")) {
+for (i=0; pstate_map[i].tidState != NULL; i++) {
+if (strcmp(pstate_map[i].tidState, value) == 0) {
+VLOG_INFO("The state is %s, status is %d\n",
+pstate_map[i].tidState, pstate_map[i].num);
+*pstate = pstate_map[i].num;
+break;
+}
+}
+break;
+}
+ln++;
+   }
+   return 0;
+#else
+   return ENOSYS;
+#endif
+}
 
 /* Causes the next call to poll_block() to wake up when process 'p' has
  * exited. */
diff --git a/lib/process.h b/lib/process.h
index 3feac7e..8a5513e 100644
--- a/lib/process.h
+++ b/lib/process.h
@@ -20,6 +20,15 @@
 #include 
 #include 
 
+enum process_states {
+UNUSED_STATE,
+STOPPED_STATE,
+ACTIVE_STATE,
+TRACED_STATE,
+DEFUNC_STATE,
+UNINTERRUPTIBLE_SLEEP_STATE
+};
+
 struct process;
 
 /* Starting and monitoring subprocesses.
@@ -38,6 +47,7 @@ bool process_exited(struct process *);
 int process_status(const struct process *);
 void process_run(void);
 void process_wait(struct process *);
+int get_process_status(int, int *);
 
 /* These functions are thread-safe. */
 char *process_status_msg(int);
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 0/6] Add OVS DPDK keep-alive functionality

[Bhanuprakash Bodireddy]

This patch is aimed at achieving Fastpath Service Assurance in
OVS-DPDK deployments. This commit adds support for monitoring the packet
processing cores(pmd thread cores) by dispatching heartbeats at regular
intervals. Incase of heartbeat miss the failure shall be detected &
reported to higher level fault management systems/frameworks.

The implementation uses POSIX shared memory object for storing the
events that will be read by monitoring framework. keep-alive feature
can be enabled through below OVSDB settings.

keepalive=true
   - Keepalive feature is disabled by default

keepalive-interval="50"
   - Timer interval in milliseconds for monitoring the packet
 processing cores.

keepalive-shm-name="/dpdk_keepalive_shm_name"
   - Shared memory block name where the events shall be updated.

When KA is enabled, 'ovs-keepalive' thread shall be spawned that wakes
up at regular intervals to update the timestamp and status of pmd cores
in shared memory region.

An external monitoring framework like collectd(with dpdk plugin support)
can read the status updates from shared memory. On a missing heartbeat,
the collectd shall relay the status to ceilometer service running in the
controller. Below is the high level overview of deployment model.

Compute Node   Controller

 Collectd  <-> Ceilometer

 OVS DPDK

   +-+
   | VM  |
   +--+--+
  \---+---/
  |
   +--+---+   ++--+ +--+---+
   | OVS  |-> |dpdkevents plugin  | --> |   collectd   |
   +--+---+   ++--+ +--+---+

 +--+-+ +---++
 | Ceilometer | <-- | collectd ceilometer plugin |  <
 +--+-+ +---++

v1->v2:
- Sort the patches in the order leaving no dead code behind.
- Remove xusleep() implementation and instead used usleep().
- Replace '_WIN32' with '__linux__' in get_process_status().
- Add comments for different Keepalive states.
- Remove semaphore and all the logic associated with it.
- Fix the documentation as suggested.
- Fix and added few appropriate comments to KA helper functions.
- Add latency stats details in the commit log for future reference.

Bhanuprakash Bodireddy (6):
  dpdk: Add helper functions for DPDK keepalive.
  process: Retrieve process status.
  dpif-netdev: Register packet processing cores for keepalive.
  netdev-dpdk: Add support for keepalive functionality.
  vswitch.xml: Add keepalive support.
  Documentation: Update DPDK doc with Keepalive feature.

 Documentation/howto/dpdk.rst |  93 ++
 lib/dpdk-stub.c  |  30 ++
 lib/dpdk.c   |  92 ++
 lib/dpdk.h   |  11 ++-
 lib/dpif-netdev.c|  19 
 lib/netdev-dpdk.c| 220 ++-
 lib/netdev-dpdk.h|   4 +
 lib/process.c|  60 
 lib/process.h|  10 ++
 vswitchd/vswitch.xml |  38 
 10 files changed, 575 insertions(+), 2 deletions(-)

-- 
2.4.11

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [v2] vswitchd: Add --cleanup option to the 'appctl exit' command

[Andy Zhou]

'appctl exit' stops the running vswitchd daemon, without releasing
the datapath resources (such as bridges and ports) that vswitchd
has created.  This is expected when vswitchd is to be relaunched, to
reduce the perturbation of exiting traffic and connections.

However, when vswitchd is intended to be shutdown permanently, it
is desirable not to leak datapath resources.  In theory, this can be
achieved by removing the corresponding configurations from
OVSDB before shutting down vswitchd. However it is not always
possible in practice. Sometimes it is convenient and robust for
vswitchd to release all datapath resources that it has configured.
Add 'appctl exit --cleanup' option for this use case.

Signed-off-by: Andy Zhou 

---
v1->v2:
remove 'appctl quit', Change to 'appctl exit --cleanup'
Add more details to the commit message.
---
 NEWS   |  1 +
 ofproto/ofproto-dpif.c | 11 +++
 ofproto/ofproto-provider.h |  2 +-
 ofproto/ofproto.c  |  2 +-
 vswitchd/bridge.c  |  4 ++--
 vswitchd/bridge.h  |  4 +++-
 vswitchd/ovs-vswitchd.8.in |  7 +--
 vswitchd/ovs-vswitchd.c| 23 ---
 8 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/NEWS b/NEWS
index ea97d84a2dea..ee50c6660468 100644
--- a/NEWS
+++ b/NEWS
@@ -26,6 +26,7 @@ Post-v2.7.0
  * Bundles now support hashing by just nw_src or nw_dst.
  * The "learn" action now supports a "limit" option (see ovs-ofctl(8)).
  * The port status bit OFPPS_LIVE now reflects link aliveness.
+   - Add --cleanup option to command 'ovs-appctl exit' (see ovs-vswitchd(8)).
 
 v2.7.0 - 21 Feb 2017
 -
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c73c2738c91c..bd2eaa60d36b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -645,7 +645,7 @@ dealloc(struct ofproto *ofproto_)
 }
 
 static void
-close_dpif_backer(struct dpif_backer *backer)
+close_dpif_backer(struct dpif_backer *backer, bool del)
 {
 ovs_assert(backer->refcount > 0);
 
@@ -661,6 +661,9 @@ close_dpif_backer(struct dpif_backer *backer)
 shash_find_and_delete(_dpif_backers, backer->type);
 free(backer->type);
 free(backer->dp_version_string);
+if (del) {
+dpif_delete(backer->dpif);
+}
 dpif_close(backer->dpif);
 free(backer);
 }
@@ -772,7 +775,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
**backerp)
 if (error) {
 VLOG_ERR("failed to listen on datapath of type %s: %s",
  type, ovs_strerror(error));
-close_dpif_backer(backer);
+close_dpif_backer(backer, false);
 return error;
 }
 
@@ -1452,7 +1455,7 @@ add_internal_flows(struct ofproto_dpif *ofproto)
 }
 
 static void
-destruct(struct ofproto *ofproto_)
+destruct(struct ofproto *ofproto_, bool del)
 {
 struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
 struct ofproto_async_msg *am;
@@ -1505,7 +1508,7 @@ destruct(struct ofproto *ofproto_)
 
 seq_destroy(ofproto->ams_seq);
 
-close_dpif_backer(ofproto->backer);
+close_dpif_backer(ofproto->backer, del);
 }
 
 static int
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index b7b12cdfd5f4..ef993d0afc4d 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -828,7 +828,7 @@ struct ofproto_class {
  */
 struct ofproto *(*alloc)(void);
 int (*construct)(struct ofproto *ofproto);
-void (*destruct)(struct ofproto *ofproto);
+void (*destruct)(struct ofproto *ofproto, bool del);
 void (*dealloc)(struct ofproto *ofproto);
 
 /* Performs any periodic activity required by 'ofproto'.  It should:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ca0f3e49bd67..7bc7b7f99d0d 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1651,7 +1651,7 @@ ofproto_destroy(struct ofproto *p, bool del)
 free(usage);
 }
 
-p->ofproto_class->destruct(p);
+p->ofproto_class->destruct(p, del);
 
 /* We should not postpone this because it involves deleting a listening
  * socket which we may want to reopen soon. 'connmgr' may be used by other
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index ebb6249416fa..e741f34f19ec 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -496,14 +496,14 @@ bridge_init(const char *remote)
 }
 
 void
-bridge_exit(void)
+bridge_exit(bool delete_datpath)
 {
 struct bridge *br, *next_br;
 
 if_notifier_destroy(ifnotifier);
 seq_destroy(ifaces_changed);
 HMAP_FOR_EACH_SAFE (br, next_br, node, _bridges) {
-bridge_destroy(br, false);
+bridge_destroy(br, delete_datpath);
 }
 ovsdb_idl_destroy(idl);
 }
diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h
index 3783a21e3b4c..7835611568cf 100644
--- a/vswitchd/bridge.h
+++ b/vswitchd/bridge.h
@@ -16,10 +16,12 @@
 #ifndef VSWITCHD_BRIDGE_H
 #define VSWITCHD_BRIDGE_H 1
 
+#include 
+
 struct simap;
 
 void 

[ovs-dev] [PATCH v3 1/5] datapath: Fixups for MPLS GSO

[Yi-Hung Wei]

This patch backports the following two upstream commits to fix MPLS GSO in
ovs datapath. Starting from upstream commit 48d2ab609b6b ("net: mpls: Fixups
for GSO"), the mpls_gso kernel module relies on the fact that
skb_network_header() points to the mpls header and skb_inner_network_header()
points to the L3 header so that it can derive the length of mpls header
correctly, and the upstream commit updates how ovs datapath marks the skb
header when push and pop mpls. However, the old mpls_gso kernel module
assumes that the skb_network_header() points to the L3 header, and the old
mpls_gso kernel module will misbehave if the ovs datapath marks the
skb_network_header() in the new way since it will treat mpls header as the L3
header.

Because of the functional signature of mpls_gso_segment() does not change,
this backport patch uses the new mpls_hdr() to determine if the kernel that
ovs datapath is compiled with has the new or legacy mpls_gso kernel module.
It has been tested on kernel 4.4 and 4.9.

Upstream commit:
commit 48d2ab609b6bbecb7698487c8579bc40de9d6dfa
Author: David Ahern 
Date:   Wed Aug 24 20:10:44 2016 -0700

net: mpls: Fixups for GSO

As reported by Lennert the MPLS GSO code is failing to properly segment
large packets. There are a couple of problems:

1. the inner protocol is not set so the gso segment functions for inner
   protocol layers are not getting run, and

2  MPLS labels for packets that use the "native" (non-OVS) MPLS code
   are not properly accounted for in mpls_gso_segment.

The MPLS GSO code was added for OVS. It is re-using skb_mac_gso_segment
to call the gso segment functions for the higher layer protocols. That
means skb_mac_gso_segment is called twice -- once with the network
protocol set to MPLS and again with the network protocol set to the
inner protocol.

This patch sets the inner skb protocol addressing item 1 above and sets
the network_header and inner_network_header to mark where the MPLS labels
start and end. The MPLS code in OVS is also updated to set the two
network markers.

>From there the MPLS GSO code uses the difference between the network
header and the inner network header to know the size of the MPLS header
that was pushed. It then pulls the MPLS header, resets the mac_len and
protocol for the inner protocol and then calls skb_mac_gso_segment
to segment the skb.

Afterward the inner protocol segmentation is done the skb protocol
is set to mpls for each segment and the network and mac headers
restored.

Reported-by: Lennert Buytenhek 
Signed-off-by: David Ahern 
Signed-off-by: David S. Miller 

Upstream commit:
commit 85de4a2101acb85c3b1dde465e84596ccca99f2c
Author: Jiri Benc 
Date:   Fri Sep 30 19:08:07 2016 +0200

openvswitch: use mpls_hdr

skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
instead.

Signed-off-by: Jiri Benc 
Acked-by: Pravin B Shelar 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4 |  2 ++
 datapath/actions.c   | 33 +++-
 datapath/linux/compat/include/net/mpls.h | 27 --
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 9f8e30d9b47a..fef4872a4cd7 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -479,6 +479,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
 [OVS_GREP_IFELSE([$KSRC/include/net/dst_cache.h], 
[dst_cache],
  
[OVS_DEFINE([USE_UPSTREAM_TUNNEL])])])])
 
+  OVS_GREP_IFELSE([$KSRC/include/net/mpls.h], [mpls_hdr],
+  [OVS_DEFINE([MPLS_HEADER_IS_L3])])
   OVS_GREP_IFELSE([$KSRC/include/linux/net.h], [sock_create_kern.*net],
   [OVS_DEFINE([HAVE_SOCK_CREATE_KERN_NET])])
   OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [ndo_fill_metadata_dst])
diff --git a/datapath/actions.c b/datapath/actions.c
index 75f17709aec0..9ac02bc8ee2f 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -186,7 +186,7 @@ static void update_ethertype(struct sk_buff *skb, struct 
ethhdr *hdr,
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 const struct ovs_action_push_mpls *mpls)
 {
-   __be32 *new_mpls_lse;
+   struct mpls_shim_hdr *new_mpls_lse;
 
/* Networking stack do not allow simultaneous Tunnel and MPLS GSO. */
if (skb->encapsulation)
@@ -195,20 +195,26 @@ static int push_mpls(struct sk_buff *skb, struct 
sw_flow_key *key,
if (skb_cow_head(skb, MPLS_HLEN) < 0)
return -ENOMEM;
 
+   if (!ovs_skb_get_inner_protocol(skb)) {
+   

Re: [ovs-dev] [PATCH 1/3] datapath: Fixups for MPLS GSO

[Yi-Hung Wei]

On Wed, Apr 26, 2017 at 4:52 AM, Simon Horman
 wrote:
> On Tue, Apr 25, 2017 at 05:45:34PM -0700, Yi-Hung Wei wrote:
>> Hi Simon,
>>
>> Thanks for your review. Please find my relies below.
>> I will sent out v2 based on your review.
>>
>> On Tue, Apr 25, 2017 at 12:43 AM, Simon Horman
>>  wrote:
>> > Hi Yi-Hung,
>> >
>> > thanks for taking on this difficult piece of work and apologies for the
>> > delay in responding.
>> >
>> > On Mon, Apr 03, 2017 at 04:24:36PM -0700, Yi-Hung Wei wrote:
>> >> This commit backports the following upstream commits to fix MPLS GSO in 
>> >> ovs
>> >> datapath. It has been tested on kernel 4.4 and 4.9.
>> >
>> > Thanks also for noting the versions this has been tested against. I expect
>> > there testing against other versions will show some residual problems but
>> > I think that fixing 4.4 and 4.9 is a good step forwards.
>> >
>> > I see that this patch backports 4 upstream patches. I am curious to know
>> > if you considered backporting them individually. That would have made
>> > reviewing a little easier for me.
>> Thanks for your suggestion. I pull two independent patches out of this
>> patch in v2.
>> Commit 48d2ab609b6b("net: mpls: Fixups for GSO") and 85de4a2101ac
>> ("openvswitch: use mpls_hdr") are backported together since I am using the
>> mpls_hdr() in the later commit to backport some logic in the first commit.
>
> Thanks.
>
>> >> Upstream commit:
>> >> commit 48d2ab609b6bbecb7698487c8579bc40de9d6dfa
>> >> Author: David Ahern 
>> >> Date:   Wed Aug 24 20:10:44 2016 -0700
>> >
>> > ...
>> >
>> >> Upstream commit:
>> >> commit f7d49bce8e741e1e6aa14ce4db1b6cea7e4be4e8
>> >> Author: Jiri Benc 
>> >> Date:   Fri Sep 30 19:08:05 2016 +0200
>> >
>> > ...
>> >
>> >> Upstream commit:
>> >> commit 85de4a2101acb85c3b1dde465e84596ccca99f2c
>> >> Author: Jiri Benc 
>> >> Date:   Fri Sep 30 19:08:07 2016 +0200
>> >>
>> >> openvswitch: use mpls_hdr
>> >>
>> >> skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
>> >> instead.
>> >>
>> >> Signed-off-by: Jiri Benc 
>> >> Acked-by: Pravin B Shelar 
>> >> Signed-off-by: David S. Miller 
>> >
>> > ...
>> >
>> >> Upstream commit:
>> >> commit c66549ffd05831abf6cf19ce0571ad868e39
>> >> Author: Jiri Benc 
>> >> Date:   Wed Oct 5 15:01:57 2016 +0200
>> >
>> > ...
>> >
>> >> diff --git a/acinclude.m4 b/acinclude.m4
>> >> index 744d8f89525c..82ca4a28015c 100644
>> >> --- a/acinclude.m4
>> >> +++ b/acinclude.m4
>> >> @@ -479,6 +479,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>> >>  
>> >> [OVS_GREP_IFELSE([$KSRC/include/net/dst_cache.h], [dst_cache],
>> >>   
>> >> [OVS_DEFINE([USE_UPSTREAM_TUNNEL])])])])
>> >>
>> >> +  OVS_GREP_IFELSE([$KSRC/include/linux/mpls.h], [mpls_hdr])
>> >
>> > Should the path be $KSRC/include/net/mpls.h?
>> >
>> > I am looking at 9095e10edd28 ("mpls: move mpls_hdr to a common location")
>> Yes, you're right. Thanks for finding this bug.
>>
>> >
>> >>OVS_GREP_IFELSE([$KSRC/include/linux/net.h], [sock_create_kern.*net],
>> >>[OVS_DEFINE([HAVE_SOCK_CREATE_KERN_NET])])
>> >>OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], 
>> >> [ndo_fill_metadata_dst])
>> >> diff --git a/datapath/actions.c b/datapath/actions.c
>> >> index 06080b24ea5a..ecc5136a3529 100644
>> >> --- a/datapath/actions.c
>> >> +++ b/datapath/actions.c
>> >
>> > ...
>> >
>> >> @@ -169,20 +170,26 @@ static int push_mpls(struct sk_buff *skb, struct 
>> >> sw_flow_key *key,
>> >>   if (skb_cow_head(skb, MPLS_HLEN) < 0)
>> >>   return -ENOMEM;
>> >>
>> >> + if (!ovs_skb_get_inner_protocol(skb)) {
>> >> + skb_set_inner_network_header(skb, skb->mac_len);
>> >> + ovs_skb_set_inner_protocol(skb, skb->protocol);
>> >> + }
>> >> +
>> >>   skb_push(skb, MPLS_HLEN);
>> >>   memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
>> >>   skb->mac_len);
>> >>   skb_reset_mac_header(skb);
>> >> +#ifdef HAVE_MPLS_HDR
>> >> + skb_set_network_header(skb, skb->mac_len);
>> >> +#endif
>> >
>> > It is not clear to me why this call to skb_set_network_header() is
>> > guarded by HAVE_MPLS_HDR here and moreover not elsewhere in this patch.
>>
>> This patch uses HAVE_MPLS_HDR to determine if the kernel that ovs datapath is
>> compiled with has the new or the old mpls_gso kernel module. Because
>> starting from
>> commit 48d2ab609b6b ("net: mpls: Fixups for GSO"), the mpls_gso kernel module
>> relies on the fact that skb_network_header() points to the mpls header and
>> skb_inner_network_header() points to the L3 header so that it can
>> derive the length of
>> mpls header correctly. However, the old mpls_gso kernel module 

[ovs-dev] Compras Para Principiantes

[Otras Maneras De Vender Con Éxito]

Adquiera su Programa Integral de Capacitación Online Ahora

Adquiera uno de nuestros planes online, los cuáles constan de 12 Temas que son 
utilizables durante 3 meses, las 24 hrs del día, las veces que usted así lo 
requiera

Programa Integral de Capacitación Online de Compras

 Programa Integral de Capacitación Online de Ventas

 
- Compras Para Principiantes.
- Estándares, Políticas Y Procedimientos Del Área De Compras.
- Competencias Del Comprador Exitoso.
- La Negociación En Compras De La A A La Z.
- Fundamentos De Administración Y Finanzas Para Compradores.
- Relación Con Proveedores, Certificaciones Y Alianzas.
- El Manejo De Los Inventarios Y Su Relación Con Las Compras.
- Compras De Mro (Mantenimiento, Refacciones Y Operacionales).
- Las 4 Herramientas Computacionales Indispensables Para El Comprador 
Profesional.
- Cómo Hacer Una Planeación Estratégica Para Compras.
- Recomendaciones Expertas Para El Jefe De Compras.
- Lo Que El Director General Debe Saber De Compras Y Sus Compradores - Ventas, 
Mi Profesión.
- Qué Vendo Y En Qué Beneficia A Mi Cliente.
- ¿Y Dónde Están Los Prospectos? Tipo De Clientes, Nichos Y Territorios.
- Venta De Cambaceo.
- Los Secretos De Las Ventas Telefónicas.
- Las Ventas De Piso Y Mostrador.
- Otras Maneras De Vender Con Éxito.
- El Poder De La Persuasión.
- Cómo Negocian Y Ganan Los Campeones De Ventas.
- Cierre Y Manejo De Objeciones.
- Técnica Sensacionales De Servicio Al Cliente.
- Cómo Vender Cuando Nadie Te Compra
 
¿Requiere la información a la Brevedad?
responda este email con la palabra: 
Info - COMPRAS.
centro telefónico: 018002129393 
 ¿Requiere la información a la Brevedad?
responda este email con la palabra: 
Info - VENTAS.
centro telefónico: 018002129393 
 



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] datapath: Delete conntrack entry clashing with an expectation.

[Jarno Rajahalme]

Upstream commit:

commit cf5d70918877c6a6655dc1e92e2ebb661ce904fd
Author: Jarno Rajahalme 
Date:   Fri Apr 14 14:26:38 2017 -0700

openvswitch: Delete conntrack entry clashing with an expectation.

Conntrack helpers do not check for a potentially clashing conntrack
entry when creating a new expectation.  Also, nf_conntrack_in() will
check expectations (via init_conntrack()) only if a conntrack entry
can not be found.  The expectation for a packet which also matches an
existing conntrack entry will not be removed by conntrack, and is
currently handled inconsistently by OVS, as OVS expects the
expectation to be removed when the connection tracking entry matching
that expectation is confirmed.

It should be noted that normally an IP stack would not allow reuse of
a 5-tuple of an old (possibly lingering) connection for a new data
connection, so this is somewhat unlikely corner case.  However, it is
possible that a misbehaving source could cause conntrack entries be
created that could then interfere with new related connections.

Fix this in the OVS module by deleting the clashing conntrack entry
after an expectation has been matched.  This causes the following
nf_conntrack_in() call also find the expectation and remove it when
creating the new conntrack entry, as well as the forthcoming reply
direction packets to match the new related connection instead of the
old clashing conntrack entry.

Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
Reported-by: Yang Song 
Signed-off-by: Jarno Rajahalme 
Acked-by: Joe Stringer 
Signed-off-by: Pablo Neira Ayuso 

Signed-off-by: Jarno Rajahalme 
---
 datapath/conntrack.c | 30 +-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 4c42a48..6f5690a 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -548,10 +548,38 @@ ovs_ct_expect_find(struct net *net, const struct 
nf_conntrack_zone *zone,
   u16 proto, const struct sk_buff *skb)
 {
struct nf_conntrack_tuple tuple;
+   struct nf_conntrack_expect *exp;
 
if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, 
))
return NULL;
-   return __nf_ct_expect_find(net, zone, );
+
+   exp = __nf_ct_expect_find(net, zone, );
+   if (exp) {
+   struct nf_conntrack_tuple_hash *h;
+
+   /* Delete existing conntrack entry, if it clashes with the
+* expectation.  This can happen since conntrack ALGs do not
+* check for clashes between (new) expectations and existing
+* conntrack entries.  nf_conntrack_in() will check the
+* expectations only if a conntrack entry can not be found,
+* which can lead to OVS finding the expectation (here) in the
+* init direction, but which will not be removed by the
+* nf_conntrack_in() call, if a matching conntrack entry is
+* found instead.  In this case all init direction packets
+* would be reported as new related packets, while reply
+* direction packets would be reported as un-related
+* established packets.
+*/
+   h = nf_conntrack_find_get(net, zone, );
+   if (h) {
+   struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
+
+   nf_ct_delete(ct, 0, 0);
+   nf_conntrack_put(>ct_general);
+   }
+   }
+
+   return exp;
 }
 
 /* This replicates logic from nf_conntrack_core.c that is not exported. */
-- 
2.1.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/8] ovsdb-idl: Fix memory leak

[Ben Pfaff]

On Wed, Apr 26, 2017 at 06:21:34PM +0200, Timothy M. Redaelli wrote:
> On 04/24/2017 06:30 PM, Ben Pfaff wrote:
> > On Fri, Apr 07, 2017 at 02:43:42PM -0700, Yi-Hung Wei wrote:
> >> In testcase "simple idl, conditional, multiple clauses in condition - C",
> >> valgrind reports a memory leak with the following call stack.
> >> xmalloc (util.c:112)
> >> resize (hmap.c:100)
> >> ovsdb_idl_condition_clone (ovsdb-idl.c:1075)
> >> ovsdb_idl_set_condition (ovsdb-idl.c:1095)
> >> update_conditions (test-ovsdb.c:2299)
> >> do_idl (test-ovsdb.c:2388)
> >> ovs_cmdl_run_command__ (command-line.c:115)
> >> main (test-ovsdb.c:73)
> >>
> >> Signed-off-by: Yi-Hung Wei 
> > 
> > Thanks, I applied this to master, branch-2.7, and branch-2.6.
> 
> Hi,
> unlucky this commit breaks build on branch-2.6 since the function
> "ovsdb_idl_condition_destroy" was added in commit
> 0164e367f5d8 ("ovsdb-idl: Change interface to conditional monitoring.")
> and it's not available on branch 2.6.

Oops.  Thanks for the report.  For now, I reverted this from branch-2.6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness

[Ben Pfaff]

No, we don't add features to release branches.

On Wed, Apr 26, 2017 at 04:10:36PM +, László Sürü wrote:
> Thanks for the bfd update!
> 
> Just one more question,
> Is the backporting of liveness propagation to OVS 2.7 branch planned also?
> 
> Thanks and regards
> Laszlo
> 
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org] 
> Sent: Friday, April 21, 2017 7:39 PM
> To: László Sürü 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as 
> link aliveness
> 
> Thanks for the revision.
> 
> I still saw the failures, so I spent some time looking at why.  It turned out 
> to be related to the two different database transactions used to configure 
> BFD.  When I combined the two transactions into one, e.g. changed
> 
> AT_CHECK([ovs-vsctl add-br br1 -- \
>   set bridge br1 datapath-type=dummy \
>   other-config:hwaddr=aa:55:aa:56:00:00 -- \
>   add-port br1 p1 -- set Interface p1 type=patch \
>   options:peer=p0 ofport_request=2 -- \
>   add-port br0 p0 -- set Interface p0 type=patch \
>   options:peer=p1 ofport_request=1])
> 
> AT_CHECK([ovs-vsctl \
>   set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 
> -- \
>   set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
> 
> into
> 
> AT_CHECK([ovs-vsctl add-br br1 -- \
>   set bridge br1 datapath-type=dummy \
>   other-config:hwaddr=aa:55:aa:56:00:00 -- \
>   add-port br1 p1 -- set Interface p1 type=patch \
>   options:peer=p0 ofport_request=2 -- \
>   add-port br0 p0 -- set Interface p0 type=patch \
>   options:peer=p1 ofport_request=1 -- \
>   set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 
> -- \
>   set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])
> 
> I no longer saw the failures.
> 
> I made that change in each of the BFD tests.  I also got rid of the use of 
> the GNU grep --no-group-separator option, which BSD doesn't appear to support.
> 
> With those changes, I applied this to master.  Thanks again!
> 
> On Thu, Apr 20, 2017 at 03:41:57PM +, László Sürü wrote:
> > Thanks for the review and comments.
> > All unit tests were passing for me before with 'make check', I haven't seen 
> > failing unit tests.
> > It might have been a temporary misbehavior.
> > 
> > I've applied the proposed changes and successfully rerun the tests 
> > also with 'testsuite' this time based on your attached log (see attachment).
> > Also, I've adapted the tests to the new OF 1.6 test as well.
> > 
> > Please find the modified patched below in this mail.
> > 
> > Thanks and regards
> > Laszlo
> > 
> > -Original Message-
> > From: Ben Pfaff [mailto:b...@ovn.org]
> > Sent: Saturday, April 15, 2017 6:44 AM
> > To: László Sürü 
> > Cc: ovs-dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable 
> > flag as link aliveness
> > 
> > Thanks a lot for the revised patch!  It will be good to get this done 
> > properly.
> > 
> > I'm appending some changes that I suggest folding in to better match the 
> > usual OVS coding style.
> > 
> > However, when I run the testsuite, I get the failures below.  Do you see 
> > these too?  Can you take a look at them?  I'm also attaching the full 
> > testsuite.log in case it helps.
> > 
> > bfd
> > 
> >  22: bfd - liveness propagation - OF1.3  FAILED (bfd.at:847)
> >  23: bfd - liveness propagation - OF1.4  FAILED (bfd.at:920)
> >  24: bfd - liveness propagation - OF1.5  FAILED (bfd.at:993)
> > 
> > ofproto
> > 
> > 890: ofproto - mod-port (OpenFlow 1.6)   FAILED 
> > (ofproto.at:1308)
> > 
> > --8<--cut here-->8--
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 
> > 218e8eb..c82640d 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
> >  bfd_set_netdev(port->bfd, netdev);
> >  }
> >  
> > +/* Set liveness, unless the link is administratively or
> > + * operationally down or link monitoring false */
> > +if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> > +!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
> > +port->may_enable) {
> > +port->up.pp.state |= OFPUTIL_PS_LIVE;
> > +} else {
> > +port->up.pp.state &= ~OFPUTIL_PS_LIVE;
> > +}
> > +
> >  ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
> >   port->lldp, 
> > >up.pp.hw_addr);
> >  
> > @@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
> >  if (ofport->rstp_port) {
> >  rstp_port_set_mac_operational(ofport->rstp_port, 

Re: [ovs-dev] [PATCH 4/8] ovsdb-idl: Fix memory leak

[Timothy M. Redaelli]

On 04/24/2017 06:30 PM, Ben Pfaff wrote:
> On Fri, Apr 07, 2017 at 02:43:42PM -0700, Yi-Hung Wei wrote:
>> In testcase "simple idl, conditional, multiple clauses in condition - C",
>> valgrind reports a memory leak with the following call stack.
>> xmalloc (util.c:112)
>> resize (hmap.c:100)
>> ovsdb_idl_condition_clone (ovsdb-idl.c:1075)
>> ovsdb_idl_set_condition (ovsdb-idl.c:1095)
>> update_conditions (test-ovsdb.c:2299)
>> do_idl (test-ovsdb.c:2388)
>> ovs_cmdl_run_command__ (command-line.c:115)
>> main (test-ovsdb.c:73)
>>
>> Signed-off-by: Yi-Hung Wei 
> 
> Thanks, I applied this to master, branch-2.7, and branch-2.6.

Hi,
unlucky this commit breaks build on branch-2.6 since the function
"ovsdb_idl_condition_destroy" was added in commit
0164e367f5d8 ("ovsdb-idl: Change interface to conditional monitoring.")
and it's not available on branch 2.6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness

[László Sürü]

Thanks for the bfd update!

Just one more question,
Is the backporting of liveness propagation to OVS 2.7 branch planned also?

Thanks and regards
Laszlo

-Original Message-
From: Ben Pfaff [mailto:b...@ovn.org] 
Sent: Friday, April 21, 2017 7:39 PM
To: László Sürü 
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link 
aliveness

Thanks for the revision.

I still saw the failures, so I spent some time looking at why.  It turned out 
to be related to the two different database transactions used to configure BFD. 
 When I combined the two transactions into one, e.g. changed

AT_CHECK([ovs-vsctl add-br br1 -- \
  set bridge br1 datapath-type=dummy \
  other-config:hwaddr=aa:55:aa:56:00:00 -- \
  add-port br1 p1 -- set Interface p1 type=patch \
  options:peer=p0 ofport_request=2 -- \
  add-port br0 p0 -- set Interface p0 type=patch \
  options:peer=p1 ofport_request=1])

AT_CHECK([ovs-vsctl \
  set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 -- 
\
  set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])

into

AT_CHECK([ovs-vsctl add-br br1 -- \
  set bridge br1 datapath-type=dummy \
  other-config:hwaddr=aa:55:aa:56:00:00 -- \
  add-port br1 p1 -- set Interface p1 type=patch \
  options:peer=p0 ofport_request=2 -- \
  add-port br0 p0 -- set Interface p0 type=patch \
  options:peer=p1 ofport_request=1 -- \
  set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 -- 
\
  set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])

I no longer saw the failures.

I made that change in each of the BFD tests.  I also got rid of the use of the 
GNU grep --no-group-separator option, which BSD doesn't appear to support.

With those changes, I applied this to master.  Thanks again!

On Thu, Apr 20, 2017 at 03:41:57PM +, László Sürü wrote:
> Thanks for the review and comments.
> All unit tests were passing for me before with 'make check', I haven't seen 
> failing unit tests.
> It might have been a temporary misbehavior.
> 
> I've applied the proposed changes and successfully rerun the tests 
> also with 'testsuite' this time based on your attached log (see attachment).
> Also, I've adapted the tests to the new OF 1.6 test as well.
> 
> Please find the modified patched below in this mail.
> 
> Thanks and regards
> Laszlo
> 
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Saturday, April 15, 2017 6:44 AM
> To: László Sürü 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable 
> flag as link aliveness
> 
> Thanks a lot for the revised patch!  It will be good to get this done 
> properly.
> 
> I'm appending some changes that I suggest folding in to better match the 
> usual OVS coding style.
> 
> However, when I run the testsuite, I get the failures below.  Do you see 
> these too?  Can you take a look at them?  I'm also attaching the full 
> testsuite.log in case it helps.
> 
> bfd
> 
>  22: bfd - liveness propagation - OF1.3  FAILED (bfd.at:847)
>  23: bfd - liveness propagation - OF1.4  FAILED (bfd.at:920)
>  24: bfd - liveness propagation - OF1.5  FAILED (bfd.at:993)
> 
> ofproto
> 
> 890: ofproto - mod-port (OpenFlow 1.6)   FAILED (ofproto.at:1308)
> 
> --8<--cut here-->8--
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 
> 218e8eb..c82640d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
>  bfd_set_netdev(port->bfd, netdev);
>  }
>  
> +/* Set liveness, unless the link is administratively or
> + * operationally down or link monitoring false */
> +if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
> +port->may_enable) {
> +port->up.pp.state |= OFPUTIL_PS_LIVE;
> +} else {
> +port->up.pp.state &= ~OFPUTIL_PS_LIVE;
> +}
> +
>  ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
>   port->lldp, 
> >up.pp.hw_addr);
>  
> @@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
>  if (ofport->rstp_port) {
>  rstp_port_set_mac_operational(ofport->rstp_port, enable);
>  }
> +
> +/* Propagate liveness, unless the link is administratively or
> + * operationally down. */
> +if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
> +enum ofputil_port_state of_state = ofport->up.pp.state;
> +if (enable) {
> 

[ovs-dev] [PATCH v2 8/8] checkpatch: remove python from line_length_blacklist

[Aaron Conole]

Even though the build system flags this error, it is better for
checkpatch to flag it early.  It doesn't cost anything to do so.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index e89769a..b968fbc 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -87,8 +87,7 @@ skip_signoff_check = False
 # name, as they may have legitimate reasons to have longer lines.
 #
 # Python isn't checked as flake8 performs these checks during build.
-line_length_blacklist = ['.am', '.at', 'etc', '.in', '.m4', '.mk', '.patch',
- '.py']
+line_length_blacklist = ['.am', '.at', 'etc', '.in', '.m4', '.mk', '.patch']
 
 
 def is_subtracted_line(line):
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 6/8] checkpatch: filename from hunks fix

[Aaron Conole]

Filenames that come from the hunks match include the git-ified 'b/'
prefix, which makes jumping to the error file that much harder.  This
patch corrects that by simply skipping those bytes.

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 06c0a56..b4e5a04 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -300,7 +300,7 @@ def ovs_checkpatch_parse(text):
 elif parse == 2:
 newfile = hunks.match(line)
 if newfile:
-current_file = newfile.group(2)
+current_file = newfile.group(2)[2:]
 print_file_name = current_file
 continue
 reset_line_number = hunk_differences.match(line)
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 5/8] checkpatch: print conformance

[Aaron Conole]

Other utilities (notoriously the linux kernel's checkpatch.pl) have a more
standardized form for printing file and lines.  With this change, the
template used to print gains two enhancements:
1. Color
2. Conformance with the kernel's version of checkpatch.pl

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index b1e70e1..06c0a56 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -23,33 +23,41 @@ __errors = 0
 __warnings = 0
 print_file_name = None
 checking_file = False
+total_line = 0
+colors = False
 
 
-def print_file():
-global print_file_name
-if print_file_name:
-print("In file %s" % print_file_name)
-print_file_name = None
+def get_color_end():
+global colors
+if colors:
+return "\033[00m"
+return ""
 
 
-def print_error(message, lineno=None):
+def get_red_begin():
+global colors
+if colors:
+return "\033[91m"
+return ""
+
+
+def get_yellow_begin():
+global colors
+if colors:
+return "\033[93m"
+return ""
+
+
+def print_error(message):
 global __errors
-print_file()
-if lineno is not None:
-print("E(%d): %s" % (lineno, message))
-else:
-print("E: %s" % (message))
+print("%sERROR%s: %s" % (get_red_begin(), get_color_end(), message))
 
 __errors = __errors + 1
 
 
-def print_warning(message, lineno=None):
+def print_warning(message):
 global __warnings
-print_file()
-if lineno:
-print("W(%d): %s" % (lineno, message))
-else:
-print("W: %s" % (message))
+print("%sWARNING%s: %s" % (get_yellow_begin(), get_color_end(), message))
 
 __warnings = __warnings + 1
 
@@ -176,33 +184,29 @@ checks = [
  'match_name':
  lambda x: not any([fmt in x for fmt in line_length_blacklist]),
  'check': lambda x: line_length_check(x),
- 'print':
- lambda(x): print_warning("Line is greater than 79-characters long", x)},
+ 'print': lambda: print_warning("Line length is >79-characters long")},
 
 {'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 4/8] checkpatch: correct a parsing issue

[Aaron Conole]

Occasionally, characters will be sent which violate the
ascii decoder's sense of propriety.  In fact, in-tree there are
a few such files (ex: tests/atlocal.in), and they cause an
exception to be raised when they are encountered.

Set the policy to ignore these cases.  This means these bytes are
omitted from the text stream during processing.

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 119a996..b1e70e1 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -252,7 +252,7 @@ def ovs_checkpatch_parse(text):
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
 
-for line in text.decode().split('\n'):
+for line in text.decode(errors='ignore').split('\n'):
 if current_file != previous_file:
 previous_file = current_file
 
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 3/8] checkpatch: move the checks to the framework

[Aaron Conole]

All of the checks are now part of the new 'check' framework.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 14d4f53..119a996 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -178,6 +178,31 @@ checks = [
  'check': lambda x: line_length_check(x),
  'print':
  lambda(x): print_warning("Line is greater than 79-characters long", x)},
+
+{'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 2/8] checkpatch: common print_line

[Aaron Conole]

With the new framework, print_line can be moved out to the checks
framework.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index ddee7c8..14d4f53 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -200,9 +200,14 @@ def get_file_type_checks(filename):
 def run_checks(current_file, line, lineno):
 """Runs the various checks for the particular line.  This will take
filename into account."""
+print_line = False
 for check in get_file_type_checks(current_file):
 if check['check'](line):
 check['print'](lineno)
+print_line = True
+
+if print_line:
+print("\n%s\n" % line)
 
 
 def ovs_checkpatch_parse(text):
@@ -258,7 +263,6 @@ def ovs_checkpatch_parse(text):
 m = is_co_author.match(line)
 co_authors.append(m.group(3))
 elif parse == 2:
-print_line = False
 newfile = hunks.match(line)
 if newfile:
 current_file = newfile.group(2)
@@ -283,26 +287,19 @@ def ovs_checkpatch_parse(text):
 continue
 if (not current_file.endswith('.mk') and
 not leading_whitespace_is_spaces(cmp_line)):
-print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
 run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
-print_line = True
 print_warning("Line has trailing whitespace", lineno)
 if not if_and_for_whitespace_checks(cmp_line):
-print_line = True
 print_error("Improper whitespace around control block",
 lineno)
 if not if_and_for_end_with_bracket_check(cmp_line):
-print_line = True
 print_error("Inappropriate bracing around statement", lineno)
 if pointer_whitespace_check(cmp_line):
-print_line = True
 print_error("Inappropriate spacing in pointer declaration",
 lineno)
-if print_line:
-print("\n%s\n" % line)
 if __errors or __warnings:
 return -1
 return 0
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/8] checkpatch: introduce a flexible framework

[Aaron Conole]

Developers wishing to add checks to checkpatch sift through an adhoc mess,
currently.  The process goes something like:
1. Figure out what to test in the patch
2. Write some code, quickly, that checks for that condition
3. Look through the statemachine to find where the check should go
4. ignore parts of the above and just throw something together

That worked fine for the initial development, but as interesting new tests
are developed, it is important to have a more flexible framework that lets
a developer just plug in a new test, easily.

This commit brings in a new framework that allows plugging in checks very
quickly.  Hook up the line-length test as an initial demonstration.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 638ac97..ddee7c8 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -164,6 +164,47 @@ def pointer_whitespace_check(line):
 return __regex_ptr_declaration_missing_whitespace.search(line) is not None
 
 
+def line_length_check(line):
+"""Return TRUE if the line length is too long"""
+if len(line) > 79:
+return True
+return False
+
+
+checks = [
+{'regex': None,
+ 'match_name':
+ lambda x: not any([fmt in x for fmt in line_length_blacklist]),
+ 'check': lambda x: line_length_check(x),
+ 'print':
+ lambda(x): print_warning("Line is greater than 79-characters long", x)},
+]
+
+
+def get_file_type_checks(filename):
+"""Returns the list of checks for a file based on matching the filename
+   against regex."""
+global checks
+checkList = []
+for check in checks:
+if check['regex'] is None and check['match_name'] is None:
+checkList.append(check)
+if check['regex'] is not None and \
+   re.compile(check['regex']).search(filename) is not None:
+checkList.append(check)
+elif check['match_name'] is not None and check['match_name'](filename):
+checkList.append(check)
+return checkList
+
+
+def run_checks(current_file, line, lineno):
+"""Runs the various checks for the particular line.  This will take
+   filename into account."""
+for check in get_file_type_checks(current_file):
+if check['check'](line):
+check['print'](lineno)
+
+
 def ovs_checkpatch_parse(text):
 global print_file_name
 lineno = 0
@@ -180,15 +221,10 @@ def ovs_checkpatch_parse(text):
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
-skip_line_length_check = False
 
 for line in text.decode().split('\n'):
 if current_file != previous_file:
 previous_file = current_file
-if any([fmt in current_file for fmt in line_length_blacklist]):
-skip_line_length_check = True
-else:
-skip_line_length_check = False
 
 lineno = lineno + 1
 if len(line) <= 0:
@@ -250,13 +286,10 @@ def ovs_checkpatch_parse(text):
 print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
+run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
 print_line = True
 print_warning("Line has trailing whitespace", lineno)
-if len(cmp_line) > 79 and not skip_line_length_check:
-print_line = True
-print_warning("Line is greater than 79-characters long",
-  lineno)
 if not if_and_for_whitespace_checks(cmp_line):
 print_line = True
 print_error("Improper whitespace around control block",
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 0/8] checkpatch: enhancements

[Aaron Conole]

The following series refactors checkpatch to make
file-type specific checks.  This lets checkpatch have
finer grained checks, and should reduce the amount
of false positives.

Additionally, I've incorporated an older request to print
files in the more standard `filename:lineno: ` form that
most editors support out of the box.  And while I was in
there, I added pretty colors, because we all need some
color in our terminals.

v2:
* Fix flake8 errors
* Remove python from the line-length errors blacklist

Aaron Conole (8):
  checkpatch: introduce a flexible framework
  checkpatch: common print_line
  checkpatch: move the checks to the framework
  checkpatch: correct a parsing issue
  checkpatch: print conformance
  checkpatch: filename from hunks fix
  checkpatch: fix pointer declaration
  checkpatch: remove python from line_length_blacklist

 utilities/checkpatch.py | 169 
 1 file changed, 113 insertions(+), 56 deletions(-)

-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Sync on PTAP, EXT-382 and NSH - Tue 2017-05-09 17:00 CET

[Jan Scheurich]

Hi,

Let's have a look at the status and work out a plan how to accelerate the 
review and merging in order to achieve the agreed target to upstream these 
changes in time for OVS 2.8.

Thank you,
Jan

Link to the Google design doc:
https://docs.google.com/document/d/1oWMYUH8sjZJzWa72o2q9kU0N6pNE-rwZcLH3-kbbDR8/edit


.
--> Join Skype Meeting
  This is an online meeting for Skype for Business, the professional meetings 
and communications app formerly known as Lync.
Join by phone

+492115343925 (Germany)  English (United 
States)
89925 (Germany) English (United States)

Find a local number

Conference ID: 70849799
 Forgot your dial-in PIN? 
|Help


To join a Lync / Skype for Business meeting from an Ericsson standard video 
room, add 77 before the Conference ID (e.g. 771234567 where 1234567 is the 
conference ID).To join from a video room outside of Ericsson add one of the 
domains after 77 and Conference ID (e.g. 771234567@ .ericsson.net, where 
=emea/apac/amcs).  For assistance contact the IT Service Desk.
[!OC([1033])!]
.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Sync on PTAP, EXT-382 and NSH: Minutes of meeting Wed 2017-04-26, 17:00 CET

[Jan Scheurich]

Participants
*   Ben P, Zoltan, Georg, Jan, Jiri

Review/Discussion of current patch packages
1.  RTNETLINK tunnel configuration - v3 still under review by Joe S
2.  L3 Tunneling - v4 out fixing sparse errors. Ben to continue review
3.  PTAP - v1 out for review. Will need rebase to v4
4.  Basic NSH MD1 fields (according to google doc) - v1 out of review
5.  Gen encap/decap for NSH - v1 out, needs rework before review
6.  Gen encap/decap for Ethernet - ongoing (Zoltan)
7.  L3 config based on RTNETLINK - not started. Jiri to talk to Eric if it 
can be started

Status/Planning
*   Lead time is getting an issue for 2.8 (branch date around July 1st)
  We need to focus on review and merge. Start with L3 tunneling v4 and 
RTNETLINK v3 then Basic NSH
*   Ben to try to find an additional committer to help review
*   Jan, Zoltan and Yi to sort out open implementation issues with respect 
to generic encap/decap for NSH. Wait with pushing v2 for review until it is 
agreed in the team and functionally tested in accordance with the google doc.
*   Jan to call for next meeting in two weeks

BR, Jan

  -Original Appointment-
  From: Jan Scheurich
  Sent: Sunday, 18 December, 2016 15:34
  To: Jan Scheurich; Zoltán Balogh; Yang, Yi Y (yi.y.y...@intel.com); Jiri 
Benc (jb...@redhat.com); Pravin Shelar; Simon Horman 
(simon.hor...@netronome.com); 'ja...@ovn.org'; 'Ben Pfaff'; 
'ben.mackcr...@corsa.com'; d...@openvswitch.org; Georg Schmuecking
  Subject: Sync on PTAP, EXT-382 and NSH
  When: Wednesday, 26 April, 2017 17:00-18:30 (UTC+01:00) Amsterdam, 
Berlin, Bern, Rome, Stockholm, Vienna.
  Where: Skype Meeting


  Hi,

  The team is making good progress in preparing the various patch packages. 
We have a lot of things working in our gitlab repo 
(https://gitlab.com/JScheurich/ovs). v3 of the L3 tunneling patches are out on 
the mailing list since a week 
(https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330488.html) and are 
waiting for review.

  The PTAP series will follow next week. Also the NSH MD1 fields and 
generic encap/decap actions for Ethernet and NSH MD1 are mostly done.

  Let's have a look at the status and work out a plan how to accelerate the 
review and merging in order to achieve the agreed target to upstream these 
changes in time for OVS 2.8.

  Thank you,
  Jan

  Link to the Google design doc:
  
https://docs.google.com/document/d/1oWMYUH8sjZJzWa72o2q9kU0N6pNE-rwZcLH3-kbbDR8/edit


  
.
  --> Join Skype Meeting
  This is an online meeting for Skype for Business, the professional meetings 
and communications app formerly known as Lync.
  Join by phone

  +492115343925 (Germany)English 
(United States)
  89925 (Germany)   English (United States)

  Find a local number

  Conference ID: 70849799
   Forgot your dial-in PIN? 
|Help


  To join a Lync / Skype for Business meeting from an Ericsson standard 
video room, add 77 before the Conference ID (e.g. 771234567 where 1234567 is 
the conference ID).To join from a video room outside of Ericsson add one of 
the domains after 77 and Conference ID (e.g. 771234567@ .ericsson.net, 
where =emea/apac/amcs).  For assistance contact the IT Service Desk.
  [!OC([1033])!]
  
.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/7] checkpatch: enhancements

[Aaron Conole]

Aaron Conole  writes:

> The following series refactors checkpatch to make
> file-type specific checks.  This lets checkpatch have
> finer grained checks, and should reduce the amount
> of false positives.
>
> Additionally, I've incorporated an older request to print
> files in the more standard `filename:lineno: ` form that
> most editors support out of the box.  And while I was in
> there, I added pretty colors, because we all need some
> color in our terminals.
>
> Aaron Conole (7):
>   checkpatch: introduce a flexible framework
>   checkpatch: common print_line
>   checkpatch: move the checks to the framework
>   checkpatch: correct a parsing issue
>   checkpatch: print conformance
>   checkpatch: filename from hunks fix
>   checkpatch: fix pointer declaration
>
>  utilities/checkpatch.py | 165 
> 
>  1 file changed, 111 insertions(+), 54 deletions(-)

Ugh... just noticed that this introduces a flake8 failure.

I'm going to rework this and remove .py files from the line-length
exemption.  I know flake8 covers it, too - but it would have saved me
from this mistake.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 7/7] checkpatch: fix pointer declaration

[Aaron Conole]

A common way of expressing 'raise to the power of' when authoring
comments uses **.  This is currently getting caught by the pointer
spacing warning.  So, catch it here.

Reported-by: Lance Richardson 
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 8f948dc..bf7aded 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -76,7 +76,7 @@ __regex_is_for_if_single_line_bracket = \
 re.compile(r'^ +(if|for|while) \(.*\)')
 __regex_ends_with_bracket = \
 re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$')
-__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*')
+__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 6/7] checkpatch: filename from hunks fix

[Aaron Conole]

Filenames that come from the hunks match include the git-ified 'b/'
prefix, which makes jumping to the error file that much harder.  This
patch corrects that by simply skipping those bytes.

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 48a2aaf..8f948dc 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -299,7 +299,7 @@ def ovs_checkpatch_parse(text):
 elif parse == 2:
 newfile = hunks.match(line)
 if newfile:
-current_file = newfile.group(2)
+current_file = newfile.group(2)[2:]
 print_file_name = current_file
 continue
 reset_line_number = hunk_differences.match(line)
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 5/7] checkpatch: print conformance

[Aaron Conole]

Other utilities (notoriously the linux kernel's checkpatch.pl) have a more
standardized form for printing file and lines.  With this change, the
template used to print gains two enhancements:
1. Color
2. Conformance with the kernel's version of checkpatch.pl

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 22afbe4..48a2aaf 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -23,33 +23,41 @@ __errors = 0
 __warnings = 0
 print_file_name = None
 checking_file = False
+total_line = 0
+colors = False
 
 
-def print_file():
-global print_file_name
-if print_file_name:
-print("In file %s" % print_file_name)
-print_file_name = None
+def get_color_end():
+global colors
+if colors:
+return "\033[00m"
+return ""
 
 
-def print_error(message, lineno=None):
+def get_red_begin():
+global colors
+if colors:
+return "\033[91m"
+return ""
+
+
+def get_yellow_begin():
+global colors
+if colors:
+return "\033[93m"
+return ""
+
+
+def print_error(message):
 global __errors
-print_file()
-if lineno is not None:
-print("E(%d): %s" % (lineno, message))
-else:
-print("E: %s" % (message))
+print("%sERROR%s: %s" % (get_red_begin(), get_color_end(), message))
 
 __errors = __errors + 1
 
 
-def print_warning(message, lineno=None):
+def print_warning(message):
 global __warnings
-print_file()
-if lineno:
-print("W(%d): %s" % (lineno, message))
-else:
-print("W: %s" % (message))
+print("%sWARNING%s: %s" % (get_yellow_begin(), get_color_end(), message))
 
 __warnings = __warnings + 1
 
@@ -176,33 +184,29 @@ checks = [
  'match_name':
  lambda x: not any([fmt in x for fmt in line_length_blacklist]),
  'check': lambda x: line_length_check(x),
- 'print':
- lambda(x): print_warning("Line is greater than 79-characters long", x)},
+ 'print': lambda: print_warning("Line is greater than 79-characters 
long")},
 
 {'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 4/7] checkpatch: correct a parsing issue

[Aaron Conole]

Occasionally, characters will be sent which violate the
ascii decoder's sense of propriety.  In fact, in-tree there are
a few such files (ex: tests/atlocal.in), and they cause an
exception to be raised when they are encountered.

Set the policy to ignore these cases.  This means these bytes are
omitted from the text stream during processing.

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 d567933..22afbe4 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -251,7 +251,7 @@ def ovs_checkpatch_parse(text):
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
 
-for line in text.decode().split('\n'):
+for line in text.decode(errors='ignore').split('\n'):
 if current_file != previous_file:
 previous_file = current_file
 
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 3/7] checkpatch: move the checks to the framework

[Aaron Conole]

All of the checks are now part of the new 'check' framework.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 1504668..d567933 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -178,6 +178,31 @@ checks = [
  'check': lambda x: line_length_check(x),
  'print':
  lambda(x): print_warning("Line is greater than 79-characters long", x)},
+
+{'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/7] checkpatch: common print_line

[Aaron Conole]

With the new framework, print_line can be moved out to the checks
framework.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index ddee7c8..1504668 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -200,10 +200,14 @@ def get_file_type_checks(filename):
 def run_checks(current_file, line, lineno):
 """Runs the various checks for the particular line.  This will take
filename into account."""
+print_line = False
 for check in get_file_type_checks(current_file):
 if check['check'](line):
 check['print'](lineno)
+print_line = True
 
+if print_line:
+print("\n%s\n" % line)
 
 def ovs_checkpatch_parse(text):
 global print_file_name
@@ -258,7 +262,6 @@ def ovs_checkpatch_parse(text):
 m = is_co_author.match(line)
 co_authors.append(m.group(3))
 elif parse == 2:
-print_line = False
 newfile = hunks.match(line)
 if newfile:
 current_file = newfile.group(2)
@@ -283,26 +286,19 @@ def ovs_checkpatch_parse(text):
 continue
 if (not current_file.endswith('.mk') and
 not leading_whitespace_is_spaces(cmp_line)):
-print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
 run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
-print_line = True
 print_warning("Line has trailing whitespace", lineno)
 if not if_and_for_whitespace_checks(cmp_line):
-print_line = True
 print_error("Improper whitespace around control block",
 lineno)
 if not if_and_for_end_with_bracket_check(cmp_line):
-print_line = True
 print_error("Inappropriate bracing around statement", lineno)
 if pointer_whitespace_check(cmp_line):
-print_line = True
 print_error("Inappropriate spacing in pointer declaration",
 lineno)
-if print_line:
-print("\n%s\n" % line)
 if __errors or __warnings:
 return -1
 return 0
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/7] checkpatch: introduce a flexible framework

[Aaron Conole]

Developers wishing to add checks to checkpatch sift through an adhoc mess,
currently.  The process goes something like:
1. Figure out what to test in the patch
2. Write some code, quickly, that checks for that condition
3. Look through the statemachine to find where the check should go
4. ignore parts of the above and just throw something together

That worked fine for the initial development, but as interesting new tests
are developed, it is important to have a more flexible framework that lets
a developer just plug in a new test, easily.

This commit brings in a new framework that allows plugging in checks very
quickly.  Hook up the line-length test as an initial demonstration.

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

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 638ac97..ddee7c8 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -164,6 +164,47 @@ def pointer_whitespace_check(line):
 return __regex_ptr_declaration_missing_whitespace.search(line) is not None
 
 
+def line_length_check(line):
+"""Return TRUE if the line length is too long"""
+if len(line) > 79:
+return True
+return False
+
+
+checks = [
+{'regex': None,
+ 'match_name':
+ lambda x: not any([fmt in x for fmt in line_length_blacklist]),
+ 'check': lambda x: line_length_check(x),
+ 'print':
+ lambda(x): print_warning("Line is greater than 79-characters long", x)},
+]
+
+
+def get_file_type_checks(filename):
+"""Returns the list of checks for a file based on matching the filename
+   against regex."""
+global checks
+checkList = []
+for check in checks:
+if check['regex'] is None and check['match_name'] is None:
+checkList.append(check)
+if check['regex'] is not None and \
+   re.compile(check['regex']).search(filename) is not None:
+checkList.append(check)
+elif check['match_name'] is not None and check['match_name'](filename):
+checkList.append(check)
+return checkList
+
+
+def run_checks(current_file, line, lineno):
+"""Runs the various checks for the particular line.  This will take
+   filename into account."""
+for check in get_file_type_checks(current_file):
+if check['check'](line):
+check['print'](lineno)
+
+
 def ovs_checkpatch_parse(text):
 global print_file_name
 lineno = 0
@@ -180,15 +221,10 @@ def ovs_checkpatch_parse(text):
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
-skip_line_length_check = False
 
 for line in text.decode().split('\n'):
 if current_file != previous_file:
 previous_file = current_file
-if any([fmt in current_file for fmt in line_length_blacklist]):
-skip_line_length_check = True
-else:
-skip_line_length_check = False
 
 lineno = lineno + 1
 if len(line) <= 0:
@@ -250,13 +286,10 @@ def ovs_checkpatch_parse(text):
 print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
+run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
 print_line = True
 print_warning("Line has trailing whitespace", lineno)
-if len(cmp_line) > 79 and not skip_line_length_check:
-print_line = True
-print_warning("Line is greater than 79-characters long",
-  lineno)
 if not if_and_for_whitespace_checks(cmp_line):
 print_line = True
 print_error("Improper whitespace around control block",
-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 0/7] checkpatch: enhancements

[Aaron Conole]

The following series refactors checkpatch to make
file-type specific checks.  This lets checkpatch have
finer grained checks, and should reduce the amount
of false positives.

Additionally, I've incorporated an older request to print
files in the more standard `filename:lineno: ` form that
most editors support out of the box.  And while I was in
there, I added pretty colors, because we all need some
color in our terminals.

Aaron Conole (7):
  checkpatch: introduce a flexible framework
  checkpatch: common print_line
  checkpatch: move the checks to the framework
  checkpatch: correct a parsing issue
  checkpatch: print conformance
  checkpatch: filename from hunks fix
  checkpatch: fix pointer declaration

 utilities/checkpatch.py | 165 
 1 file changed, 111 insertions(+), 54 deletions(-)

-- 
2.9.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Ben Pfaff]

On Wed, Apr 26, 2017 at 04:00:55PM +0100, Stephen Finucane wrote:
> On Wed, 2017-04-26 at 10:55 -0400, Lance Richardson wrote:
> > > From: "Ben Pfaff" 
> > > To: "Lance Richardson" 
> > > Cc: "Leif Madsen" , "ovs dev"  > > org>
> > > Sent: Wednesday, 26 April, 2017 10:33:09 AM
> > > Subject: Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST
> > > 
> > > On Tue, Apr 25, 2017 at 04:03:55PM -0400, Lance Richardson wrote:
> > > > I ran into this a little while ago. The problem in my case was
> > > > that
> > > > sphinx-build
> > > > was not installed, so these man pages were not being built (seems
> > > > odd that
> > > > this
> > > > would not cause a build failure...)
> > > > 
> > > > Doing "yum install python-sphinx" let me build the rpms
> > > > successfully once
> > > > again. Seems we should add "BuildRequires: python-sphinx" to the
> > > > spec file.
> > > 
> > > Huh.  Somehow I thought that we'd made Sphinx an overall build
> > > requirement.  Maybe that's the right thing to do.
> > > 
> > 
> > The rules to build rst-formatted man pages in the top-level
> > Makefile.in are predicated on HAVE_SPHINX_TRUE, which does give the
> > impression that Sphinx is optional.
> > 
> >    Lance
> 
> Yup, it's optional since these were only used for HTML docs initially.
> I didn't change that as HAVE_GROFF is also a thing, afaict, and I
> wasn't sure if man pages were essential. Maybe they should be now.

HAVE_GROFF might be a deceptive example, because the availability of
groff only affects whether the build can do a little bit of checking of
the manpages' syntax at build time.  It doesn't affect whether manpages
can be built; they always can be.

I'm inclined to try making Sphinx a hard requirement for the build, and
then relaxing that if it proves untenable in practice.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Lance Richardson]

> From: "Stephen Finucane" 
> To: "Lance Richardson" , "Ben Pfaff" 
> Cc: "ovs dev" 
> Sent: Wednesday, 26 April, 2017 11:00:55 AM
> Subject: Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST
> 
> On Wed, 2017-04-26 at 10:55 -0400, Lance Richardson wrote:
> > > From: "Ben Pfaff" 
> > > To: "Lance Richardson" 
> > > Cc: "Leif Madsen" , "ovs dev"  > > org>
> > > Sent: Wednesday, 26 April, 2017 10:33:09 AM
> > > Subject: Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST
> > > 
> > > On Tue, Apr 25, 2017 at 04:03:55PM -0400, Lance Richardson wrote:
> > > > I ran into this a little while ago. The problem in my case was
> > > > that
> > > > sphinx-build
> > > > was not installed, so these man pages were not being built (seems
> > > > odd that
> > > > this
> > > > would not cause a build failure...)
> > > > 
> > > > Doing "yum install python-sphinx" let me build the rpms
> > > > successfully once
> > > > again. Seems we should add "BuildRequires: python-sphinx" to the
> > > > spec file.
> > > 
> > > Huh.  Somehow I thought that we'd made Sphinx an overall build
> > > requirement.  Maybe that's the right thing to do.
> > > 
> > 
> > The rules to build rst-formatted man pages in the top-level
> > Makefile.in are predicated on HAVE_SPHINX_TRUE, which does give the
> > impression that Sphinx is optional.
> > 
> >    Lance
> 
> Yup, it's optional since these were only used for HTML docs initially.
> I didn't change that as HAVE_GROFF is also a thing, afaict, and I
> wasn't sure if man pages were essential. Maybe they should be now.
> 

It seems fine to me to handle this by having stricter requirements for
packaging than for the base build (as they need to be anyway). OTOH there
would be some benefits to making it a requirement for the base build, such
as catching build errors earlier.

   Lance

> Stephen
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] [packaging] Remove unneeded manpage globs

[Leif Madsen]

On Mon, Apr 24, 2017 at 4:07 PM, Ben Pfaff  wrote:

> On Mon, Apr 24, 2017 at 04:00:30PM -0400, Leif Madsen wrote:
> > Remove unneeded manpage globs from the RPM spec file since they
> > are no longer being generated.
> > ---
> >  rhel/openvswitch-fedora.spec.in | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.
> spec.in
> > index dbcab00..1095cd0 100644
> > --- a/rhel/openvswitch-fedora.spec.in
> > +++ b/rhel/openvswitch-fedora.spec.in
> > @@ -468,8 +468,6 @@ fi
> >  %{_bindir}/ovs-pcap
> >  %{_bindir}/ovs-tcpdump
> >  %{_bindir}/ovs-tcpundump
> > -%{_mandir}/man8/ovs-test.8*
> > -%{_mandir}/man8/ovs-vlan-test.8*
> >  %{_mandir}/man8/ovs-l3ping.8*
> >  %{_mandir}/man1/ovs-pcap.1*
> >  %{_mandir}/man8/ovs-tcpdump.8*
>
> I'm confused.  There was no removal or renaming of the installed man8
> pages, only of the source files.  So, when I run "make install
> DESTDIR=$PWD/inst", I get a file inst/usr/share/man/man8/ovs-test.8
> installed, and that should be the same as before.
>
> Any idea what's going on?
>

Fixed by Lance Richardson in another patch. This should be abandoned.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] rhel: add python-sphinx as a build dependency

[Leif Madsen]

On Tue, Apr 25, 2017 at 4:48 PM, Lance Richardson 
wrote:

> The python-sphinx package is now required in order to build
> man pages, add this package as a build requirement.
>
> Reported-by: Leif Madsen 
> Signed-off-by: Lance Richardson 
> ---
>  rhel/openvswitch-fedora.spec.in | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.
> spec.in
> index dbcab00..5d53284 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -75,6 +75,7 @@ BuildRequires: python3-devel
>  BuildRequires: desktop-file-utils
>  BuildRequires: groff graphviz
>  BuildRequires: checkpolicy, selinux-policy-devel
> +BuildRequires: python-sphinx
>  # make check dependencies
>  BuildRequires: %{_py2}-twisted%{?rhel:-core} %{_py2}-zope-interface
> %{_py2}-six
>  BuildRequires: procps-ng
> --
> 2.9.3
>

Acked-By: Leif Madsen 
Tested-By: Leif Madsen 

Ran a local build test with latest master OVS repo. Test results in failed
build due to files not being found for glob:

error: File not found by glob:
/builddir/build/BUILDROOT/openvswitch-2.7.90.14194.git4decc1bf-1.el7.centos.x86_64/usr/share/man/man8/ovs-test.8*
error: File not found by glob:
/builddir/build/BUILDROOT/openvswitch-2.7.90.14194.git4decc1bf-1.el7.centos.x86_64/usr/share/man/man8/ovs-vlan-test.8*

Manual patch being applied to repo, then local build with mock validates
that things build successfully.

Test method:

patch -p1 <
~/Downloads/ovs-dev-rhel-add-python-sphinx-as-a-build-dependency.patch
mock --root epel-7-x86_64 --dnf --spec ovs/rhel/openvswitch-fedora.spec
--sources=ovs-sources/ --resultdir=result --buildsrpm
mock --root epel-7-x86_64 --dnf --without=check --no-clean
result/openvswitch-2.7.90.14194.git4decc1bf-1.el7.centos.src.rpm
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Leif Madsen]

On Wed, Apr 26, 2017 at 11:00 AM, Stephen Finucane 
wrote:

> On Wed, 2017-04-26 at 10:55 -0400, Lance Richardson wrote:
> > The rules to build rst-formatted man pages in the top-level
> > Makefile.in are predicated on HAVE_SPHINX_TRUE, which does give the
> > impression that Sphinx is optional.
>
> Yup, it's optional since these were only used for HTML docs initially.
> I didn't change that as HAVE_GROFF is also a thing, afaict, and I
> wasn't sure if man pages were essential. Maybe they should be now.


Looks like it might not be optional now (at least for package building). I
can confirm 100% now that the change Lance provided does result in
successful builds.

Leif.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Stephen Finucane]

On Wed, 2017-04-26 at 10:55 -0400, Lance Richardson wrote:
> > From: "Ben Pfaff" 
> > To: "Lance Richardson" 
> > Cc: "Leif Madsen" , "ovs dev"  > org>
> > Sent: Wednesday, 26 April, 2017 10:33:09 AM
> > Subject: Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST
> > 
> > On Tue, Apr 25, 2017 at 04:03:55PM -0400, Lance Richardson wrote:
> > > I ran into this a little while ago. The problem in my case was
> > > that
> > > sphinx-build
> > > was not installed, so these man pages were not being built (seems
> > > odd that
> > > this
> > > would not cause a build failure...)
> > > 
> > > Doing "yum install python-sphinx" let me build the rpms
> > > successfully once
> > > again. Seems we should add "BuildRequires: python-sphinx" to the
> > > spec file.
> > 
> > Huh.  Somehow I thought that we'd made Sphinx an overall build
> > requirement.  Maybe that's the right thing to do.
> > 
> 
> The rules to build rst-formatted man pages in the top-level
> Makefile.in are predicated on HAVE_SPHINX_TRUE, which does give the
> impression that Sphinx is optional.
> 
>    Lance

Yup, it's optional since these were only used for HTML docs initially.
I didn't change that as HAVE_GROFF is also a thing, afaict, and I
wasn't sure if man pages were essential. Maybe they should be now.

Stephen
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Lance Richardson]

> From: "Ben Pfaff" 
> To: "Lance Richardson" 
> Cc: "Leif Madsen" , "ovs dev" 
> Sent: Wednesday, 26 April, 2017 10:33:09 AM
> Subject: Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST
> 
> On Tue, Apr 25, 2017 at 04:03:55PM -0400, Lance Richardson wrote:
> > I ran into this a little while ago. The problem in my case was that
> > sphinx-build
> > was not installed, so these man pages were not being built (seems odd that
> > this
> > would not cause a build failure...)
> > 
> > Doing "yum install python-sphinx" let me build the rpms successfully once
> > again. Seems we should add "BuildRequires: python-sphinx" to the spec file.
> 
> Huh.  Somehow I thought that we'd made Sphinx an overall build
> requirement.  Maybe that's the right thing to do.
> 

The rules to build rst-formatted man pages in the top-level Makefile.in
are predicated on HAVE_SPHINX_TRUE, which does give the impression that Sphinx
is optional.

   Lance
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Leif Madsen]

On Wed, Apr 26, 2017 at 10:33 AM, Ben Pfaff  wrote:

> On Tue, Apr 25, 2017 at 04:03:55PM -0400, Lance Richardson wrote:
> > I ran into this a little while ago. The problem in my case was that
> sphinx-build
> > was not installed, so these man pages were not being built (seems odd
> that this
> > would not cause a build failure...)
> >
> > Doing "yum install python-sphinx" let me build the rpms successfully once
> > again. Seems we should add "BuildRequires: python-sphinx" to the spec
> file.
>
> Huh.  Somehow I thought that we'd made Sphinx an overall build
> requirement.  Maybe that's the right thing to do.
>

Unfortunately, actually, the build failed (what I saw must have been an out
of date build that succeeded). I'm going to test locally and I'll get back
to you :)

Leif.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Ben Pfaff]

On Tue, Apr 25, 2017 at 04:03:55PM -0400, Lance Richardson wrote:
> I ran into this a little while ago. The problem in my case was that 
> sphinx-build
> was not installed, so these man pages were not being built (seems odd that 
> this
> would not cause a build failure...)
> 
> Doing "yum install python-sphinx" let me build the rpms successfully once
> again. Seems we should add "BuildRequires: python-sphinx" to the spec file.

Huh.  Somehow I thought that we'd made Sphinx an overall build
requirement.  Maybe that's the right thing to do.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST

[Leif Madsen]

This is indeed the correct approach it seems. I've validated that adding
Lance's change results in a successful build with the original globs. I'll
ACK his patch here in a moment.

Thanks!
Leif.

On Tue, Apr 25, 2017 at 4:03 PM, Lance Richardson 
wrote:

> > From: "Ben Pfaff" 
> > To: "Leif Madsen" 
> > Cc: "ovs dev" 
> > Sent: Monday, 24 April, 2017 5:02:28 PM
> > Subject: Re: [ovs-dev] [RFC 2/4] doc: Convert ovs-vlan-test to rST
> >
> > On Mon, Apr 24, 2017 at 04:37:51PM -0400, Leif Madsen wrote:
> > > On Mon, Apr 24, 2017 at 3:57 PM, Ben Pfaff  wrote:
> > >
> > > > On Mon, Apr 24, 2017 at 03:52:53PM -0400, Leif Madsen wrote:
> > > > > I think this change might have broken packaging :)
> > > > >
> > > > > I just tested, and with the removal / renaming of the man8 pages
> for
> > > > > ovs-test and ovs-vlan-test, the RPM fails to build because of
> missing
> > > > files
> > > > > that no longer match the glob.
> > > > >
> > > > > These two lines need to be removed from the build:
> > > > >
> > > > > 471 %{_mandir}/man8/ovs-test.8*
> > > > > 472 %{_mandir}/man8/ovs-vlan-test.8*
> > > > >
> > > > >
> > > > > I'll submit a patch shortly.
> > > >
> > > > OK, I'm confused then.  There was no removal or renaming of the
> > > > installed man8 pages, only of the source files.  So, when I run "make
> > > > install DESTDIR=$PWD/inst", I get a file
> > > > inst/usr/share/man/man8/ovs-test.8 installed, and that should be the
> > > > same as before.
> > > >
> > > > Any idea what's going on?
> > > >
> > >
> > > Huh, well then I'm very confused as well :)
> > >
> > > I just did a test, and things seem to build fine when I remove those
> two
> > > lines. Otherwise, the RPM will fail to build with the following:
> > >
> > > Processing files:
> > > openvswitch-test-2.7.90.14191.git3570f7e4-1.el7.centos.noarch
> > > error: File not found by glob:
> > > /builddir/build/BUILDROOT/openvswitch-2.7.90.14191.
> git3570f7e4-1.el7.centos.x86_64/usr/share/man/man8/ovs-test.8*
> > > error: File not found by glob:
> > > /builddir/build/BUILDROOT/openvswitch-2.7.90.14191.
> git3570f7e4-1.el7.centos.x86_64/usr/share/man/man8/ovs-vlan-test.8*
> > >
> > >
> > > RPM build errors:
> > > File not found by glob:
> > > /builddir/build/BUILDROOT/openvswitch-2.7.90.14191.
> git3570f7e4-1.el7.centos.x86_64/usr/share/man/man8/ovs-test.8*
> > > File not found by glob:
> > > /builddir/build/BUILDROOT/openvswitch-2.7.90.14191.
> git3570f7e4-1.el7.centos.x86_64/usr/share/man/man8/ovs-vlan-test.8*
> > >
>
> I ran into this a little while ago. The problem in my case was that
> sphinx-build
> was not installed, so these man pages were not being built (seems odd that
> this
> would not cause a build failure...)
>
> Doing "yum install python-sphinx" let me build the rpms successfully once
> again. Seems we should add "BuildRequires: python-sphinx" to the spec file.
>
> Regards,
>
>Lance
>



-- 
Leif Madsen | Partner Engineer - NFV & CI
NFV Partner Engineering
Red Hat
GPG: (D670F846) BEE0 336E 5406 42BA 6194 6831 B38A 291E D670 F846
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] datapath: Fixups for MPLS GSO

[Simon Horman]

On Tue, Apr 25, 2017 at 05:45:34PM -0700, Yi-Hung Wei wrote:
> Hi Simon,
> 
> Thanks for your review. Please find my relies below.
> I will sent out v2 based on your review.
> 
> On Tue, Apr 25, 2017 at 12:43 AM, Simon Horman
>  wrote:
> > Hi Yi-Hung,
> >
> > thanks for taking on this difficult piece of work and apologies for the
> > delay in responding.
> >
> > On Mon, Apr 03, 2017 at 04:24:36PM -0700, Yi-Hung Wei wrote:
> >> This commit backports the following upstream commits to fix MPLS GSO in ovs
> >> datapath. It has been tested on kernel 4.4 and 4.9.
> >
> > Thanks also for noting the versions this has been tested against. I expect
> > there testing against other versions will show some residual problems but
> > I think that fixing 4.4 and 4.9 is a good step forwards.
> >
> > I see that this patch backports 4 upstream patches. I am curious to know
> > if you considered backporting them individually. That would have made
> > reviewing a little easier for me.
> Thanks for your suggestion. I pull two independent patches out of this
> patch in v2.
> Commit 48d2ab609b6b("net: mpls: Fixups for GSO") and 85de4a2101ac
> ("openvswitch: use mpls_hdr") are backported together since I am using the
> mpls_hdr() in the later commit to backport some logic in the first commit.

Thanks.

> >> Upstream commit:
> >> commit 48d2ab609b6bbecb7698487c8579bc40de9d6dfa
> >> Author: David Ahern 
> >> Date:   Wed Aug 24 20:10:44 2016 -0700
> >
> > ...
> >
> >> Upstream commit:
> >> commit f7d49bce8e741e1e6aa14ce4db1b6cea7e4be4e8
> >> Author: Jiri Benc 
> >> Date:   Fri Sep 30 19:08:05 2016 +0200
> >
> > ...
> >
> >> Upstream commit:
> >> commit 85de4a2101acb85c3b1dde465e84596ccca99f2c
> >> Author: Jiri Benc 
> >> Date:   Fri Sep 30 19:08:07 2016 +0200
> >>
> >> openvswitch: use mpls_hdr
> >>
> >> skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
> >> instead.
> >>
> >> Signed-off-by: Jiri Benc 
> >> Acked-by: Pravin B Shelar 
> >> Signed-off-by: David S. Miller 
> >
> > ...
> >
> >> Upstream commit:
> >> commit c66549ffd05831abf6cf19ce0571ad868e39
> >> Author: Jiri Benc 
> >> Date:   Wed Oct 5 15:01:57 2016 +0200
> >
> > ...
> >
> >> diff --git a/acinclude.m4 b/acinclude.m4
> >> index 744d8f89525c..82ca4a28015c 100644
> >> --- a/acinclude.m4
> >> +++ b/acinclude.m4
> >> @@ -479,6 +479,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >>  [OVS_GREP_IFELSE([$KSRC/include/net/dst_cache.h], 
> >> [dst_cache],
> >>   
> >> [OVS_DEFINE([USE_UPSTREAM_TUNNEL])])])])
> >>
> >> +  OVS_GREP_IFELSE([$KSRC/include/linux/mpls.h], [mpls_hdr])
> >
> > Should the path be $KSRC/include/net/mpls.h?
> >
> > I am looking at 9095e10edd28 ("mpls: move mpls_hdr to a common location")
> Yes, you're right. Thanks for finding this bug.
> 
> >
> >>OVS_GREP_IFELSE([$KSRC/include/linux/net.h], [sock_create_kern.*net],
> >>[OVS_DEFINE([HAVE_SOCK_CREATE_KERN_NET])])
> >>OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], 
> >> [ndo_fill_metadata_dst])
> >> diff --git a/datapath/actions.c b/datapath/actions.c
> >> index 06080b24ea5a..ecc5136a3529 100644
> >> --- a/datapath/actions.c
> >> +++ b/datapath/actions.c
> >
> > ...
> >
> >> @@ -169,20 +170,26 @@ static int push_mpls(struct sk_buff *skb, struct 
> >> sw_flow_key *key,
> >>   if (skb_cow_head(skb, MPLS_HLEN) < 0)
> >>   return -ENOMEM;
> >>
> >> + if (!ovs_skb_get_inner_protocol(skb)) {
> >> + skb_set_inner_network_header(skb, skb->mac_len);
> >> + ovs_skb_set_inner_protocol(skb, skb->protocol);
> >> + }
> >> +
> >>   skb_push(skb, MPLS_HLEN);
> >>   memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
> >>   skb->mac_len);
> >>   skb_reset_mac_header(skb);
> >> +#ifdef HAVE_MPLS_HDR
> >> + skb_set_network_header(skb, skb->mac_len);
> >> +#endif
> >
> > It is not clear to me why this call to skb_set_network_header() is
> > guarded by HAVE_MPLS_HDR here and moreover not elsewhere in this patch.
> 
> This patch uses HAVE_MPLS_HDR to determine if the kernel that ovs datapath is
> compiled with has the new or the old mpls_gso kernel module. Because
> starting from
> commit 48d2ab609b6b ("net: mpls: Fixups for GSO"), the mpls_gso kernel module
> relies on the fact that skb_network_header() points to the mpls header and
> skb_inner_network_header() points to the L3 header so that it can
> derive the length of
> mpls header correctly. However, the old mpls_gso kernel module assumes that 
> the
> skb_network_header() points to the L3 header, and the old mpls_gso
> kernel module will
> misbehave if the ovs datapath marks the skb_network_header() in the
> new way since it will
> treat