Re: [ovs-dev] [per-pmd ftb/cls V2 1/3] dpif-netdev: Add function to get pmd using core id.

2014-12-29 Thread Pravin Shelar 
On Sat, Dec 20, 2014 at 12:35 AM, Alex Wang  wrote:
> This commit adds the function dp_netdev_get_pmd() which allows
> users to get 'struct dp_netdev_pmd_thread' based on the core id.
>
> Signed-off-by: Alex Wang 
>
LGTM
Acked-by: Pravin B Shelar 
> ---
> PATCH->V2:
> - rebase to master.
> ---
>  lib/dpif-netdev.c |   14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 890870c..336277c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -431,7 +431,8 @@ static void dp_netdev_configure_pmd(struct 
> dp_netdev_pmd_thread *pmd,
>  struct dp_netdev *dp, int index,
>  int core_id, int numa_id);
>  static void dp_netdev_set_nonpmd(struct dp_netdev *dp);
> -static struct dp_netdev_pmd_thread *dp_netdev_get_nonpmd(struct dp_netdev 
> *dp);
> +static struct dp_netdev_pmd_thread *dp_netdev_get_pmd(struct dp_netdev *dp,
> +  int core_id);
>  static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp);
>  static void dp_netdev_del_pmds_on_numa(struct dp_netdev *dp, int numa_id);
>  static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id);
> @@ -2044,10 +2045,10 @@ dpif_netdev_execute(struct dpif *dpif, struct 
> dpif_execute *execute)
>
>  /* Tries finding the 'pmd'.  If NULL is returned, that means
>   * the current thread is a non-pmd thread and should use
> - * dp_netdev_get_nonpmd(). */
> + * dp_netdev_get_pmd(dp, NON_PMD_CORE_ID). */
>  pmd = ovsthread_getspecific(dp->per_pmd_key);
>  if (!pmd) {
> -pmd = dp_netdev_get_nonpmd(dp);
> +pmd = dp_netdev_get_pmd(dp, NON_PMD_CORE_ID);
>  }
>
>  /* If the current thread is non-pmd thread, acquires
> @@ -2241,7 +2242,8 @@ dpif_netdev_run(struct dpif *dpif)
>  {
>  struct dp_netdev_port *port;
>  struct dp_netdev *dp = get_dp_netdev(dpif);
> -struct dp_netdev_pmd_thread *non_pmd = dp_netdev_get_nonpmd(dp);
> +struct dp_netdev_pmd_thread *non_pmd = dp_netdev_get_pmd(dp,
> + 
> NON_PMD_CORE_ID);
>  uint64_t new_tnl_seq;
>
>  ovs_mutex_lock(&dp->non_pmd_mutex);
> @@ -2440,12 +2442,12 @@ dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread 
> *pmd)
>
>  /* Returns the pointer to the dp_netdev_pmd_thread for non-pmd threads. */
>  static struct dp_netdev_pmd_thread *
> -dp_netdev_get_nonpmd(struct dp_netdev *dp)
> +dp_netdev_get_pmd(struct dp_netdev *dp, int core_id)
>  {
>  struct dp_netdev_pmd_thread *pmd;
>  const struct cmap_node *pnode;
>
> -pnode = cmap_find(&dp->poll_threads, hash_int(NON_PMD_CORE_ID, 0));
> +pnode = cmap_find(&dp->poll_threads, hash_int(core_id, 0));
>  ovs_assert(pnode);
>  pmd = CONTAINER_OF(pnode, struct dp_netdev_pmd_thread, node);
>
> --
> 1.7.9.5
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [per-pmd ftb/cls V2 2/3] ovs-numa: Refine the module.

2014-12-29 Thread Pravin Shelar 
On Sat, Dec 20, 2014 at 12:35 AM, Alex Wang  wrote:
> This commit refines the ovs-numa module by eliminating
> duplicated codes and exposing API for dumping the cores
> on numa node.
>
> Signed-off-by: Alex Wang 
>

LGTM
Acked-by: Pravin B Shelar 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [per-pmd ftb/cls V2 3/3] dpif-netdev: Add per-pmd flow-table/classifier.

2014-12-29 Thread Pravin Shelar 
On Sat, Dec 20, 2014 at 12:35 AM, Alex Wang  wrote:
> This commit changes the per dpif-netdev datapath flow-table/
> classifier to per pmd-thread.  As direct benefit, datapath
> and flow statistics no longer need to be protected by mutex
> or be declared as per-thread variable, since they are only
> written by the owning pmd thread.
>
> As side effects, the flow-dump output of userspace datapath
> can contain overlapping flows.  To reduce confusion, the dump
> from different pmd thread will be separated by a title line.
> In addition, the flow operations via 'ovs-appctl dpctl/*'
> are modified so that if the given flow in_port corresponds
> to a dpdk interface, the operation will be conducted to all
> pmd threads recv from that interface.
>
> Signed-off-by: Alex Wang 
>
> ---
> PATCH->V2:
> - replace 'poller_id' with 'pmd_id'.
> - remove unnecessary 'ovs_numa_core_id_is_valid()' check in
>   dpif_netdev_flow_*().
> - inline clear_stats().
> - ovsrcu_postpone the destroy of 'pmd' for thread-safety.
> - rebase to master.

Thanks for all fixes.

LGTM
Acked-by: Pravin B Shelar 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] '27: test atomic operations' unit test hanging

2014-12-29 Thread Ben Pfaff 
On Tue, Dec 23, 2014 at 11:56:40PM +, Sharo, Randall A CIV 
SPAWARSYSCEN-ATLANTIC, 55200 wrote:
> I see this same error, Stephen -- the test gets caught in an
> infinite while loop where the atomic reader waits for an atomic
> writer that is not running (maybe terminated or failed to spawn).  I
> see the error when building on Ubuntu 14.10 and CentOS 7.  I do not
> see it on CentOS 6.6.

Can you give a backtrace?

> I also see the OVS atomic source code cause an internal compiler
> error when using clang as delivered in the Ubuntu 14.10 distro.
> (Compiler says it is unable to determine whether atomic boolean base
> type should be "bool" or "u8").  Some forum threads have claimed
> that recent Ubuntu clang crashes can be a result of a bug where
> clang builds improperly under GCC 4.8.  I tried rebuilding clang
> using GCC 4.7 on Ubuntu but it didn't eliminate the internal error
> when the resultant clang binary compiled OVS atomic code.

By "internal compiler error", do you mean that compiling OVS triggers
a bug in Clang?  I haven't seen this--what version of Clang are you
using?  But you can work around the problem by compiling with GCC
instead.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Warn the free of 'recirc_id' by wrong 'ofproto'.

2014-12-29 Thread Ben Pfaff 
On Tue, Dec 23, 2014 at 02:21:06PM -0800, Alex Wang wrote:
> Issues a ERR log when the 'recirc_id' is not freed by the
> owning 'ofproto'.
> 
> Signed-off-by: Alex Wang 

How did this come up?  (Should this be an assertion?)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] odp-util: Generate MPLS and vlan actions in correct order.

2014-12-29 Thread Pravin B Shelar 
Kernel datapath expect specific order when it comes to mpls and vlan
action combination. Datapath can push mpls first and then vlan and
reverse for pop action. Following patch generate actions in
correct order and enforces this requirement for Open Flow
actions so that user can see rejected actions.

