gmane.network.openvswitch.devel

[PATCH] netdev-bsd: Silence warnings on unimplementedplatform.

Ed Maste — 2013-05-24T12:33:51

netdev_bsd_get_next_hop currently lacks an implementation on FreeBSD, so
its arguments are unused; mark them so.

Signed-off-by: Ed Maste <emaste-h+KGxgPPiopAfugRpC6u6w< at >public.gmane.org>
---
 lib/netdev-bsd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 87ac79f..0fb0057 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
< at >< at > -1226,8 +1226,9 < at >< at > netdev_bsd_convert_kernel_name_to_ovs_name(const char *kernel_name)
 #endif
 
 static int
-netdev_bsd_get_next_hop(const struct in_addr *host, struct in_addr *next_hop,
-                        char **netdev_name)
+netdev_bsd_get_next_hop(const struct in_addr *host OVS_UNUSED,
+                        struct in_addr *next_hop OVS_UNUSED,
+                        char **netdev_name OVS_UNUSED)
 {
 #if defined(__NetBSD__)
     static int seq = 0;

Re: [PATCH v10 4/4] Add packet recirculation

Rajahalme, Jarno (NSN - FI/Espoo — 2013-05-24T08:43:59

Simon,

Here is some initial review of the ofproto-dpif.c changes,

  Jarno

On May 24, 2013, at 10:18 , ext Simon Horman wrote:


If this was 0 by default, and set to a valid recirculation_id when recirculation is to take place, then you might get rid of the bool recirculated?


The comment seems the reverse of the intended meaning. If not then need to elaborate to make this clearer.


s/sero/zero/


s/'recirculation_id'/'recirculation_ids'/


Is this flag needed, as recirculation_id is already non-zero for facets that recirculate packets?


This essentially assumes the IPSEC_MARK bit to be 1. This would work for any bit, so the assert would be unnecessary:

recirculation_id += IPSEC_MARK;


It was a bit surprising from the name of the function that the packet is actually modified. Maybe add a comment above?

...

Maybe compute hash here instead?

...

s/recircualte/recirculate/


"may_recirculate" is not exactly what is meant here. It is more like "recirculate before any further L3+ actions", so maybe rena

[PATCH v10 1/4] Add execute_actions

Simon Horman — 2013-05-24T07:18:35

This moves generic action execution code out of lib/dpif-netedev.c
and into a new file, lib/execute-actions.c.

This is in preparation for using execute_set_action()
in lib/odp-util.c to handle recirculation/

Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ< at >public.gmane.org>

---

v10
* As suggested by Ben Pfaff
- Rename lib/execute-actions.[ch] as lib/odp-execute.[ch]
- Sort headers in alphabetical order
- Minimise includes in lib/odp-execute.h
- Include lib/odp-execute.h immediately after config.h in lib/odp-execute.c

v9
* As suggested by Jesse Gross
- Follow the convention on other code that when passing around
  a void pointer and casting it, the variable named with an underscore
  is the void pointer.
- Move extraction of userspace data into execute_actions().
  Previously it was in the userspace callback passed to execute_actions().
- Remove ovs_assert() calls from execute_actions().
  They seem unnecessary at best.
* Constify key parameter of userspace callback of execute_actions().
  The k

[PATCH v10 2/4] Add set skb_mark,set_priority and tunnel support to execute_set_action

Simon Horman — 2013-05-24T07:18:36

Add set skb_mark support to execute_set_action.
This also adds support for the user-space datapath
to honour such actions if they occur before recirculation,
which will be added by a subsequent patch.

This is in preparation for using execute_set_action()
to handle recirculation.

Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ< at >public.gmane.org>

---

v10
As suggested by Ben Pfaff
- Correct coding style of prototype of tun_key_from_attr
- Remove dubious memset from set_tunnel_action

v9
* Combined the following patches:
  - Add set skb_priority support to execute_set_action
  - Add set skb_mark support to execute_set_action
  - Add set tunnel support to execute_set_action
* As suggested by Jesse Gross
  - Reduce the number of new parameters added to functions,
    in many cases to zero, by using the skb_priority, skb_mark
    and tunnel elements of struct flow.

v7 - v8
* No change

v6
* First post
---
 lib/dpif-netdev.c | 21 +++++++++++++--------
 lib/odp-execute.c | 23 ++++++++++++++++++++---
 li

[PATCH v10 3/4] ofproto-dpif: Use execute_actions toexecute controller actions

Simon Horman — 2013-05-24T07:18:37

Now that execute_actions() is available it can be used as a generic
replacement for special-case action execution in
execute_controller_action().

As suggested by Jesse Gross.

Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ< at >public.gmane.org>

---

v10
* As suggested by Ben Pfaff
- use {} with if statement for OVS_ACTION_ATTR_OUTPUT in execute_actions()
- Do not put required side effects in conditions for ovs_assert();

v9
* First post
---
 lib/odp-execute.c      |  4 +++-
 ofproto/ofproto-dpif.c | 46 +++++++---------------------------------------
 2 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 63403c0..439d961 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
< at >< at > -170,7 +170,9 < at >< at > odp_execute_actions(void *dp, struct ofpbuf *packet, struct flow *key,
 
         switch ((enum ovs_action_attr) type) {
         case OVS_ACTION_ATTR_OUTPUT:
-            output(dp, packet, nl_attr_get_u32(a));
+            if (output) {
+                o

[PATCH v10 0/4] Add packet recirculation

Simon Horman — 2013-05-24T07:18:34

Recirculation is a technique to allow a frame to re-enter
frame processing. This is intended to be used after actions
have been applied to the frame with modify the frame in
some way that makes it possible for richer processing to occur.

An example is and indeed targeted use case is MPLS. If an MPLS frame has an
mpls_pop action applied with the IPv4 ethernet type then it becomes
possible to decode the IPv4 portion of the frame. This may be used to
construct a facet that modifies the IPv4 portion of the frame. This is not
possible prior to the mpls_pop action as the contents of the frame after
the MPLS stack is not known to be IPv4.


Design:

* New recirculation action.

  ovs-vswitchd adds a recirculation action to the end of a list of
  datapath actions for a flow when the actions are truncated because
  insufficient flow match information is available to add the next
  OpenFlow action.  The recirculation action is preceded by an action
  to set the skb_mark to an id which can be used to scope a facet loo

[PATCH v10 4/4] Add packet recirculation

Simon Horman — 2013-05-24T07:18:38

Recirculation is a technique to allow a frame to re-enter
frame processing. This is intended to be used after actions
have been applied to the frame with modify the frame in
some way that makes it possible for richer processing to occur.

An example is and indeed targeted use case is MPLS. If an MPLS frame has an
mpls_pop action applied with the IPv4 ethernet type then it becomes
possible to decode the IPv4 portion of the frame. This may be used to
construct a facet that modifies the IPv4 portion of the frame. This is not
possible prior to the mpls_pop action as the contents of the frame after
the MPLS stack is not known to be IPv4.

Design:
* New recirculation action.

  ovs-vswitchd adds a recirculation action to the end of a list of
  datapath actions for a flow when the actions are truncated because
  insufficient flow match information is available to add the next
  OpenFlow action.  The recirculation action is preceded by an action
  to set the skb_mark to an id which can be used to scope a facet looku

[PATCH net-next v5 1/2] net: Use 16bits for *_headersfields of struct skbuff

Simon Horman — 2013-05-24T07:02:51

In order to mitigate ongoing incresase in the size of struct skbuff
use 16 bit integer offsets rather than pointers for inner_*_headers.

This appears to reduce the size of struct skbuff from 0xd0 to 0xc0
bytes on x86_64 with the following all unset.

CONFIG_XFRM
CONFIG_NF_CONNTRACK
CONFIG_NF_CONNTRACK_MODULE
NET_SKBUFF_NF_DEFRAG_NEEDED
CONFIG_BRIDGE_NETFILTER
CONFIG_NET_SCHED
CONFIG_IPV6_NDISC_NODETYPE
CONFIG_NET_DMA
CONFIG_NETWORK_SECMARK

Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ< at >public.gmane.org>
---
 include/linux/skbuff.h | 119 +++----------------------------------------------
 1 file changed, 6 insertions(+), 113 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2e0ced1..5663e35 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
< at >< at > -509,12 +509,12 < at >< at > struct sk_buff {
 __u32reserved_tailroom;
 };
 
-sk_buff_data_tinner_transport_header;
-sk_buff_data_tinner_network_header;
-sk_buff_data_tinner_mac_header;
-sk_buff_data

[PATCH next-next v4 0/2] MPLS: Add limited GSO support

Simon Horman — 2013-05-24T06:51:33

In the case where a non-MPLS packet is received and an MPLS stack is
added it may well be the case that the original skb is GSO but the
NIC used for transmit does not support GSO of MPLS packets.

The aim of this short series is to provide GSO in software for MPLS packets
whose skbs are GSO.

Change since v4:

Update first patch of the series to use 16 bits for all *_headers
rather than just inner_*_headers

Simon Horman (2):
  net: Use 16bits for *_headers fields of struct skbuff
  MPLS: Add limited GSO support

 include/linux/netdev_features.h |   4 +-
 include/linux/netdevice.h       |   2 +
 include/linux/skbuff.h          | 123 ++++------------------------------------
 net/Kconfig                     |   1 +
 net/Makefile                    |   1 +
 net/core/dev.c                  |   4 ++
 net/core/ethtool.c              |   1 +
 net/ipv4/af_inet.c              |   1 +
 net/ipv4/tcp.c                  |   1 +
 net/ipv4/udp.c                  |   2 +-
 net/ipv6/ip6_offload.c          |   1 +
 net/ipv6/u

Re: [PATCH] Implement duration fields in OpenFlow 1.3 portstats.

Rajahalme, Jarno (NSN - FI/Espoo — 2013-05-24T06:47:34

Ben,

Tested, works, and looks good. I can now see port duration in "ovs-ofctl -O OpenFlow13 dump-ports br0" :-)

  Jarno

On May 24, 2013, at 2:56 , ext Ben Pfaff wrote:

Re: [PATCH v3] gre: Restructure tunneling.

Rajahalme, Jarno (NSN - FI/Espoo — 2013-05-24T06:12:29

Pravin,

Please find some review comments below,

  Jarno

On May 23, 2013, at 23:01 , ext Pravin B Shelar wrote:
...

What is the value of this? You already use skb_is_gso() above.


Add some spaces? Also below.


It seems the csum_err returned via the parameter pointer is ignored by the caller, hence it could be removed. 


Is there a symbol you could use here instead the literal 12?


Spaces...

...

Shouldn't this be 2*ETH_ALEN instead, since that is where VLAN header starts at if there is one?


Are you sure you want to ignore the tx_path and insist on 'true' here? Using 'false' selects a looser check of skb->ip_summed at __skb_gso_segment. We have seen the check implied by 'true' having the kernel fill the disk with non-rate-limited warnings, so this may be a significant choice you are making here.


Could you educate me a bit here and explain shortly why it is necessary to overwrite the IP headers of the segmented skbs?


IMO it would be less surprising if you used 'rpl_ip_local_out' here. Don't know

Re: [PATCH v9 3/6] Add execute_actions

Simon Horman — 2013-05-24T06:00:54

Thanks, I have renamed the files as odp-execute.[ch],
and prefixed the functions with odp_


I'm not sure that I see an advantage in shuffling things around
in that manner at this point.


Thanks, fixed.


Thanks, fixed.


Thanks, fixed.


Sure, I have updated the patch accordingly.

Re: [PATCH v9 4/6] Add set skb_mark, set_priority and tunnel support to execute_set_action

Simon Horman — 2013-05-24T05:51:11

Sorry about that. I believe that the memset() should be removed entirely.
I can only assume it is left over from some debugging I was doing.


Thanks, I will fix that.

Re: [PATCH v9 6/6] ofproto-dpif: Use execute_actions to execute controller actions

Simon Horman — 2013-05-24T05:42:05

Thanks, fixed.


Thanks, I have removed the assert as I'm not entirely convinced of its value.

Re: [PATCH] lib/dpif-netdev.c: Remove redundant calltoflow_extract

Rajahalme, Jarno (NSN - FI/Espoo — 2013-05-24T04:55:10

On May 23, 2013, at 21:21 , ext Alex Wang wrote:

Hey Ben,

Just found, I think there are cases that a whole packet is wanted.

In function "dp_netdev_execute_actions()", there are calls to functions like "push_mpls()" and "pop__mpls()", which requires the packet header pointers (e.g. l2_5, l2 ) in "struct ofpbuf *" be initialized. I think the call to "flow_extract()" here serves to initialize the packet header pointers.

So, it seems a bit confusing to use "flow_extract()" here, since the "struct flow key" is actually initialized in function "dpif_netdev_flow_from_nlattrs()". Also, I want to ask, does it make sense to add a function in ofpbuf.c/h that conducts the initialization of packet header pointers?


I was baffled with similar issue in ofproto-dpif.c (in most cases the work done by flow_extract() is wasted). The issue is that flow_extract() really does two things: it initializes the packet layer pointers and the struct flow. You really need to do pretty much the same work parsing the packet for both

Re: skb_gso_segment() bad offloads warnings fixed?

Rajahalme, Jarno (NSN - FI/Espoo — 2013-05-24T04:41:41

On May 23, 2013, at 20:28 , ext Jesse Gross wrote:

Any chance on backporting this? Obviously it requires Linux 3.9, but it would be nice to have the looser check in the OVS releases that currently go the the various distributions.

  Jarno

Re: [PATCH] OPENFLOW-1.1+: OFPCML_NO_BUFFER is effectivelyalready implemented.

Kyle Mestery (kmestery — 2013-05-24T02:51:08


Makes sense and looks ok.

Re: [PATCH] Document OVS packet buffering, to satisfy an OpenFlow 1.2+ requirement.

Kyle Mestery (kmestery — 2013-05-24T02:49:55


Looks fine to me Ben, the wording is quite clear here.

[PATCH] BFD: Reconfigure BFD on port deletion

Pavithra Ramesh — 2013-05-24T00:31:09


Invoke the function to reconfigure BFD when a port/interface
is deleted.

Signed-off-by: Pavithra Ramesh <paramesh-pghWNbHTmq7QT0dZR+AlfA< at >public.gmane.org>
---
 lib/bfd.c              |    2 +-
 ofproto/ofproto-dpif.c |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/bfd.c b/lib/bfd.c
index 95dad2d..48c0e38 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
< at >< at > -246,7 +246,7 < at >< at > bfd_configure(struct bfd *bfd, const char *name,
         init = true;
     }
 
-    if (!smap_get_bool(cfg, "enable", false)) {
+    if (!cfg || !smap_get_bool(cfg, "enable", false)) {
         if (bfd) {
             hmap_remove(&all_bfds, &bfd->node);
             free(bfd->name);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8e51655..3c337ab 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
< at >< at > -597,6 +597,7 < at >< at > static void port_run(struct ofport_dpif *);
 static void port_run_fast(struct ofport_dpif *);
 static void port_wait(struct ofport_dpif *);
 static int set_cfm(struct ofpo

Re: [xlate 2/4] ofproto-dpif: Move odp_actions from subfacet to facet.

Ethan Jackson — 2013-05-24T00:15:34

It was a bug introduced in this patch.  Basically facet_create()
didn't set may_learn to true.  I removed the statement from the commit
message as it was a bit confusing.


I reduced the stub length to 256 bytes.  If we have 10k facets, that
works out to about 3mb of extra overhead, which seems pretty
reasonable to me.  If we have more then 10k facets, vswitchd will
likely fall over anyways.

I've resent the series with your comments incorporated.

Ethan

[xlate v2 5/6] ofproto-dpif: Rename action_xlate_ctx.

Ethan Jackson — 2013-05-24T00:15:19

This patch changes the name of action_xlate_ctx to xlate_ctx. Aside
from being a bit snappier, it fits more cleanly with structures
added in future patches.

Signed-off-by: Ethan Jackson <ethan-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 ofproto/ofproto-dpif.c |  186 ++++++++++++++++++++++++------------------------
 1 file changed, 92 insertions(+), 94 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e8e3565..cc4abd7 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
< at >< at > -208,8 +208,8 < at >< at > static int set_stp_port(struct ofport *,
 
 static bool ofbundle_includes_vlan(const struct ofbundle *, uint16_t vlan);
 
-struct action_xlate_ctx {
-/* action_xlate_ctx_init() initializes these members. */
+struct xlate_ctx {
+    /* xlate_ctx_init() initializes these members. */
 
     /* The ofproto. */
     struct ofproto_dpif *ofproto;
< at >< at > -262,19 +262,19 < at >< at > struct action_xlate_ctx {
      * resubmit or OFPP_TABLE action didn't find a matching rule.
      *
      * This is n