Signed-off-by: Pravin B Shelar 
---
 lib/odp-util.c|   17 +++-
 lib/ofp-actions.c |   10 ++
 tests/mpls-xlate.at   |   20 +
 tests/ofproto-dpif.at |  222 -
 tests/ofproto.at  |   13 +++
 5 files changed, 57 insertions(+), 225 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index d52c172..db74f00 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3961,14 +3961,24 @@ pop_vlan(struct flow *base,
 }
 
 static void
-commit_vlan_action(ovs_be16 vlan_tci, struct flow *base,
-   struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+commit_vlan_pop_action(ovs_be16 vlan_tci, struct flow *base,
+   struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
 if (base->vlan_tci == vlan_tci) {
 return;
 }
 
 pop_vlan(base, odp_actions, wc);
+}
+
+static void
+commit_vlan_push_action(ovs_be16 vlan_tci, struct flow *base,
+   struct ofpbuf *odp_actions)
+{
+if (base->vlan_tci == vlan_tci) {
+return;
+}
+
 if (vlan_tci & htons(VLAN_CFI)) {
 struct ovs_action_push_vlan vlan;
 
@@ -4326,8 +4336,9 @@ commit_odp_actions(const struct flow *flow, struct flow 
*base,
 commit_set_ether_addr_action(flow, base, odp_actions, wc, use_masked);
 slow = commit_set_nw_action(flow, base, odp_actions, wc, use_masked);
 commit_set_port_action(flow, base, odp_actions, wc, use_masked);
+commit_vlan_pop_action(flow->vlan_tci, base, odp_actions, wc);
 commit_mpls_action(flow, base, odp_actions);
-commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
+commit_vlan_push_action(flow->vlan_tci, base, odp_actions);
 commit_set_priority_action(flow, base, odp_actions, wc, use_masked);
 commit_set_pkt_mark_action(flow, base, odp_actions, wc, use_masked);
 
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 4680d81..cdbfa32 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -5463,6 +5463,11 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, 
struct ofpact *a,
 return 0;
 
 case OFPACT_PUSH_MPLS:
+if (flow->vlan_tci & htons(VLAN_CFI)) {
+/* OVS does not support MPLS push after VLAN header */
+return OFPERR_OFPBAC_UNSUPPORTED_ORDER;
+}
+
 flow->dl_type = ofpact_get_PUSH_MPLS(a)->ethertype;
 /* The packet is now MPLS and the MPLS payload is opaque.
  * Thus nothing can be assumed about the network protocol.
@@ -5471,6 +5476,11 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, 
struct ofpact *a,
 return 0;
 
 case OFPACT_POP_MPLS:
+if (flow->vlan_tci & htons(VLAN_CFI)) {
+/* OVS does not support VLAN POP after MPLS pop */
+return OFPERR_OFPBAC_UNSUPPORTED_ORDER;
+}
+
 if (!eth_type_mpls(flow->dl_type)) {
 inconsistent_match(usable_protocols);
 }
diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
index e2ef2e7..0051f54 100644
--- a/tests/mpls-xlate.at
+++ b/tests/mpls-xlate.at
@@ -46,5 +46,25 @@ AT_CHECK([tail -1 stdout], [0],
   [Datapath actions: pop_mpls(eth_type=0x8847),recirc(300)
 ])
 
+dnl Test vlan and MPLS actions
+AT_CHECK([ovs-ofctl del-flows br0])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13  add-flow br0 
in_port=local,dl_type=0x0800,action=push_mpls:0x8847,set_field:10-\>mpls_label,mod_vlan_vid:11,output:1])
+
+dnl incoming
+AT_CHECK([ovs-ofctl  add-flow br0 
dl_type=0x8847,in_port=1,vlan_tci=0x1015,mpls_label=20,action=strip_vlan,pop_mpls:0x0800,output:LOCAL])
+
+dnl Double MPLS push
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'],
 [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 
push_mpls(label=10,tc=0,ttl=64,bos=1,eth_type=0x8847),push_vlan(vid=11,pcp=0),1
+])
+
+dnl Double MPLS pop
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8100),vlan(vid=21,pcp=0),encap(eth_type(0x8847),mpls(label=20/0xf,tc=0/0,ttl=64/0x0,bos=1/1))'],
 [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: pop_vlan,pop_mpls(eth_type=0x800),100
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 43bde4a..2f1a151 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -3187,12 +3187,6 @@ cookie=0xa dl_src=40:44:44:44:54:50 
actions=push_mpls:0x8847,load:10->OXM_OF_MPL
 cookie=0xa dl_src=40:44:44:44:54:51 
actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],pus

Re: [ovs-dev] [PATCH] configure.ac: Enable 'tar-ustar' by default

2014-12-29 Thread Ben Pfaff 
On Fri, Dec 19, 2014 at 10:19:10AM +, Stephen Finucane wrote:
> Automake defaults to the 'v7' legacy tar format in GNU tar, through
> passing of the '-o' parameter to GNU tar. Enabling this option results
> in errors for users with 32 bit UIDs:
> 
> $ make dist
> ...
> tardir=openvswitch-2.3.90 && ${TAR-tar} chof - "$tardir" | GZIP=--best 
> gzip -c >openvswitch-2.3.90.tar.gz
> tar: value 12345678 out of uid_t range 0..2097151
> tar: Exiting with failure status due to previous errors
> make[1]: Leaving directory `/development/ovs'
> ...
> 
> The 'tar-ustar' format is a 1988 POSIX standard that allow longer file
> names and other niceties. It's use is an option in Automake 1.9+.
> Enable this option.
> 
> Signed-off-by: Stephen Finucane 
> Reviewed-by: Mark D. Gray 

Doesn't this affect every program that uses Automake?  Have you
reported it to the Automake mailing list?  Is there an upstream fix?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] cfm: Mark CFM as deprecated.

2014-12-29 Thread Ben Pfaff 
On Thu, Dec 18, 2014 at 01:10:07PM -0800, Ethan Jackson wrote:
> The current implementation is not standards compliant and inferior to
> BFD in every way.  This patch marks it as deprecated so it may be
> removed in the future.
> 
> Signed-off-by: Ethan Jackson 

I wonder whether anyone uses CFM to interoperate with hardware that
only supports CFM.  Does that make any sense?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Prevent revalidation during purge.

2014-12-29 Thread Joe Stringer 
Thanks Alex,

On 23 December 2014 at 19:04, Alex Wang  wrote:
> When 'ovs-appctl revalidator/purge' is called, the main thread
> sweeps and destroys all ukeys and the associated datapath flows.
> If, at the same time, revalidators are dumping those flows from
> datapath, the ukey lookup of dumped flows could fail due to
> deletion by main thread.  This race will also cause the mistaken
> issue of few warnings.

Perhaps we should include the actual warnings in the commit message?

The issue might be better described in terms of the revalidation
cycle, and the main thread modifying udpif state during a phase that
expects no modification. I intend to write up a separate patch to
document these phases to explain the threading model around udpif
access, which will hopefully make bugs like this more shallow.

> @@ -104,6 +104,8 @@ struct udpif {
>
>  struct revalidator *revalidators;  /* Flow revalidators. */
>  size_t n_revalidators;
> +struct ovs_mutex revalidate_mutex; /* Must be taken during revalidation 
> */
> +   /* cycle or during purge. */
>
>  struct latch exit_latch;   /* Tells child threads to exit. */
>

Just floating an idea: Perhaps we could think of this as the flow
manager mutex. It prevents multiple threads from attempting to manage
(delete) the datapath flows at once. Whoever takes the mutex must
ensure that operations on the datapath are mirrored with operations to
the umaps, and vice versa.

> @@ -709,7 +713,7 @@ free_dupcall:
>  }
>
>  static void *
> -udpif_revalidator(void *arg)
> +udpif_revalidator(void *arg) OVS_NO_THREAD_SAFETY_ANALYSIS
>  {
>  /* Used by all revalidators. */
>  struct revalidator *revalidator = arg;

It's a pity that we drop threadsafety analysis in this function to fix
a particular race condition, but I can't come up with a better way.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] How to know if sampled packet was cache miss?

2014-12-29 Thread Ben Pfaff 
On Thu, Dec 18, 2014 at 05:13:47PM -0800, Neil McKee wrote:
> I want the sFlow module to know when a sampled packet was a cache miss in
> the datapath.  That way we can include this information with the sFlow
> sample feed that is sent out,  and applications can analyze the data to see
> what mix of traffic is suffering cache misses.Any ideas on how to add
> this?
> 
> When a cache miss is upcall'd to userspace (from In
> datapath/datapath.c:ovs_dp_process_packet()),   then userspace reacts by
> pushing down operations to create a new flow and send that packet
> (lib/dpif-netlink.c:dpif_operate()).   But if that packet is then sampled
> and upcall'd to sFlow there does not seem to be a flag we can inspect to
> know that it was a cache miss in the first place.
> 
> If the datapath were to include the number of packets seen by the flow so
> far when it upcalls a sample (that number is kept in struct sw_flow in the
> datapath flow cache) then we could infer it was a cache miss when we see
> that number == 1.
> 
> Alternatively,  if we could set a bit somewhere when we pass the cache-miss
> back down to be sent,  so that the bit is preserved and visible if that
> same packet is then sampled,  then we could do it that way.   Trying to
> avoid datapath changes I looked to see if there was a way to set a bit in
> the skbuff that would survive and come back in a sample,  but I'm not sure
> how to go about doing that.
> 
> Or maybe there is another way?

Two approaches come to mind.

One way is to add a bit to struct user_action_cookie's 'sflow' member
that indicates whether the sample was produced by a cache miss.  This
might be tricky because it would mean that the OVS_PACKET_CMD_EXECUTE
and OVS_FLOW_CMD_SET passed to the datapath would need different
actions in the sflow case; until now, they have always been the same.

The other way is to process sflow sample actions entirely in
userspace, without passing them to the datapath at all.  I am not sure
of the best place to do that; I haven't looked.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Warn the free of 'recirc_id' by wrong 'ofproto'.

2014-12-29 Thread Alex Wang 
On Mon, Dec 29, 2014 at 12:47 PM, Ben Pfaff  wrote:

> On Tue, Dec 23, 2014 at 02:21:06PM -0800, Alex Wang wrote:
> > Issues a ERR log when the 'recirc_id' is not freed by the
> > owning 'ofproto'.
> >
> > Signed-off-by: Alex Wang 
>
> How did this come up?  (Should this be an assertion?)
>

Hey Ben,

Since ofprotos of each datapath share one recirc_id pool, here the ofproto
is just used to dereference the 'backer'.  And that's why it is
theoretically
possible to pass in non-owning ofproto as argument. So far, there is no such
use.

I think we should use an assert here with proper comments.  Will send out a
patch soon,

Thanks,
Alex Wang,
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] hash: Add hash_add64().

2014-12-29 Thread Ben Pfaff 
On Wed, Dec 17, 2014 at 10:30:41AM -0800, Jarno Rajahalme wrote:
> Add support for adding 64-bit words to hashes.  This will be used by
> subsequent patches.
> 
> Signed-off-by: Jarno Rajahalme 

I noticed that hash_words64() takes a 64-bit basis (which is not new).
That seems odd because it returns a 32-bit hash value.  Usually the
basis is used for chaining together the hashes of several values, so
usually I expect the basis to be the same size as the hash (both
either 32 or 64 bits, for example).

Assuming that a 64-bit basis does make sense, would you mind adding
parentheses here in hash_words64_inline():
> +hash = basis ^ basis >> 32;
so that it becomes:
   hash = basis ^ (basis >> 32);

Also, again assuming that a 64-bit basis does makes sense, if the
provided basis is really a hash, then a plain XOR as above should be
OK, but if it's some other value provided by the caller, without
necessarily high or evenly distributed entropy, as we occasionally
use, then a better hash function than XOR might be useful.

> +for (i = 0; i < n_words; i++) {
> +hash = hash_add64(hash, p[i]);
> +}
> +return hash_finish(hash, n_words * 8);
>  }

Other than that philosophical issue:
Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ofproto-dpif: Do not allow recirc_id freed by non-owning ofproto.

2014-12-29 Thread Alex Wang 
This commit changes the VLOG_ERR (for warning unmatched ofproto)
in ofproto_dpif_free_recirc_id() to an assert statement, so that
recirc_id is never allowed to be freed by non-owning ofproto.

Suggested-by: Ben Pfaff 
Signed-off-by: Alex Wang 
---
 ofproto/ofproto-dpif.c |7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b5fe73f..c92931d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5459,11 +5459,8 @@ ofproto_dpif_free_recirc_id(struct ofproto_dpif 
*ofproto, uint32_t recirc_id)
 ovs_mutex_unlock(&backer->recirc_mutex);
 recirc_id_free(backer->rid_pool, node->recirc_id);
 
-if (node->ofproto != ofproto) {
-VLOG_ERR("recirc_id %"PRIu32", freed by incorrect ofproto (%s),"
- " expect ofproto (%s)", node->recirc_id, ofproto->up.name,
- node->ofproto->up.name);
-}
+/* 'recirc_id' should never be freed by non-owning 'ofproto'. */
+ovs_assert(node->ofproto == ofproto);
 
 /* RCU postpone the free, since other threads may be referring
  * to 'node' at same time. */
-- 
1.7.9.5

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

Re: [ovs-dev] [PATCH 1/2] hash: Add hash_add64().

2014-12-29 Thread Ben Pfaff 
On Mon, Dec 29, 2014 at 02:12:14PM -0800, Ben Pfaff wrote:
> On Wed, Dec 17, 2014 at 10:30:41AM -0800, Jarno Rajahalme wrote:
> > Add support for adding 64-bit words to hashes.  This will be used by
> > subsequent patches.
> > 
> > Signed-off-by: Jarno Rajahalme 
>
> Other than that philosophical issue:
> Acked-by: Ben Pfaff 

Oh, also this patch breaks the 32-bit build.  Clang:

../ofproto/tunnel.c:479:5: error: bit-field 'build_assert_failed' has 
negative
  width (-1)
BUILD_ASSERT_DECL(sizeof *match % sizeof(uint64_t) == 0);
^
../lib/util.h:55:42: note: expanded from macro 'BUILD_ASSERT_DECL'
extern int (*build_assert(void))[BUILD_ASSERT__(EXPR)]
 ^
../lib/util.h:48:38: note: expanded from macro 'BUILD_ASSERT__'
sizeof(struct { unsigned int build_assert_failed : (EXPR) ? 1 : -1; 
})
 ^
../ofproto/tunnel.c:479:5: error: conflicting types for 'build_assert'
../lib/util.h:55:22: note: expanded from macro 'BUILD_ASSERT_DECL'
extern int (*build_assert(void))[BUILD_ASSERT__(EXPR)]
 ^
../ofproto/ofproto-dpif.h:221:1: note: previous declaration is here
BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
^
../lib/util.h:55:22: note: expanded from macro 'BUILD_ASSERT_DECL'
extern int (*build_assert(void))[BUILD_ASSERT__(EXPR)]
 ^
2 errors generated.
make[2]: *** [ofproto/ofproto_libofproto_la-tunnel.lo] Error 1

or GCC:

../ofproto/tunnel.c: In function 'tnl_hash':
../ofproto/tunnel.c:479:5: error: negative width in bit-field 
'build_assert_failed'
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] miniflow: Use 64-bit data.

2014-12-29 Thread Ben Pfaff 
On Wed, Dec 17, 2014 at 10:30:42AM -0800, Jarno Rajahalme wrote:
> So far the compressed flow data in struct miniflow has been in 32-bit
> words with a 63-bit map, allowing for a maximum size of struct flow of
> 252 bytes.  With the forthcoming Geneve options this is not sufficient
> any more.
> 
> This patch solves the problem by changing the miniflow data to 64-bit
> words, doubling the flow max size to 504 bytes.  Since the word size
> is doubled, there is some loss in compression efficiency.  To counter
> this some of the flow fields have been reordered to keep related
> fields together (e.g., the source and destination IP addresses share
> the same 64-bit word).
> 
> This change should speed up flow data processing on 64-bit CPUs, which
> may help counterbalance the impact of making the struct flow bigger in
> the future.
> 
> Classifier lookup stage boundaries are also changed to 64-bit
> alignment, as the current algorithm depends on each miniflow word to
> not be split between ranges.  This has resulted in new padding (part
> of the 'mpls_lse' field).
> 
> The 'dp_hash' field is also moved to packet metadata to eliminate
> otherwise needed padding there.  This allows the L4 to fit into one
> 64-bit word, and also makes matches on 'dp_hash' more efficient as
> misses can be found already on stage 1.
> 
> Signed-off-by: Jarno Rajahalme 
> Summary:

I don't think we have a Summary: tag.

This seems mostly straightforward.  Are there particular parts you'd
like me to look over carefully?

I have a suggestion for miniflow_extract().  It is getting more
complex over time, and becoming more difficult to understand.  I
wonder whether a simpler approach would be just as fast and easier to
understand.  Suppose that, instead of constructing a miniflow
initially, we instead construct a regular "struct flow" on the stack.
All the zeroing and then later checking for nonzero values is what
drove us earlier to move to building a miniflow directly, so we'd want
to avoid that.  But we can do that by not initializing the flow at
all, and just keeping track in a map of the u32s (or u64s) we've
initialized, and then copying those fields into a miniflow based on
the map we assembled.  (I made the same suggestion in November, but I
didn't get a reply, so I think that you must have missed it in all the
email that you get.)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH trivial 3/3] odp-util: Format UFID hi/lo in correct order.

2014-12-29 Thread Ben Pfaff 
On Mon, Dec 15, 2014 at 03:30:05PM -0800, Joe Stringer wrote:
> Signed-off-by: Joe Stringer 

Seem obviously correct.

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

Re: [ovs-dev] [PATCH trivial 2/3] ovs-dpctl: Minor manpages fixes.

2014-12-29 Thread Ben Pfaff 
On Mon, Dec 15, 2014 at 03:30:04PM -0800, Joe Stringer wrote:
> Signed-off-by: Joe Stringer 

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

Re: [ovs-dev] '27: test atomic operations' unit test hanging

2014-12-29 Thread Sharo, Randall A CIV SPAWARSYSCEN-ATLANTIC, 55200 
Please see the following backtraces for test 27:



(gdb) attach 128246
Attaching to program: ovs/tests/ovstest, process 128246

[...]

(gdb) bt
#0  0x7ffebd820f37 in pthread_join () from /lib64/libpthread.so.0
#1  0x00471ed9 in xpthread_join 
(arg1=arg1@entry=140732040972032, 
arg2=arg2@entry=0x0) at lib/ovs-thread.c:174
#2  0x004121a3 in test_acq_rel () at tests/test-atomic.c:313
#3  test_atomic_main (argc=, argv=) at 
tests/test-atomic.c:393
#4  0x004307ba in run_command 
(argc=argc@entry=1, 
argv=argv@entry=0x7fff9f4f0620, 
commands=)
at lib/command-line.c:115
#5  0x004063cd in main (argc=2, argv=0x7fff9f4f0618) at 
tests/ovstest.c:128

[...]

(gdb) thread 2
[Switching to thread 2 (Thread 0x7ffebacf7700 (LWP 128248))]
#0  atomic_writer (aux_=0x1bd0030) at tests/test-atomic.c:284
284 atomic_read_explicit(&aux->data64, &data, memory_order_acquire);
(gdb) bt
#0  atomic_writer (aux_=0x1bd0030) at tests/test-atomic.c:284
#1  0x004716e1 in ovsthread_wrapper (aux_=) at 
lib/ovs-thread.c:338
#2  0x7ffebd81fdf3 in start_thread () from /lib64/libpthread.so.0
#3  0x7ffebd04301d in clone () from /lib64/libc.so.6

[...]

(gdb) thread 3
[Switching to thread 3 (Thread 0x7ffebb4f8700 (LWP 128247))]
#0  atomic_reader (aux_=0x1bd0030) at tests/test-atomic.c:258
258 atomic_read_explicit(&aux->data64, &data, memory_order_acquire);
(gdb) bt
#0  atomic_reader (aux_=0x1bd0030) at tests/test-atomic.c:258
#1  0x004716e1 in ovsthread_wrapper (aux_=) at 
lib/ovs-thread.c:338
#2  0x7ffebd81fdf3 in start_thread () from /lib64/libpthread.so.0
#3  0x7ffebd04301d in clone () from /lib64/libc.so.6

[...]

(gdb) thread 4
Thread ID 4 not known.
(gdb) quit

[...]

Script done on Mon 29 Dec 2014 05:50:30 PM EST







As for clang, yes, the compiler itself crashes ("internal compiler error") with 
an error message stating that it can't resolve a data type. I'll have to look 
at that another time -- I don't have access to that machine right now.  What I 
can say for certain at this point is it is the latest binary package from the 
Ubuntu 14.10 repos (no ppa's added, no build from source, just "apt-get install 
clang").  Probably 3.5.  I was only using clang to check for warnings not 
produced by gcc, though.  I use gcc normally.



Regards,

   Randy Sharo




From: Ben Pfaff [b...@nicira.com]
Sent: Monday, December 29, 2014 3:39 PM
To: Sharo, Randall A CIV SPAWARSYSCEN-ATLANTIC, 55200
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] '27: test atomic operations' unit test hanging

On Tue, Dec 23, 2014 at 11:56:40PM +, Sharo, Randall A CIV 
SPAWARSYSCEN-ATLANTIC, 55200 wrote:
> I see this same error, Stephen -- the test gets caught in an
> infinite while loop where the atomic reader waits for an atomic
> writer that is not running (maybe terminated or failed to spawn).  I
> see the error when building on Ubuntu 14.10 and CentOS 7.  I do not
> see it on CentOS 6.6.

Can you give a backtrace?

> I also see the OVS atomic source code cause an internal compiler
> error when using clang as delivered in the Ubuntu 14.10 distro.
> (Compiler says it is unable to determine whether atomic boolean base
> type should be "bool" or "u8").  Some forum threads have claimed
> that recent Ubuntu clang crashes can be a result of a bug where
> clang builds improperly under GCC 4.8.  I tried rebuilding clang
> using GCC 4.7 on Ubuntu but it didn't eliminate the internal error
> when the resultant clang binary compiled OVS atomic code.

By "internal compiler error", do you mean that compiling OVS triggers
a bug in Clang?  I haven't seen this--what version of Clang are you
using?  But you can work around the problem by compiling with GCC
instead.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [branch-2.3] ofproto-dpif-upcall: Make handler always call poll_block.

2014-12-29 Thread Alex Wang 
This commit makes handler threads always call poll_block() at
the end of each handling cycle.  If there are upcalls received
in the current cycle, the handler will register to wake up
immediately.  Otherwise, it will wait on both the netlink
socket and the exit latch.

Calling poll_block() at every handling cycle makes sure that
coverage counter stats are always timely attributed, and that
the execution of ovsrcu-postponed events is not held by any
busy handler thread.

Signed-off-by: Alex Wang 
---
 ofproto/ofproto-dpif-upcall.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 8577b0e..193e6b7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -526,8 +526,9 @@ udpif_upcall_handler(void *arg)
 if (!n_upcalls) {
 dpif_recv_wait(udpif->dpif, handler->handler_id);
 latch_wait(&udpif->exit_latch);
-poll_block();
 } else {
+poll_immediate_wake();
+
 handle_upcalls(handler, &misses, upcalls, n_upcalls);
 
 HMAP_FOR_EACH (miss, hmap_node, &misses) {
@@ -539,7 +540,7 @@ udpif_upcall_handler(void *arg)
 ofpbuf_uninit(&upcalls[i].upcall_buf);
 }
 }
-coverage_clear();
+poll_block();
 }
 hmap_destroy(&misses);
 
-- 
1.7.9.5

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

[ovs-dev] [PATCH 0/7] Datapath backports to support 3.18.x, net, net-next

2014-12-29 Thread Thomas Graf 
This series includes several backports which affect the datapath
and brings it closer to upstream. It also allows to build the
datapath against current net and net-next kernels.

Thomas Graf (7):
  datapath: Account for rename to vlan_insert_tag_set_proto()
  datapath: vlan: introduce __vlan_insert_tag helper which does not free
skb
  datapath: move make_writable helper into common code
  datapath: move vlan pop/push functions into common code
  datapath: introduce rtnl ops stub
  datapath: replace remaining users of arch_fast_hash with jhash
  travis: Update build matrix to include latest stable kernels

 .travis.yml   |  17 ++--
 acinclude.m4  |   7 +-
 datapath/actions.c| 120 +-
 datapath/datapath.c   |  11 ++-
 datapath/flow_table.c |   4 +-
 datapath/linux/Modules.mk |   4 -
 datapath/linux/compat/gso.c   |   4 +-
 datapath/linux/compat/hash-x86.c  |  95 
 datapath/linux/compat/hash.c  |  51 ---
 datapath/linux/compat/include/asm/hash.h  |  18 
 datapath/linux/compat/include/linux/hash.h|  44 --
 datapath/linux/compat/include/linux/if_vlan.h |  35 
 datapath/linux/compat/include/linux/skbuff.h  |  15 
 datapath/linux/compat/skbuff-openvswitch.c| 110 +++
 datapath/vport-geneve.c   |   6 +-
 datapath/vport-gre.c  |   6 +-
 datapath/vport-internal_dev.c |  21 -
 datapath/vport-internal_dev.h |   2 +
 18 files changed, 237 insertions(+), 333 deletions(-)
 delete mode 100644 datapath/linux/compat/hash-x86.c
 delete mode 100644 datapath/linux/compat/hash.c
 delete mode 100644 datapath/linux/compat/include/asm/hash.h
 delete mode 100644 datapath/linux/compat/include/linux/hash.h

-- 
1.9.3

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

[ovs-dev] [PATCH 1/7] datapath: Account for rename to vlan_insert_tag_set_proto()

2014-12-29 Thread Thomas Graf 
__vlan_put_tag() was renamed to vlan_insert_tag_set_proto() with
the argument list kept intact.

Upstream: 62749e ("vlan: rename __vlan_put_tag to vlan_insert_tag_set_proto")
Signed-off-by: Thomas Graf 
---
 acinclude.m4  | 1 +
 datapath/actions.c| 2 +-
 datapath/datapath.c   | 2 +-
 datapath/linux/compat/gso.c   | 4 ++--
 datapath/linux/compat/include/linux/if_vlan.h | 6 ++
 datapath/vport-geneve.c   | 6 +++---
 datapath/vport-gre.c  | 6 +++---
 datapath/vport-internal_dev.c | 6 +++---
 8 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 3121b09..05dc112 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -376,6 +376,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
 
   OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [ADD_ALL_VLANS_CMD],
   [OVS_DEFINE([HAVE_VLAN_BUG_WORKAROUND])])
+  OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [vlan_insert_tag_set_proto])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/u64_stats_sync.h], 
[u64_stats_fetch_begin_irq])
 
diff --git a/datapath/actions.c b/datapath/actions.c
index 5a1dbe2..9a49cd5 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -292,7 +292,7 @@ static int push_vlan(struct sk_buff *skb, struct 
sw_flow_key *key,
/* push down current VLAN tag */
current_tag = vlan_tx_tag_get(skb);
 
-   if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
+   if (!vlan_insert_tag_set_proto(skb, skb->vlan_proto, 
current_tag))
return -ENOMEM;
/* Update mac_len for subsequent MPLS actions */
skb->mac_len += VLAN_HLEN;
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 3607170..ebab68c 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -428,7 +428,7 @@ static int queue_userspace_packet(struct datapath *dp, 
struct sk_buff *skb,
if (!nskb)
return -ENOMEM;
 
-   nskb = __vlan_put_tag(nskb, nskb->vlan_proto, 
vlan_tx_tag_get(nskb));
+   nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto, 
vlan_tx_tag_get(nskb));
if (!nskb)
return -ENOMEM;
 
diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
index 5079f79..56f9493 100644
--- a/datapath/linux/compat/gso.c
+++ b/datapath/linux/compat/gso.c
@@ -110,8 +110,8 @@ int rpl_dev_queue_xmit(struct sk_buff *skb)
features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
  NETIF_F_UFO | NETIF_F_FSO);
 
-   skb = __vlan_put_tag(skb, skb->vlan_proto,
-vlan_tx_tag_get(skb));
+   skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+   vlan_tx_tag_get(skb));
if (unlikely(!skb))
return err;
vlan_set_tci(skb, 0);
diff --git a/datapath/linux/compat/include/linux/if_vlan.h 
b/datapath/linux/compat/include/linux/if_vlan.h
index 730175b..e3456ac 100644
--- a/datapath/linux/compat/include/linux/if_vlan.h
+++ b/datapath/linux/compat/include/linux/if_vlan.h
@@ -5,6 +5,10 @@
 #include 
 #include_next 
 
+#ifndef HAVE_VLAN_INSERT_TAG_SET_PROTO
+#define vlan_insert_tag_set_proto __vlan_put_tag
+#endif
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(3,10,0)
 /*
  * The behavior of __vlan_put_tag() has changed over time:
@@ -16,6 +20,8 @@
  *
  *  - In 2.6.29 and later, it adjusts the MAC header pointer only.
  *
+ *  - In 3.19 and later, it was renamed to vlan_insert_tag_set_proto()
+ *
  * This is the version from 2.6.33.  We unconditionally substitute this version
  * to avoid the need to guess whether the version in the kernel tree is
  * acceptable.
diff --git a/datapath/vport-geneve.c b/datapath/vport-geneve.c
index d54101e..86f90f5 100644
--- a/datapath/vport-geneve.c
+++ b/datapath/vport-geneve.c
@@ -404,9 +404,9 @@ static int geneve_send(struct vport *vport, struct sk_buff 
*skb)
}
 
if (vlan_tx_tag_present(skb)) {
-   if (unlikely(!__vlan_put_tag(skb,
-skb->vlan_proto,
-vlan_tx_tag_get(skb {
+   if (unlikely(!vlan_insert_tag_set_proto(skb,
+   skb->vlan_proto,
+   vlan_tx_tag_get(skb 
{
err = -ENOMEM;
skb = NULL;
goto err_free_rt;
diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
index daf7fc3..53df865 100644
--- a/datapath/vport-gre.c
+++ b/datapath/vport-gre.c
@@ -175,9 +175,9 @@ s

[ovs-dev] [PATCH 3/7] datapath: move make_writable helper into common code

2014-12-29 Thread Thomas Graf 
note that skb_make_writable already exists in net/netfilter/core.c
but does something slightly different.

Upstream: e219512 ("net: move make_writable helper into common code")
Signed-off-by: Thomas Graf 
---
 acinclude.m4 |  1 +
 datapath/actions.c   | 39 ++--
 datapath/linux/compat/include/linux/skbuff.h |  5 
 datapath/linux/compat/skbuff-openvswitch.c   | 13 ++
 4 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 444141d..9766bed 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -345,6 +345,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [int.skb_zerocopy(],
   [OVS_DEFINE([HAVE_SKB_ZEROCOPY])])
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [l4_rxhash])
+  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_ensure_writable])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/types.h], [bool],
   [OVS_DEFINE([HAVE_BOOL_TYPE])])
diff --git a/datapath/actions.c b/datapath/actions.c
index 9a49cd5..7f61915 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -122,17 +122,6 @@ static bool is_flow_key_valid(const struct sw_flow_key 
*key)
return !!key->eth.type;
 }
 
-static int make_writable(struct sk_buff *skb, int write_len)
-{
-   if (!pskb_may_pull(skb, write_len))
-   return -ENOMEM;
-
-   if (!skb_cloned(skb) || skb_clone_writable(skb, write_len))
-   return 0;
-
-   return pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
-}
-
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 const struct ovs_action_push_mpls *mpls)
 {
@@ -174,7 +163,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key 
*key,
struct ethhdr *hdr;
int err;
 
-   err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+   err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
if (unlikely(err))
return err;
 
@@ -207,7 +196,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key 
*key,
__be32 *stack;
int err;
 
-   err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+   err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
if (unlikely(err))
return err;
 
@@ -229,7 +218,7 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16 
*current_tci)
struct vlan_hdr *vhdr;
int err;
 
-   err = make_writable(skb, VLAN_ETH_HLEN);
+   err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
if (unlikely(err))
return err;
 
@@ -313,7 +302,7 @@ static int set_eth_addr(struct sk_buff *skb, struct 
sw_flow_key *key,
const struct ovs_key_ethernet *eth_key)
 {
int err;
-   err = make_writable(skb, ETH_HLEN);
+   err = skb_ensure_writable(skb, ETH_HLEN);
if (unlikely(err))
return err;
 
@@ -419,8 +408,8 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key 
*key,
struct iphdr *nh;
int err;
 
-   err = make_writable(skb, skb_network_offset(skb) +
-sizeof(struct iphdr));
+   err = skb_ensure_writable(skb, skb_network_offset(skb) +
+ sizeof(struct iphdr));
if (unlikely(err))
return err;
 
@@ -457,8 +446,8 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key 
*key,
__be32 *saddr;
__be32 *daddr;
 
-   err = make_writable(skb, skb_network_offset(skb) +
-   sizeof(struct ipv6hdr));
+   err = skb_ensure_writable(skb, skb_network_offset(skb) +
+ sizeof(struct ipv6hdr));
if (unlikely(err))
return err;
 
@@ -500,7 +489,7 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key 
*key,
return 0;
 }
 
-/* Must follow make_writable() since that can move the skb data. */
+/* Must follow skb_ensure_writable() since that can move the skb data. */
 static void set_tp_port(struct sk_buff *skb, __be16 *port,
 __be16 new_port, __sum16 *check)
 {
@@ -530,8 +519,8 @@ static int set_udp(struct sk_buff *skb, struct sw_flow_key 
*key,
struct udphdr *uh;
int err;
 
-   err = make_writable(skb, skb_transport_offset(skb) +
-sizeof(struct udphdr));
+   err = skb_ensure_writable(skb, skb_transport_offset(skb) +
+ sizeof(struct udphdr));
if (unlikely(err))
return err;
 
@@ -555,8 +544,8 @@ static int set_tcp(struct sk_buff *skb, struct sw_flow_key 
*key,
struct tcphdr *th;
int err;
 
-   err = make_writable(skb, skb_transport_offset(skb) +
-sizeof(struct tcphdr));
+   err = skb_ensure_writable(skb, skb_transport_offset(skb) +
+

[ovs-dev] [PATCH 4/7] datapath: move vlan pop/push functions into common code

2014-12-29 Thread Thomas Graf 
So it can be used from out of openvswitch code.
Did couple of cosmetic changes on the way, namely variable naming and
adding support for 8021AD proto.

Note on backwards compatability:
Unlike the upstream version, the backport of skb_vlan_push() does not
support translating a hardware accelerated 8021AD tag to software.
This is not a problem though as it preserves existing behaviour.

Upstream: 93515d53 ("net: move vlan pop/push functions into common code")
Signed-off-by: Thomas Graf 
---
 acinclude.m4 |  2 +
 datapath/actions.c   | 83 +++-
 datapath/linux/compat/include/linux/skbuff.h | 10 +++
 datapath/linux/compat/skbuff-openvswitch.c   | 97 
 4 files changed, 119 insertions(+), 73 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 9766bed..2579754 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -346,6 +346,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   [OVS_DEFINE([HAVE_SKB_ZEROCOPY])])
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [l4_rxhash])
   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_ensure_writable])
+  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_vlan_pop])
+  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_vlan_push])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/types.h], [bool],
   [OVS_DEFINE([HAVE_BOOL_TYPE])])
diff --git a/datapath/actions.c b/datapath/actions.c
index 7f61915..0ac6684 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -212,90 +212,29 @@ static int set_mpls(struct sk_buff *skb, struct 
sw_flow_key *key,
return 0;
 }
 
-/* remove VLAN header from packet and update csum accordingly. */
-static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
-{
-   struct vlan_hdr *vhdr;
-   int err;
-
-   err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
-   if (unlikely(err))
-   return err;
-
-   if (skb->ip_summed == CHECKSUM_COMPLETE)
-   skb->csum = csum_sub(skb->csum, csum_partial(skb->data
-   + (2 * ETH_ALEN), VLAN_HLEN, 0));
-
-   vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
-   *current_tci = vhdr->h_vlan_TCI;
-
-   memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
-   __skb_pull(skb, VLAN_HLEN);
-
-   vlan_set_encap_proto(skb, vhdr);
-   skb->mac_header += VLAN_HLEN;
-   /* Update mac_len for subsequent MPLS actions */
-   skb->mac_len -= VLAN_HLEN;
-
-   return 0;
-}
-
 static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 {
-   __be16 tci;
int err;
 
-   if (likely(vlan_tx_tag_present(skb))) {
-   vlan_set_tci(skb, 0);
-   } else {
-   if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
-skb->len < VLAN_ETH_HLEN))
-   return 0;
-
-   err = __pop_vlan_tci(skb, &tci);
-   if (err)
-   return err;
-   }
-   /* move next vlan tag to hw accel tag */
-   if (likely(skb->protocol != htons(ETH_P_8021Q) ||
-  skb->len < VLAN_ETH_HLEN)) {
+   err = skb_vlan_pop(skb);
+   if (vlan_tx_tag_present(skb))
+   invalidate_flow_key(key);
+   else
key->eth.tci = 0;
-   return 0;
-   }
-
-   invalidate_flow_key(key);
-   err = __pop_vlan_tci(skb, &tci);
-   if (unlikely(err))
-   return err;
 
-   __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
-   return 0;
+   return err;
 }
 
 static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
 const struct ovs_action_push_vlan *vlan)
 {
-   if (unlikely(vlan_tx_tag_present(skb))) {
-   u16 current_tag;
-
-   /* push down current VLAN tag */
-   current_tag = vlan_tx_tag_get(skb);
-
-   if (!vlan_insert_tag_set_proto(skb, skb->vlan_proto, 
current_tag))
-   return -ENOMEM;
-   /* Update mac_len for subsequent MPLS actions */
-   skb->mac_len += VLAN_HLEN;
-
-   if (skb->ip_summed == CHECKSUM_COMPLETE)
-   skb->csum = csum_add(skb->csum, csum_partial(skb->data
-   + (2 * ETH_ALEN), VLAN_HLEN, 0));
-
+   if (vlan_tx_tag_present(skb))
invalidate_flow_key(key);
-   } else {
+   else
key->eth.tci = vlan->vlan_tci;
-   }
-   __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & 
~VLAN_TAG_PRESENT);
-   return 0;
+
+   return skb_vlan_push(skb, vlan->vlan_tpid,
+ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
 }
 
 static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
@@ -865,8 +804,6 @@ static int do_execute_actions(struct datapath *dp, struct 
sk_buff *skb,

[ovs-dev] [PATCH 5/7] datapath: introduce rtnl ops stub

2014-12-29 Thread Thomas Graf 
This stub now allows userspace to see IFLA_INFO_KIND for ovs master and
IFLA_INFO_SLAVE_KIND for slave.

Upstream: 5b9e7e16 ("openvswitch: introduce rtnl ops stub")
Signed-off-by: Thomas Graf 
---
 datapath/datapath.c   |  9 -
 datapath/vport-internal_dev.c | 15 +++
 datapath/vport-internal_dev.h |  2 ++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index ebab68c..de912f6 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -2149,10 +2149,14 @@ static int __init dp_init(void)
if (err)
goto error;
 
-   err = ovs_flow_init();
+   err = ovs_internal_dev_rtnl_link_register();
if (err)
goto error_action_fifos_exit;
 
+   err = ovs_flow_init();
+   if (err)
+   goto error_unreg_rtnl_link;
+
err = ovs_vport_init();
if (err)
goto error_flow_exit;
@@ -2179,6 +2183,8 @@ error_vport_exit:
ovs_vport_exit();
 error_flow_exit:
ovs_flow_exit();
+error_unreg_rtnl_link:
+   ovs_internal_dev_rtnl_link_unregister();
 error_action_fifos_exit:
action_fifos_exit();
 error:
@@ -2193,6 +2199,7 @@ static void dp_cleanup(void)
rcu_barrier();
ovs_vport_exit();
ovs_flow_exit();
+   ovs_internal_dev_rtnl_link_unregister();
action_fifos_exit();
 }
 
diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
index 997bb3f..b698f6f 100644
--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -143,6 +143,10 @@ static const struct net_device_ops internal_dev_netdev_ops 
= {
 #endif
 };
 
+static struct rtnl_link_ops internal_dev_link_ops __read_mostly = {
+   .kind = "openvswitch",
+};
+
 static void do_setup(struct net_device *netdev)
 {
ether_setup(netdev);
@@ -153,6 +157,7 @@ static void do_setup(struct net_device *netdev)
netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
netdev->destructor = internal_dev_destructor;
netdev->ethtool_ops = &internal_dev_ethtool_ops;
+   netdev->rtnl_link_ops = &internal_dev_link_ops;
netdev->tx_queue_len = 0;
 
netdev->features = NETIF_F_LLTX | NETIF_F_SG | NETIF_F_FRAGLIST |
@@ -300,3 +305,13 @@ struct vport *ovs_internal_dev_get_vport(struct net_device 
*netdev)
 
return internal_dev_priv(netdev)->vport;
 }
+
+int ovs_internal_dev_rtnl_link_register(void)
+{
+   return rtnl_link_register(&internal_dev_link_ops);
+}
+
+void ovs_internal_dev_rtnl_link_unregister(void)
+{
+   rtnl_link_unregister(&internal_dev_link_ops);
+}
diff --git a/datapath/vport-internal_dev.h b/datapath/vport-internal_dev.h
index 9a7d30e..1b179a1 100644
--- a/datapath/vport-internal_dev.h
+++ b/datapath/vport-internal_dev.h
@@ -24,5 +24,7 @@
 
 int ovs_is_internal_dev(const struct net_device *);
 struct vport *ovs_internal_dev_get_vport(struct net_device *);
+int ovs_internal_dev_rtnl_link_register(void);
+void ovs_internal_dev_rtnl_link_unregister(void);
 
 #endif /* vport-internal_dev.h */
-- 
1.9.3

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

[ovs-dev] [PATCH 6/7] datapath: replace remaining users of arch_fast_hash with jhash

2014-12-29 Thread Thomas Graf 
This patch effectively reverts commit 500f80872645 ("net: ovs: use CRC32
accelerated flow hash if available"), and other remaining arch_fast_hash()
users such as from nfsd via commit 6282cd565553 ("NFSD: Don't hand out
delegations for 30 seconds after recalling them.") where it has been used
as a hash function for bloom filtering.

While we think that these users are actually not much of concern, it has
been requested to remove the arch_fast_hash() library bits that arose
from [1] entirely as per recent discussion [2]. The main argument is that
using it as a hash may introduce bias due to its linearity (see avalanche
criterion) and thus makes it less clear (though we tried to document that)
when this security/performance trade-off is actually acceptable for a
general purpose library function.

Lets therefore avoid any further confusion on this matter and remove it to
prevent any future accidental misuse of it. For the time being, this is
going to make hashing of flow keys a bit more expensive in the ovs case,
but future work could reevaluate a different hashing discipline.

  [1] https://patchwork.ozlabs.org/patch/299369/
  [2] https://patchwork.ozlabs.org/patch/418756/

Upstream: 8754589 ("net: replace remaining users of arch_fast_hash with jhash")
Signed-off-by: Thomas Graf 
---
 acinclude.m4   |  1 -
 datapath/flow_table.c  |  4 +-
 datapath/linux/Modules.mk  |  4 --
 datapath/linux/compat/hash-x86.c   | 95 --
 datapath/linux/compat/hash.c   | 51 
 datapath/linux/compat/include/asm/hash.h   | 18 --
 datapath/linux/compat/include/linux/hash.h | 44 --
 7 files changed, 2 insertions(+), 215 deletions(-)
 delete mode 100644 datapath/linux/compat/hash-x86.c
 delete mode 100644 datapath/linux/compat/hash.c
 delete mode 100644 datapath/linux/compat/include/asm/hash.h
 delete mode 100644 datapath/linux/compat/include/linux/hash.h

diff --git a/acinclude.m4 b/acinclude.m4
index 2579754..10ede83 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -284,7 +284,6 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
 
   OVS_GREP_IFELSE([$KSRC/include/linux/err.h], [ERR_CAST])
   OVS_GREP_IFELSE([$KSRC/include/linux/err.h], [IS_ERR_OR_NULL])
-  OVS_GREP_IFELSE([$KSRC/include/linux/hash.h], [fast_hash_ops])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/etherdevice.h], [eth_hw_addr_random])
   OVS_GREP_IFELSE([$KSRC/include/linux/etherdevice.h], [ether_addr_copy])
diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index ad410fd..2f8f3fb 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
@@ -25,7 +25,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -448,7 +448,7 @@ static u32 flow_hash(const struct sw_flow_key *key, int 
key_start,
/* Make sure number of hash bytes are multiple of u32. */
BUILD_BUG_ON(sizeof(long) % sizeof(u32));
 
-   return arch_fast_hash2(hash_key, hash_u32s, 0);
+   return jhash2(hash_key, hash_u32s, 0);
 }
 
 static int flow_key_start(const struct sw_flow_key *key)
diff --git a/datapath/linux/Modules.mk b/datapath/linux/Modules.mk
index 4b38fd5..a463331 100644
--- a/datapath/linux/Modules.mk
+++ b/datapath/linux/Modules.mk
@@ -6,8 +6,6 @@ openvswitch_sources += \
linux/compat/gre.c \
linux/compat/gso.c \
linux/compat/genetlink-openvswitch.c \
-   linux/compat/hash.c \
-   linux/compat/hash-x86.c \
linux/compat/ip_tunnels_core.c \
linux/compat/netdevice.c \
linux/compat/net_namespace.c \
@@ -17,7 +15,6 @@ openvswitch_sources += \
linux/compat/utils.c
 openvswitch_headers += \
linux/compat/gso.h \
-   linux/compat/include/asm/hash.h \
linux/compat/include/linux/percpu.h \
linux/compat/include/linux/bug.h \
linux/compat/include/linux/compiler.h \
@@ -26,7 +23,6 @@ openvswitch_headers += \
linux/compat/include/linux/err.h \
linux/compat/include/linux/etherdevice.h \
linux/compat/include/linux/flex_array.h \
-   linux/compat/include/linux/hash.h \
linux/compat/include/linux/icmp.h \
linux/compat/include/linux/icmpv6.h \
linux/compat/include/linux/if.h \
diff --git a/datapath/linux/compat/hash-x86.c b/datapath/linux/compat/hash-x86.c
deleted file mode 100644
index ca259b9..000
--- a/datapath/linux/compat/hash-x86.c
+++ /dev/null
@@ -1,95 +0,0 @@
-/*
- * Some portions derived from code covered by the following notice:
- *
- * Copyright (c) 2010-2013 Intel Corporation. All rights reserved.
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *
- *   * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *   * Redistributions in bina

[ovs-dev] [PATCH 7/7] travis: Update build matrix to include latest stable kernels

2014-12-29 Thread Thomas Graf 
Signed-off-by: Thomas Graf 
---
 .travis.yml | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index b91327d..67354c7 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -7,16 +7,17 @@ before_install: ./.travis/prepare.sh
 
 env:
   - OPTS="--disable-ssl"
-  - TESTSUITE=1 KERNEL=3.17.4
-  - KERNEL=3.17.4 DPDK=1 OPTS="--with-dpdk=./dpdk-1.7.1/build"
+  - TESTSUITE=1 KERNEL=3.18.1
   - TESTSUITE=1 OPTS="--enable-shared"
-  - KERNEL=3.17.4 DPDK=1 OPTS="--enable-shared"
+  - KERNEL=3.17.7 DPDK=1 OPTS="--with-dpdk=./dpdk-1.7.1/build"
+  - KERNEL=3.17.7 DPDK=1 OPTS="--enable-shared"
+  - KERNEL=3.17.7
   - KERNEL=3.16.7
-  - KERNEL=3.14.25
-  - KERNEL=3.12.33
-  - KERNEL=3.10.61
-  - KERNEL=3.4.104
-  - KERNEL=2.6.32.64
+  - KERNEL=3.14.27
+  - KERNEL=3.12.35
+  - KERNEL=3.10.63
+  - KERNEL=3.4.105
+  - KERNEL=2.6.32.65
 
 script: ./.travis/build.sh $OPTS
 
-- 
1.9.3

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

Re: [ovs-dev] [PATCH 0/7] Datapath backports to support 3.18.x, net, net-next

2014-12-29 Thread Pravin Shelar 
On Mon, Dec 29, 2014 at 4:19 PM, Thomas Graf  wrote:
> This series includes several backports which affect the datapath
> and brings it closer to upstream. It also allows to build the
> datapath against current net and net-next kernels.
>
> Thomas Graf (7):
>   datapath: Account for rename to vlan_insert_tag_set_proto()
>   datapath: vlan: introduce __vlan_insert_tag helper which does not free
> skb
>   datapath: move make_writable helper into common code
>   datapath: move vlan pop/push functions into common code
>   datapath: introduce rtnl ops stub
>   datapath: replace remaining users of arch_fast_hash with jhash
>   travis: Update build matrix to include latest stable kernels
>
I have not review this yet, but looking at my inbox I do not see
second patch from this series.

>  .travis.yml   |  17 ++--
>  acinclude.m4  |   7 +-
>  datapath/actions.c| 120 
> +-
>  datapath/datapath.c   |  11 ++-
>  datapath/flow_table.c |   4 +-
>  datapath/linux/Modules.mk |   4 -
>  datapath/linux/compat/gso.c   |   4 +-
>  datapath/linux/compat/hash-x86.c  |  95 
>  datapath/linux/compat/hash.c  |  51 ---
>  datapath/linux/compat/include/asm/hash.h  |  18 
>  datapath/linux/compat/include/linux/hash.h|  44 --
>  datapath/linux/compat/include/linux/if_vlan.h |  35 
>  datapath/linux/compat/include/linux/skbuff.h  |  15 
>  datapath/linux/compat/skbuff-openvswitch.c| 110 +++
>  datapath/vport-geneve.c   |   6 +-
>  datapath/vport-gre.c  |   6 +-
>  datapath/vport-internal_dev.c |  21 -
>  datapath/vport-internal_dev.h |   2 +
>  18 files changed, 237 insertions(+), 333 deletions(-)
>  delete mode 100644 datapath/linux/compat/hash-x86.c
>  delete mode 100644 datapath/linux/compat/hash.c
>  delete mode 100644 datapath/linux/compat/include/asm/hash.h
>  delete mode 100644 datapath/linux/compat/include/linux/hash.h
>
> --
> 1.9.3
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev