[ovs-dev] skb_gso_segment() bad offloads warnings fixed?

[Rajahalme, Jarno (NSN - FI/Espoo)]

>From 0aa52d88bdf6bc22fed80beb175a6dfe420dfabd Mon Sep 17 00:00:00 2001
From: Cong Wang mailto:amw...@redhat.com>>
Date: Wed, 6 Feb 2013 14:40:36 -0800
Subject: [PATCH] datapath: adjust skb_gso_segment() for calling in rx path

skb_gso_segment() is almost always called in tx path,
except for openvswitch. It calls this function when
it receives the packet and tries to queue it to user-space.
In this special case, the ->ip_summed check inside
skb_gso_segment() is no longer true, as ->ip_summed value
has different meanings on rx path.

This patch adjusts skb_gso_segment() so that we can at least
avoid such warnings on checksum.

Cc: Jesse Gross mailto:je...@nicira.com>>
Cc: David S. Miller mailto:da...@davemloft.net>>
Signed-off-by: Cong Wang mailto:amw...@redhat.com>>
Signed-off-by: David S. Miller mailto:da...@davemloft.net>>
[jesse: backport to kernels before 3.9 and add to tunnel.c]
Signed-off-by: Jesse Gross mailto:je...@nicira.com>>
---


...

diff --git a/datapath/tunnel.c b/datapath/tunnel.c
index 6193891..7c2560f 100644
--- a/datapath/tunnel.c
+++ b/datapath/tunnel.c
@@ -435,7 +435,7 @@ static struct sk_buff *handle_offloads(struct sk_buff *skb,
if (skb_is_gso(skb)) {
struct sk_buff *nskb;

-   nskb = skb_gso_segment(skb, 0);
+   nskb = __skb_gso_segment(skb, 0, false);
if (IS_ERR(nskb)) {
kfree_skb(skb);
err = PTR_ERR(nskb);
--
1.7.9.5


What is the logic of using bool tx_path = false on this call in tunnel.c, as 
this is called on ovs_tnl_send() which is on the tx path?

Is it so that this avoids the (skb->ip_summed != CHECKSUM_PARTIAL) comparison 
that leads to skb_warn_bad_offload(skb)?

If this is the case, how likely is the new comparison (skb->ip_summed == 
CHECKSUM_NONE) in Linux 3.9 to resolve the problem with these warnings?

We have a case in which an OpenStack (grizzly) controller node is filling the 
disk with these kernel warnings and it would be nice to have this resolved. I 
guess turning off GSO on the nic involved should work as an interim solution, 
but it would be nice to understand what is going on here.

  Jarno

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

[ovs-dev] [PATCH] configure: Add if_mib.h prerequisites.

[Ed Maste]

Signed-off-by: Ed Maste 
---
 configure.ac | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1e23289..91f3835 100644
--- a/configure.ac
+++ b/configure.ac
@@ -63,8 +63,9 @@ AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct 
stat.st_mtimensec],
   [], [], [[#include ]])
 AC_CHECK_MEMBERS([struct ifreq.ifr_flagshigh], [], [], [[#include ]])
 AC_CHECK_FUNCS([mlockall strnlen strsignal getloadavg statvfs getmntent_r])
-AC_CHECK_HEADERS(
-  [mntent.h sys/statvfs.h linux/types.h linux/if_ether.h net/if_mib.h])
+AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h])
+AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include 
+#include ]])
 
 OVS_CHECK_PKIDIR
 OVS_CHECK_RUNDIR
-- 
1.7.11.5

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

[ovs-dev] [PATCH] netdev-bsd: Correct pointer use after refactoring.

[Ed Maste]

Introduced in commit 666afb55e84e9118812de81a75655ec9567b7a5b.

Signed-off-by: Ed Maste 
---
 lib/netdev-bsd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 1301aad..87ac79f 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1705,9 +1705,9 @@ static int
 ifr_get_flags(const struct ifreq *ifr)
 {
 #ifdef HAVE_STRUCT_IFREQ_IFR_FLAGSHIGH
-return (ifr.ifr_flagshigh << 16) | ifr.ifr_flags;
+return (ifr->ifr_flagshigh << 16) | ifr->ifr_flags;
 #else
-return ifr.ifr_flags;
+return ifr->ifr_flags;
 #endif
 }
 
-- 
1.7.11.5

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

Re: [ovs-dev] [PATCH 00/13] NetBSD support

[Ed Maste]

On 23 May 2013 00:05, Ben Pfaff  wrote:
> I have all of these patches ready to apply, after Ed reviewed my changes
> to patch 2.

Apparently I didn't review them well enough; sorry for letting some
minor nits slip through.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] configure: Add if_mib.h prerequisites.

[Ben Pfaff]

On Thu, May 23, 2013 at 10:00:57AM -0400, Ed Maste wrote:
> 
> Signed-off-by: Ed Maste 

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

Re: [ovs-dev] [PATCH] netdev-bsd: Correct pointer use after refactoring.

[Ben Pfaff]

On Thu, May 23, 2013 at 10:16:16AM -0400, Ed Maste wrote:
> Introduced in commit 666afb55e84e9118812de81a75655ec9567b7a5b.
> 
> Signed-off-by: Ed Maste 

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

Re: [ovs-dev] [PATCH 00/13] NetBSD support

[Ben Pfaff]

On Thu, May 23, 2013 at 10:38:11AM -0400, Ed Maste wrote:
> On 23 May 2013 00:05, Ben Pfaff  wrote:
> > I have all of these patches ready to apply, after Ed reviewed my changes
> > to patch 2.
> 
> Apparently I didn't review them well enough; sorry for letting some
> minor nits slip through.

No problem, I applied your fixes.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofp-errors: Add missing copyright and license notice.

[Kyle Mestery (kmestery)]

On May 22, 2013, at 11:33 PM, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 


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

Re: [ovs-dev] [PATCH v2.29] datapath: Add basic MPLS support to kernel

[Jesse Gross]

On Wed, May 22, 2013 at 6:38 PM, Simon Horman  wrote:
> On Tue, May 21, 2013 at 09:07:06AM -0700, Jesse Gross wrote:
>> On Mon, May 20, 2013 at 5:55 PM, Simon Horman  wrote:
>> > On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:
>> >> On Fri, May 17, 2013 at 12:06 AM, Simon Horman  wrote:
>> >> > +static int push_mpls(struct sk_buff *skb,
>> >> > +const struct ovs_action_push_mpls *mpls)
>> >> > +{
>> >> > +   __be32 *new_mpls_lse;
>> >> > +   int err;
>> >> > +
>> >> > +   err = make_writable(skb, skb->mac_len + MPLS_HLEN);
>> >> > +   if (unlikely(err))
>> >> > +   return err;
>> >> > +
>> >> > +   skb_push(skb, MPLS_HLEN);
>> >>
>> >> What happens if there isn't enough headroom?
>> >
>> > Good point. How about this?
>> >
>> > if (unlikely(skb->datahead))
>> > return -EINVAL;
>> > skb_push(skb, MPLS_HLEN);
>>
>> The amount of headroom is an internal kernel property so we can't
>> reject actions on the basis of it. We need to expand the headroom,
>> similar to __vlan_put_tag().
>
> Thanks. I have changed the code around and it is now as follows.
> I am a little unsure if the combination of make_writable() and
> skb_cow_head() is sensible.
>
> err = make_writable(skb, skb->mac_len + MPLS_HLEN);
> if (unlikely(err))
> return err;
>
> if (skb_cow_head(skb, MPLS_HLEN) < 0)
> return -ENOMEM;
>
> skb_push(skb, MPLS_HLEN);

I would reverse the order of make_writable() and skb_cow_head() to
avoid possibly having to reallocate twice. I think you also don't need
to add MPLS_HLEN to the length in make_writable() because that
function is ensuring that existing packet data can be modified and the
MPLS label isn't there yet.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofp-errors: Add missing copyright and license notice.

[Ben Pfaff]

On Thu, May 23, 2013 at 04:28:08PM +, Kyle Mestery (kmestery) wrote:
> On May 22, 2013, at 11:33 PM, Ben Pfaff  wrote:
> > Signed-off-by: Ben Pfaff 
> 
> 
> Looks good.

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

Re: [ovs-dev] [PATCH] lib/dpif-netdev.c: Remove redundant call to flow_extract

[Ben Pfaff]

On Tue, May 21, 2013 at 11:33:58AM -0700, Alex Wang wrote:
> This commit removes the redundant call to flow_extract in dpif_netdev_execute,
> since in the next line, the call to dpif_netdev_flow_from_nlattrs will reset
> the flow variable.
> 
> Signed-off-by: Alex Wang 

I think that the flow_extract() call is here because the
dpif_netdev_execute() interface only requires the caller to pass in
metadata fields (e.g. in_port).  The current caller actually passes in
everything, but the interface doesn't require it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] skb_gso_segment() bad offloads warnings fixed?

[Jesse Gross]

On Thu, May 23, 2013 at 4:49 AM, Rajahalme, Jarno (NSN - FI/Espoo)
 wrote:
> From 0aa52d88bdf6bc22fed80beb175a6dfe420dfabd Mon Sep 17 00:00:00 2001
> From: Cong Wang 
> Date: Wed, 6 Feb 2013 14:40:36 -0800
> Subject: [PATCH] datapath: adjust skb_gso_segment() for calling in rx path
>
> skb_gso_segment() is almost always called in tx path,
> except for openvswitch. It calls this function when
> it receives the packet and tries to queue it to user-space.
> In this special case, the ->ip_summed check inside
> skb_gso_segment() is no longer true, as ->ip_summed value
> has different meanings on rx path.
>
> This patch adjusts skb_gso_segment() so that we can at least
> avoid such warnings on checksum.
>
> Cc: Jesse Gross 
> Cc: David S. Miller 
> Signed-off-by: Cong Wang 
> Signed-off-by: David S. Miller 
> [jesse: backport to kernels before 3.9 and add to tunnel.c]
> Signed-off-by: Jesse Gross 
> ---
>
> ...
>
> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index 6193891..7c2560f 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> @@ -435,7 +435,7 @@ static struct sk_buff *handle_offloads(struct sk_buff
> *skb,
>   if (skb_is_gso(skb)) {
>   struct sk_buff *nskb;
>
> - nskb = skb_gso_segment(skb, 0);
> + nskb = __skb_gso_segment(skb, 0, false);
>   if (IS_ERR(nskb)) {
>   kfree_skb(skb);
>   err = PTR_ERR(nskb);
> --
> 1.7.9.5
>
>
> What is the logic of using bool tx_path = false on this call in tunnel.c, as
> this is called on ovs_tnl_send() which is on the tx path?
>
> Is it so that this avoids the (skb->ip_summed != CHECKSUM_PARTIAL)
> comparison that leads to skb_warn_bad_offload(skb)?
>
> If this is the case, how likely is the new comparison (skb->ip_summed ==
> CHECKSUM_NONE) in Linux 3.9 to resolve the problem with these warnings?
>
> We have a case in which an OpenStack (grizzly) controller node is filling
> the disk with these kernel warnings and it would be nice to have this
> resolved. I guess turning off GSO on the nic involved should work as an
> interim solution, but it would be nice to understand what is going on here.

The upstream commit is not really right - the check should depend on
the origin of the packet, not the location in the networking stack
processing it. Most of the time these are somewhat the same but
obviously when bridging, received packets commonly go through the
transmit path. In the OVS tunnel code, we know that we are doing
bridging so I used the looser check.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] Always update ethertype on mpls_pop

[Ben Pfaff]

On Thu, May 23, 2013 at 10:10:22AM +0900, Simon Horman wrote:
> On Wed, May 15, 2013 at 05:51:15PM +0900, Simon Horman wrote:
> > On Fri, May 03, 2013 at 08:31:58AM +0900, Simon Horman wrote:
> > > On Thu, May 02, 2013 at 12:56:37PM -0700, Ben Pfaff wrote:
> > > > I made the following comment on v1, did you do anything about it?
> > > > 
> > > > > Also, the action parser in ofp-actions.def rejects any pop_mpls action
> > > > > with an MPLS ethertype attached, in two places.  We should fix that.
> > > 
> > > Sorry for not being clearer.
> > > 
> > > As per my note below I have a patch that removes restrictions on
> > > the use of MPLS push and pop. This includes removing the
> > > code that rejects pop_mpls actions with an MPLS ethertype attached.
> > > 
> > > I now have that change working and I will post it shortly.
> > 
> > It wasn't shortly, but I have posted it as
> > 
> > [PATCH] Allow multiple levels of MPLS push and pop
> 
> Hi Ben,
> 
> I'm wondering if you could take a moment to review this.

Based on the devel/mpls-multi-push-and-pop branch it has multiple
prerequisites.  I'm starting to take a look at those.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v9 1/6] dpif-netdev: Constify key parameter of dp_netdev_action_userspace()

[Ben Pfaff]

On Wed, May 22, 2013 at 04:08:05PM +0900, Simon Horman wrote:
> Signed-off-by: Simon Horman 

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

Re: [ovs-dev] [PATCH v9 2/6] dpif-netdev: Move decoding of data out of dp_netdev_output_userspace()

[Ben Pfaff]

On Wed, May 22, 2013 at 04:08:06PM +0900, Simon Horman wrote:
> This is in preparation for making dp_netdev_action_userspace()
> more generic and passing dp_netdev_output_userspace() as a callback.
> In this case it makes sense to decode userdata in generic code.
> 
> Signed-off-by: Simon Horman 

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

Re: [ovs-dev] [PATCH v9 3/6] Add execute_actions

[Ben Pfaff]

Let's use a better name for the new files, say odp-execute.[ch], and
use an appropriately prefixed name for the function,
e.g. odp_actions_execute().  It would also be acceptable to break
action-related code out of odp-util.[ch] and put all of it in a new
odp-actions.[ch].

Some more comments:

On Wed, May 22, 2013 at 04:08:07PM +0900, Simon Horman wrote:
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a4ac23f..1717602 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -37,6 +37,7 @@
>  #include "dummy.h"
>  #include "dynamic-string.h"
>  #include "flow.h"
> +#include "execute-actions.h"
>  #include "hmap.h"

Please keep the #includes in alphabetical order.

>  const struct dpif_class dpif_netdev_class = {
> diff --git a/lib/execute-actions.c b/lib/execute-actions.c
> new file mode 100644
> index 000..d1bbeee
> --- /dev/null
> +++ b/lib/execute-actions.c
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> + * Copyright (c) 2013 Simon Horman
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "execute-actions.h"

#include "execute-actions.h" should go right after #include
, please see CodingStyle.

> +case OVS_KEY_ATTR_ETHERNET:
> +eth_set_src_and_dst(packet,
> +   nl_attr_get_unspec(a, sizeof(struct ovs_key_ethernet)));

Please fix indentation here.

> +#ifndef EXECUTE_ACTIONS_H
> +#define EXECUTE_ACTIONS_H 1
> +
> +#include "flow.h"
> +#include "netlink-protocol.h"
> +#include "ofpbuf.h"

It would be better to minimize the header dependencies, more like
this:

#include 
#include 

struct flow;
struct nlattr;
struct ofpbuf;

> +void
> +execute_actions(void *dp, struct ofpbuf *packet, struct flow *key,
> +const struct nlattr *actions, size_t actions_len,
> +void (*output)(void *dp, struct ofpbuf *packet,
> +   uint32_t out_port),
> +void (*userspace)(void *dp, struct ofpbuf *packet,
> +  const struct flow *key,
> +  const struct nlattr *a));
> +#endif

Thanks,

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

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

[Ben Pfaff]

On Wed, May 22, 2013 at 04:08:08PM +0900, Simon Horman wrote:
> +set_tunnel_action(const struct nlattr *a, struct flow_tnl *tun_key)
> +{
> +enum odp_key_fitness fitness = tun_key_from_attr(a, tun_key);
> +
> +memset(&tun_key, 0, sizeof tun_key);
> +ovs_assert(fitness != ODP_FIT_ERROR);
> +}

I don't understand the above code, at all.  The memset call apparently
means to zero out '*tun_key' (and throw away the conversion work we
just did) but the way it's written it only zeros out the pointer to
'*tun_key'.  Either way, it doesn't make sense.

> +enum odp_key_fitness
> +tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun);

We don't put the return type on a separate line for a prototype, and
the parameter names shouldn't be used in this case, please see
CodingStyle.

Thanks,

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

Re: [ovs-dev] [PATCH v9 5/6] Add packet recirculation

[Ben Pfaff]

On Wed, May 22, 2013 at 04:08:09PM +0900, Simon Horman wrote:
> 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.

I think that Jesse isn't done reviewing this, so I'll leave it to him.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Ben Pfaff]

On Wed, May 22, 2013 at 04:08:10PM +0900, Simon Horman wrote:
> diff --git a/lib/execute-actions.c b/lib/execute-actions.c
> index 5e05dc5..1036ede 100644
> --- a/lib/execute-actions.c
> +++ b/lib/execute-actions.c
> @@ -170,7 +170,8 @@ 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)
> +output(dp, packet, nl_attr_get_u32(a));

Needs {}.

> +ovs_assert(!execute_actions(NULL, packet, &key, ctx->odp_actions->data,
> +ctx->odp_actions->size, NULL, NULL));

Please don't put required side effects in the condition for
ovs_assert().  (Even though there's no way to turn off ovs_assert()
yet, I don't want to rule that out for the future.)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Allow multiple levels of MPLS push and pop

[Ben Pfaff]

On Thu, May 23, 2013 at 09:41:44AM +0900, Simon Horman wrote:
> For reference I have made it available, applied on top of its dependencies
> at:
> 
> git://github.com/horms/openvswitch.git devel/mpls-multi-push-and-pop-v2

Since there are several of them, I think I'd rather wait for the
dependencies to make it in before proceeding with this patch.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] lib/dpif-netdev.c: Remove redundant call to flow_extract

[Alex Wang]

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?

Kind Regards,
Alex Wang


On Thu, May 23, 2013 at 10:08 AM, Ben Pfaff  wrote:

> On Tue, May 21, 2013 at 11:33:58AM -0700, Alex Wang wrote:
> > This commit removes the redundant call to flow_extract in
> dpif_netdev_execute,
> > since in the next line, the call to dpif_netdev_flow_from_nlattrs will
> reset
> > the flow variable.
> >
> > Signed-off-by: Alex Wang 
>
> I think that the flow_extract() call is here because the
> dpif_netdev_execute() interface only requires the caller to pass in
> metadata fields (e.g. in_port).  The current caller actually passes in
> everything, but the interface doesn't require it.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Allow multiple levels of MPLS push and pop

[Jesse Gross]

On Thu, May 23, 2013 at 11:20 AM, Ben Pfaff  wrote:
> On Thu, May 23, 2013 at 09:41:44AM +0900, Simon Horman wrote:
>> For reference I have made it available, applied on top of its dependencies
>> at:
>>
>> git://github.com/horms/openvswitch.git devel/mpls-multi-push-and-pop-v2
>
> Since there are several of them, I think I'd rather wait for the
> dependencies to make it in before proceeding with this patch.

I had the same feeling - I think the execute actions and MPLS kernel
datapath code are both pretty close so I think it makes sense to focus
on those first before moving on.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v3] gre: Restructure tunneling.

[Pravin B Shelar]

Following patch restructures ovs tunneling and gre vport
implementation to make ovs tunneling more in sync with
upstream kernel tunneling.  Doing this tunneling code is
simplified as most of protocol processing on send and
recv is pushed to kernel tunneling.  For external ovs
module the code is moved to kernel compatibility code.

Signed-off-by: Pravin B Shelar 
---
v2-v3:
 - Move rcv packet datapath processing to ip_tunnel code.
 - Use TUNNEL_* flags.
 - better check gre64 vport.
 - return skb if there is no gre port created.

v1-v2:
 - Fix Modules path for ip_tunnels_core.c
---
 datapath/compat.h  |6 +
 datapath/flow.c|   12 +-
 datapath/flow.h|9 +-
 datapath/linux/Modules.mk  |6 +
 datapath/linux/compat/gre.c|  352 
 datapath/linux/compat/gso.c|  166 +++
 datapath/linux/compat/gso.h|   46 +++
 datapath/linux/compat/include/linux/err.h  |9 +
 datapath/linux/compat/include/linux/if_ether.h |4 +
 datapath/linux/compat/include/net/gre.h|  109 
 datapath/linux/compat/include/net/ip_tunnels.h |   54 
 datapath/linux/compat/ip_tunnels_core.c|  115 
 datapath/tunnel.c  |   14 +-
 datapath/tunnel.h  |   11 +-
 datapath/vport-gre.c   |  342 +---
 datapath/vport-lisp.c  |2 +-
 datapath/vport-vxlan.c |2 +-
 17 files changed, 1017 insertions(+), 242 deletions(-)
 create mode 100644 datapath/linux/compat/gre.c
 create mode 100644 datapath/linux/compat/gso.c
 create mode 100644 datapath/linux/compat/gso.h
 create mode 100644 datapath/linux/compat/include/net/gre.h
 create mode 100644 datapath/linux/compat/include/net/ip_tunnels.h
 create mode 100644 datapath/linux/compat/ip_tunnels_core.c

diff --git a/datapath/compat.h b/datapath/compat.h
index c7fd225..6095323 100644
--- a/datapath/compat.h
+++ b/datapath/compat.h
@@ -93,5 +93,11 @@ static inline void skb_set_mark(struct sk_buff *skb, u32 
mark)
skb->mark = mark;
 }
 #endif /* after 2.6.20 */
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,36)
+#define rt_dst(rt) (rt->dst)
+#else
+#define rt_dst(rt) (rt->u.dst)
+#endif
+
 
 #endif /* compat.h */
diff --git a/datapath/flow.c b/datapath/flow.c
index 3ce926e..a4479cd 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1013,7 +1013,7 @@ int ipv4_tun_from_nlattr(const struct nlattr *attr,
switch (type) {
case OVS_TUNNEL_KEY_ATTR_ID:
tun_key->tun_id = nla_get_be64(a);
-   tun_key->tun_flags |= OVS_TNL_F_KEY;
+   tun_key->tun_flags |= TUNNEL_KEY;
break;
case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
tun_key->ipv4_src = nla_get_be32(a);
@@ -1029,10 +1029,10 @@ int ipv4_tun_from_nlattr(const struct nlattr *attr,
ttl = true;
break;
case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT:
-   tun_key->tun_flags |= OVS_TNL_F_DONT_FRAGMENT;
+   tun_key->tun_flags |= TUNNEL_DONT_FRAGMENT;
break;
case OVS_TUNNEL_KEY_ATTR_CSUM:
-   tun_key->tun_flags |= OVS_TNL_F_CSUM;
+   tun_key->tun_flags |= TUNNEL_CSUM;
break;
default:
return -EINVAL;
@@ -1060,7 +1060,7 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
if (!nla)
return -EMSGSIZE;
 
-   if (tun_key->tun_flags & OVS_TNL_F_KEY &&
+   if (tun_key->tun_flags & TUNNEL_KEY &&
nla_put_be64(skb, OVS_TUNNEL_KEY_ATTR_ID, tun_key->tun_id))
return -EMSGSIZE;
if (tun_key->ipv4_src &&
@@ -1073,10 +1073,10 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
return -EMSGSIZE;
if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, tun_key->ipv4_ttl))
return -EMSGSIZE;
-   if ((tun_key->tun_flags & OVS_TNL_F_DONT_FRAGMENT) &&
+   if ((tun_key->tun_flags & TUNNEL_DONT_FRAGMENT) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
return -EMSGSIZE;
-   if ((tun_key->tun_flags & OVS_TNL_F_CSUM) &&
+   if ((tun_key->tun_flags & TUNNEL_CSUM) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_CSUM))
return -EMSGSIZE;
 
diff --git a/datapath/flow.h b/datapath/flow.h
index dba66cf..dfffed7 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -30,7 +30,9 @@
 #include 
 #include 
 #include 
+
 #include 
+#include 
 
 struct sk_buff;
 
@@ -40,11 +42,6 @@ struct sw_flow_actions {
struct nlattr actions[];
 };
 
-/* Tunnel flow flags. */
-#define O

[ovs-dev] [PATCH] ovs-vswitchd: Update documentation of MAC table limits.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 vswitchd/ovs-vswitchd.8.in |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
index 43c5d24..1606b87 100644
--- a/vswitchd/ovs-vswitchd.8.in
+++ b/vswitchd/ovs-vswitchd.8.in
@@ -227,7 +227,9 @@ requires 17 file descriptors per datapath.)
 per bridge due to fixed hash table sizing.
 .
 .IP \(bu
-2,048 MAC learning entries per bridge.
+2,048 MAC learning entries per bridge, by default.  (This is
+configurable via \fBother\-config:mac\-table\-size\fR in the
+\fBBridge\fR table.  See \fBovs\-vswitchd.conf.db\fR(5) for details.)
 .
 .IP \(bu
 Kernel flows are limited only by memory available to the kernel.
-- 
1.7.2.5

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

[ovs-dev] [PATCH] Document OVS packet buffering, to satisfy an OpenFlow 1.2+ requirement.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 OPENFLOW-1.1+  |2 --
 vswitchd/ovs-vswitchd.8.in |   20 
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index b6a4222..975f0d4 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -70,8 +70,6 @@ probably incomplete.
   implement it.  It should be implemented so that the default OVS
   behavior does not change.
 
-* Document how OVS does packet buffering.
-
 * MPLS.  Simon Horman maintains a patch series that adds this
   feature.  This is partially merged.
 
diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
index 43c5d24..4f588cb 100644
--- a/vswitchd/ovs-vswitchd.8.in
+++ b/vswitchd/ovs-vswitchd.8.in
@@ -211,6 +211,26 @@ enabled.
 .so lib/coverage-unixctl.man
 .so lib/stress-unixctl.man
 .
+.SH "OPENFLOW IMPLEMENTATION"
+.
+.PP
+This section documents aspects of OpenFlow for which the OpenFlow
+specification requires documentation.
+.
+.SS "Packet buffering."
+The OpenFlow specification, version 1.2, says:
+.
+.IP
+Switches that implement buffering are expected to expose, through
+documentation, both the amount of available buffering, and the length
+of time before buffers may be reused.
+.
+.PP
+Open vSwitch maintain a separate set of 256 packet buffers for each
+OpenFlow connection.  Any given packet buffer is preserved until it is
+referenced by an \fBOFPT_FLOW_MOD\fR or \fBOFPT_PACKET_OUT\fR request
+or for 5 seconds, whichever comes first.
+.
 .SH "LIMITS"
 .
 .PP
-- 
1.7.2.5

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

[ovs-dev] [PATCH] OPENFLOW-1.1+: OFPCML_NO_BUFFER is effectively already implemented.

[Ben Pfaff]

OpenFlow 1.2 and later define a value of 65535 (OFPCML_NO_BUFFER) for
the max_len field in an output action to mean that the switch should send
the entire packet without buffering it.  Open vSwitch never buffers packets
sent via an output action, so it trivially satisfies this requirement.

Signed-off-by: Ben Pfaff 
---
 OPENFLOW-1.1+ |2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index b6a4222..4bdd37e 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -113,8 +113,6 @@ end of the OF1.2 spec.  I didn't compare the specs 
carefully yet.)
 
 * Update DESIGN to describe OF1.2 behavior also.
 
-* Add ability to turn off packet buffering with OFPCML_NO_BUFFER.
-
 OpenFlow 1.3
 
 
-- 
1.7.2.5

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

Re: [ovs-dev] [PATCH] ovs-vswitchd: Update documentation of MAC table limits.

[Alex Wang]

looks good to me,


On Thu, May 23, 2013 at 3:35 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  vswitchd/ovs-vswitchd.8.in |4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
> index 43c5d24..1606b87 100644
> --- a/vswitchd/ovs-vswitchd.8.in
> +++ b/vswitchd/ovs-vswitchd.8.in
> @@ -227,7 +227,9 @@ requires 17 file descriptors per datapath.)
>  per bridge due to fixed hash table sizing.
>  .
>  .IP \(bu
> -2,048 MAC learning entries per bridge.
> +2,048 MAC learning entries per bridge, by default.  (This is
> +configurable via \fBother\-config:mac\-table\-size\fR in the
> +\fBBridge\fR table.  See \fBovs\-vswitchd.conf.db\fR(5) for details.)
>  .
>  .IP \(bu
>  Kernel flows are limited only by memory available to the kernel.
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovs-xapi-sync: Handle exceptions from XAPI for get_single_bridge_id.

[Gurucharan Shetty]

There are possibilities when records disappear underneath ovs-xapi-sync.
In this particular case, when VLAN network was deleted, the corresponding
record in bridge's external_ids:xs_network_ids column was not deleted by
xenserver.  In situations like that handle the exceptions cleanly.

Bug #17390.
Signed-off-by: Gurucharan Shetty 
---
 .../usr_share_openvswitch_scripts_ovs-xapi-sync|   24 ++--
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync 
b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
index e14b319..ac56d37 100755
--- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
+++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
@@ -98,13 +98,23 @@ def get_single_bridge_id(bridge_ids, default=None):
 return default
 
 for bridge_id in bridge_ids:
-   recs = session.xenapi.network.get_all_records_where('field "uuid"="%s"' 
% bridge_id)
-   if recs:
-   pifs = recs.values()[0]['PIFs']
-   for pif in pifs:
-   rec = session.xenapi.PIF.get_record(pif)
-   if rec['VLAN'] == '-1':
-   return bridge_id
+try:
+recs = session.xenapi.network.get_all_records_where(\
+'field "uuid"="%s"' % bridge_id)
+if recs:
+pifs = recs.values()[0]['PIFs']
+for pif in pifs:
+try:
+rec = session.xenapi.PIF.get_record(pif)
+if rec['VLAN'] == '-1':
+return bridge_id
+except XenAPI.Failure:
+vlog.warn("Could not find XAPI entry for PIF %s" % pif)
+continue
+
+except XenAPI.Failure:
+vlog.warn("Could not find XAPI entry for bridge_id %s" % bridge_id)
+continue
 
 return default
 
-- 
1.7.9.5

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

[ovs-dev] [PATCH] Implement duration fields in OpenFlow 1.3 port stats.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 OPENFLOW-1.1+  |3 ++-
 lib/ofp-print.c|6 ++
 lib/ofp-util.c |   17 +++--
 lib/ofp-util.h |2 ++
 ofproto/ofproto-provider.h |1 +
 ofproto/ofproto.c  |   17 +++--
 tests/ofp-print.at |   39 +++
 7 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index b6a4222..d312dab 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -162,7 +162,8 @@ didn't compare the specs carefully yet.)
 * Rework tag order.  I'm not sure whether we need to do anything
   for this.
 
-* Duration for stats.
+* Duration for queue stats.  (Duration for port stats is already
+  implemented.)
 
 * On-demand flow counters.  I think this might be a real
   optimization in some cases for the software switch.
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index e899df3..0a9917a 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1218,6 +1218,12 @@ ofp_print_ofpst_port_reply(struct ds *string, const 
struct ofp_header *oh,
 print_port_stat(string, "drop=", ps.stats.tx_dropped, 1);
 print_port_stat(string, "errs=", ps.stats.tx_errors, 1);
 print_port_stat(string, "coll=", ps.stats.collisions, 0);
+
+if (ps.duration_sec != UINT32_MAX) {
+ds_put_cstr(string, "   duration=");
+ofp_print_duration(string, ps.duration_sec, ps.duration_nsec);
+ds_put_char(string, '\n');
+}
 }
 }
 
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index b4ff09b..3c8d5a2 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4595,11 +4595,8 @@ ofputil_port_stats_to_ofp13(const struct 
ofputil_port_stats *ops,
 struct ofp13_port_stats *ps13)
 {
 ofputil_port_stats_to_ofp11(ops, &ps13->ps);
-
-/* OF 1.3 adds duration fields */
-/* FIXME: Need to implement port alive duration (sec + nsec) */
-ps13->duration_sec = htonl(~0);
-ps13->duration_nsec = htonl(~0);
+ps13->duration_sec = htonl(ops->duration_sec);
+ps13->duration_nsec = htonl(ops->duration_nsec);
 }
 
 
@@ -4655,6 +4652,7 @@ ofputil_port_stats_from_ofp10(struct ofputil_port_stats 
*ops,
 ops->stats.rx_over_errors = ntohll(get_32aligned_be64(&ps10->rx_over_err));
 ops->stats.rx_crc_errors = ntohll(get_32aligned_be64(&ps10->rx_crc_err));
 ops->stats.collisions = ntohll(get_32aligned_be64(&ps10->collisions));
+ops->duration_sec = ops->duration_nsec = UINT32_MAX;
 
 return 0;
 }
@@ -4683,6 +4681,7 @@ ofputil_port_stats_from_ofp11(struct ofputil_port_stats 
*ops,
 ops->stats.rx_over_errors = ntohll(ps11->rx_over_err);
 ops->stats.rx_crc_errors = ntohll(ps11->rx_crc_err);
 ops->stats.collisions = ntohll(ps11->collisions);
+ops->duration_sec = ops->duration_nsec = UINT32_MAX;
 
 return 0;
 }
@@ -4691,13 +4690,11 @@ static enum ofperr
 ofputil_port_stats_from_ofp13(struct ofputil_port_stats *ops,
   const struct ofp13_port_stats *ps13)
 {
-enum ofperr error =
-ofputil_port_stats_from_ofp11(ops, &ps13->ps);
+enum ofperr error = ofputil_port_stats_from_ofp11(ops, &ps13->ps);
 if (!error) {
-/* FIXME: Get ps13->duration_sec and ps13->duration_nsec,
- * Add to netdev_stats? */
+ops->duration_sec = ntohl(ps13->duration_sec);
+ops->duration_nsec = ntohl(ps13->duration_nsec);
 }
-
 return error;
 }
 
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 4d0d8ad..010c34d 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -689,6 +689,8 @@ bool ofputil_parse_key_value(char **stringp, char **keyp, 
char **valuep);
 struct ofputil_port_stats {
 uint16_t port_no;
 struct netdev_stats stats;
+uint32_t duration_sec;  /* UINT32_MAX if unknown. */
+uint32_t duration_nsec;
 };
 
 struct ofpbuf *ofputil_encode_dump_ports_request(enum ofp_version ofp_version,
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index b9d6f0d..db0d589 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -119,6 +119,7 @@ struct ofport {
 struct ofputil_phy_port pp;
 uint16_t ofp_port;  /* OpenFlow port number. */
 unsigned int change_seq;
+long long int created;  /* Time created, in msec. */
 int mtu;
 };
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ca1dc89..2f91d65 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -203,6 +203,8 @@ static bool handle_openflow(struct ofconn *, struct ofpbuf 
*);
 static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
  const struct ofputil_flow_mod *,
  const struct ofp_header *);
+static void calc_duration(long long int start, long long int now,
+  uint32_t *sec, uint32_t *nsec);
 
 /* ofproto. */
 static uin

Re: [ovs-dev] [PATCH] ovs-xapi-sync: Handle exceptions from XAPI for get_single_bridge_id.

[Ben Pfaff]

On Thu, May 23, 2013 at 04:28:30PM -0700, Gurucharan Shetty wrote:
> There are possibilities when records disappear underneath ovs-xapi-sync.
> In this particular case, when VLAN network was deleted, the corresponding
> record in bridge's external_ids:xs_network_ids column was not deleted by
> xenserver.  In situations like that handle the exceptions cleanly.
> 
> Bug #17390.
> Signed-off-by: Gurucharan Shetty 

Thanks, looks good.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [xlate v2 2/6] ofproto-dpif: Ditch SLOW_MATCH slow path reason.

[Ethan Jackson]

Before this patch, datapath keys with ODP_FIT_TO_LITTLE, would be
assigned subfacets and installed in the kernel with a SLOW_MATCH
slow path reason.  This is problematic, because these flow keys
can't be reliable converted into a 'struct flow' thus breaking a
fundamental assumption of ofproto-dpif.  This patch circumvents the
issue by skipping facet creation for these flows altogether.  This
approach has the added benefit of simplifying the code for future
patches.

Signed-off-by: Ethan Jackson 
---
 lib/odp-util.c |2 --
 lib/odp-util.h |4 
 ofproto/ofproto-dpif.c |   31 ++-
 tests/odp.at   |2 +-
 4 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 5bf94fe..aa50333 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -187,8 +187,6 @@ slow_path_reason_to_string(uint32_t data)
 return "bfd";
 case SLOW_CONTROLLER:
 return "controller";
-case SLOW_MATCH:
-return "match";
 default:
 return NULL;
 }
diff --git a/lib/odp-util.h b/lib/odp-util.h
index a981d17..a2f2b14 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -184,10 +184,6 @@ enum slow_path_reason {
 /* Mutually exclusive with SLOW_BFD, SLOW_CFM, SLOW_LACP, SLOW_STP.
  * Could possibly appear with SLOW_IN_BAND. */
 SLOW_CONTROLLER = 1 << 5,   /* Packets must go to OpenFlow controller. */
-
-/* This can appear on its own, or, theoretically at least, along with any
- * other combination of reasons. */
-SLOW_MATCH = 1 << 6,/* Datapath can't match specifically enough. */
 };
 
 #endif /* odp-util.h */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 280fd57..022c3b8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3806,7 +3806,13 @@ handle_flow_miss(struct flow_miss *miss, struct 
flow_miss_op *ops,
 if (!facet) {
 struct rule_dpif *rule = rule_dpif_lookup(ofproto, &miss->flow);
 
-if (!flow_miss_should_make_facet(ofproto, miss, hash)) {
+/* There does not exist a bijection between 'struct flow' and datapath
+ * flow keys with fitness ODP_FIT_TO_LITTLE.  This breaks a fundamental
+ * assumption used throughout the facet and subfacet handling code.
+ * Since we have to handle these misses in userspace anyway, we simply
+ * skip facet creation, avoiding the problem alltogether. */
+if (miss->key_fitness == ODP_FIT_TOO_LITTLE
+|| !flow_miss_should_make_facet(ofproto, miss, hash)) {
 handle_flow_miss_without_facet(miss, rule, ops, n_ops);
 return;
 }
@@ -5120,19 +5126,16 @@ facet_revalidate(struct facet *facet)
 memset(&ctx, 0, sizeof ctx);
 ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
 LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
-enum slow_path_reason slow;
-
 action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
   &subfacet->initial_vals, new_rule, 0, NULL);
 xlate_actions(&ctx, new_rule->up.ofpacts, new_rule->up.ofpacts_len,
   &odp_actions);
 
-slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
-if (subfacet_should_install(subfacet, slow, &odp_actions)) {
+if (subfacet_should_install(subfacet, ctx.slow, &odp_actions)) {
 struct dpif_flow_stats stats;
 
-subfacet_install(subfacet,
- odp_actions.data, odp_actions.size, &stats, slow);
+subfacet_install(subfacet, odp_actions.data, odp_actions.size,
+ &stats, ctx.slow);
 subfacet_update_stats(subfacet, &stats);
 
 if (!new_actions) {
@@ -5162,7 +5165,7 @@ facet_revalidate(struct facet *facet)
 
 i = 0;
 LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
-subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
+subfacet->slow = ctx.slow;
 
 if (new_actions && new_actions[i].odp_actions) {
 free(subfacet->actions);
@@ -5359,9 +5362,7 @@ subfacet_create(struct facet *facet, struct flow_miss 
*miss,
 subfacet->dp_byte_count = 0;
 subfacet->actions_len = 0;
 subfacet->actions = NULL;
-subfacet->slow = (subfacet->key_fitness == ODP_FIT_TOO_LITTLE
-  ? SLOW_MATCH
-  : 0);
+subfacet->slow = 0;
 subfacet->path = SF_NOT_INSTALLED;
 subfacet->initial_vals = miss->initial_vals;
 subfacet->odp_in_port = miss->odp_in_port;
@@ -5456,7 +5457,7 @@ subfacet_make_actions(struct subfacet *subfacet, const 
struct ofpbuf *packet,
 facet->nf_flow.output_iface = ctx.nf_output_iface;
 facet->mirrors = ctx.mirrors;
 
-subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
+subfacet->slow = ctx.slow;
 if (subfacet->actions_len != odp_actions->size
 || memcmp(subfacet->actions, odp_acti

[ovs-dev] [xlate v2 1/6] ofpbuf: New helper ofpbuf_equal().

[Ethan Jackson]

Used in future commits.

Signed-off-by: Ethan Jackson 
---
 lib/ofpbuf.h |6 ++
 1 file changed, 6 insertions(+)

diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
index 8b03c7e..0c12162 100644
--- a/lib/ofpbuf.h
+++ b/lib/ofpbuf.h
@@ -107,6 +107,12 @@ static inline struct ofpbuf *ofpbuf_from_list(const struct 
list *list)
 }
 void ofpbuf_list_delete(struct list *);
 
+static inline bool
+ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b)
+{
+return a->size == b->size && memcmp(a->data, b->data, a->size) == 0;
+}
+
 #ifdef  __cplusplus
 }
 #endif
-- 
1.7.9.5

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

[ovs-dev] [xlate v2 3/6] ofproto: Ditch SLOW_IN_BAND slow path reason.

[Ethan Jackson]

Before this patch, when in band control was enabled, every DHCP
packet had to be sent to userspace to calculate it's actions.
Those DHCP packets intended for the local port would have a special
action added to ensure they actually make it there.  This
unnecessarily complicates the code, so this patch takes a slightly
different approach.  When in-band is enabled, *all* DHCP packets
must be sent to the local port.  This guarantees that
xlate_actions() returns the same result every time for a given
flow.

Signed-off-by: Ethan Jackson 
---
 lib/odp-util.c |2 --
 lib/odp-util.h |1 -
 ofproto/connmgr.c  |7 -
 ofproto/connmgr.h  |2 --
 ofproto/in-band.c  |   31 -
 ofproto/in-band.h  |2 --
 ofproto/ofproto-dpif.c |   72 
 7 files changed, 23 insertions(+), 94 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index aa50333..4ff1f96 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -181,8 +181,6 @@ slow_path_reason_to_string(uint32_t data)
 return "lacp";
 case SLOW_STP:
 return "stp";
-case SLOW_IN_BAND:
-return "in_band";
 case SLOW_BFD:
 return "bfd";
 case SLOW_CONTROLLER:
diff --git a/lib/odp-util.h b/lib/odp-util.h
index a2f2b14..8a81f89 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -178,7 +178,6 @@ enum slow_path_reason {
 SLOW_CFM = 1 << 0,  /* CFM packets need per-packet processing. */
 SLOW_LACP = 1 << 1, /* LACP packets need per-packet processing. */
 SLOW_STP = 1 << 2,  /* STP packets need per-packet processing. */
-SLOW_IN_BAND = 1 << 3,  /* In-band control needs every packet. */
 SLOW_BFD = 1 << 4,  /* BFD packets need per-packet processing. */
 
 /* Mutually exclusive with SLOW_BFD, SLOW_CFM, SLOW_LACP, SLOW_STP.
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b8cdfa5..0ad96a6 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1643,13 +1643,6 @@ any_extras_changed(const struct connmgr *mgr,
 /* In-band implementation. */
 
 bool
-connmgr_msg_in_hook(struct connmgr *mgr, const struct flow *flow,
-const struct ofpbuf *packet)
-{
-return mgr->in_band && in_band_msg_in_hook(mgr->in_band, flow, packet);
-}
-
-bool
 connmgr_may_set_up_flow(struct connmgr *mgr, const struct flow *flow,
 uint32_t local_odp_port,
 const struct nlattr *odp_actions,
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 48b8119..f0a83c2 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -156,8 +156,6 @@ void connmgr_set_extra_in_band_remotes(struct connmgr *,
 void connmgr_set_in_band_queue(struct connmgr *, int queue_id);
 
 /* In-band implementation. */
-bool connmgr_msg_in_hook(struct connmgr *, const struct flow *,
- const struct ofpbuf *packet);
 bool connmgr_may_set_up_flow(struct connmgr *, const struct flow *,
  uint32_t local_odp_port,
  const struct nlattr *odp_actions,
diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index 1a08fcc..ba6fc54 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -222,37 +222,6 @@ refresh_local(struct in_band *ib)
 return true;
 }
 
-/* Returns true if 'packet' should be sent to the local port regardless
- * of the flow table. */
-bool
-in_band_msg_in_hook(struct in_band *in_band, const struct flow *flow,
-const struct ofpbuf *packet)
-{
-/* Regardless of how the flow table is configured, we want to be
- * able to see replies to our DHCP requests. */
-if (flow->dl_type == htons(ETH_TYPE_IP)
-&& flow->nw_proto == IPPROTO_UDP
-&& flow->tp_src == htons(DHCP_SERVER_PORT)
-&& flow->tp_dst == htons(DHCP_CLIENT_PORT)
-&& packet->l7) {
-struct dhcp_header *dhcp;
-
-dhcp = ofpbuf_at(packet, (char *)packet->l7 - (char *)packet->data,
- sizeof *dhcp);
-if (!dhcp) {
-return false;
-}
-
-refresh_local(in_band);
-if (!eth_addr_is_zero(in_band->local_mac)
-&& eth_addr_equals(dhcp->chaddr, in_band->local_mac)) {
-return true;
-}
-}
-
-return false;
-}
-
 /* Returns true if the rule that would match 'flow' with 'actions' is
  * allowed to be set up in the datapath. */
 bool
diff --git a/ofproto/in-band.h b/ofproto/in-band.h
index 71de6ff..4f52e00 100644
--- a/ofproto/in-band.h
+++ b/ofproto/in-band.h
@@ -39,8 +39,6 @@ void in_band_set_remotes(struct in_band *,
 bool in_band_run(struct in_band *);
 void in_band_wait(struct in_band *);
 
-bool in_band_msg_in_hook(struct in_band *, const struct flow *,
- const struct ofpbuf *packet);
 bool in_band_rule_check(const struct flow *, uint32_t local_odp_port,
 const struct nlattr *odp_act

[ovs-dev] [xlate v2 4/6] ofproto-dpif: Move odp_actions from subfacet to facet.

[Ethan Jackson]

Upon close inspection, it appears that it's not possible for
actions to differ between subfacets belonging to a given facet.
Given this fact, it makes sense to move datapath actions from
subfacets to their parent facets.  It's both conceptually more
straightforward, and necessary for future threading and megaflow
work.

Co-authored-by: Justin Pettit 
Signed-off-by: Ethan Jackson 
---
 ofproto/ofproto-dpif.c |  413 
 1 file changed, 138 insertions(+), 275 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f42101d..e8e3565 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -364,8 +364,6 @@ enum subfacet_path {
 SF_SLOW_PATH,   /* Send-to-userspace action is installed. */
 };
 
-static const char *subfacet_path_to_string(enum subfacet_path);
-
 /* A dpif flow and actions associated with a facet.
  *
  * See also the large comment on struct facet. */
@@ -385,19 +383,8 @@ struct subfacet {
 uint64_t dp_packet_count;   /* Last known packet count in the datapath. */
 uint64_t dp_byte_count; /* Last known byte count in the datapath. */
 
-/* Datapath actions.
- *
- * These should be essentially identical for every subfacet in a facet, but
- * may differ in trivial ways due to VLAN splinters. */
-size_t actions_len; /* Number of bytes in actions[]. */
-struct nlattr *actions; /* Datapath actions. */
-
-enum slow_path_reason slow; /* 0 if fast path may be used. */
 enum subfacet_path path;/* Installed in datapath? */
 
-/* Initial values of the packet that may be needed later. */
-struct initial_vals initial_vals;
-
 /* Datapath port the packet arrived on.  This is needed to remove
  * flows for ports that are no longer part of the bridge.  Since the
  * flow definition only has the OpenFlow port number and the port is
@@ -422,15 +409,11 @@ static void subfacet_reset_dp_stats(struct subfacet *,
 static void subfacet_update_time(struct subfacet *, long long int used);
 static void subfacet_update_stats(struct subfacet *,
   const struct dpif_flow_stats *);
-static void subfacet_make_actions(struct subfacet *,
-  const struct ofpbuf *packet);
 static int subfacet_install(struct subfacet *,
-const struct nlattr *actions, size_t actions_len,
-struct dpif_flow_stats *, enum slow_path_reason);
+const struct ofpbuf *odp_actions,
+struct dpif_flow_stats *);
 static void subfacet_uninstall(struct subfacet *);
 
-static enum subfacet_path subfacet_want_path(enum slow_path_reason);
-
 /* An exact-match instantiation of an OpenFlow flow.
  *
  * A facet associates a "struct flow", which represents the Open vSwitch
@@ -483,18 +466,21 @@ struct facet {
 struct netflow_flow nf_flow; /* Per-flow NetFlow tracking data. */
 uint8_t tcp_flags;   /* TCP flags seen for this 'rule'. */
 
-/* Properties of datapath actions.
- *
- * Every subfacet has its own actions because actions can differ slightly
- * between splintered and non-splintered subfacets due to the VLAN tag
- * being initially different (present vs. absent).  All of them have these
- * properties in common so we just store one copy of them here. */
 bool has_learn;  /* Actions include NXAST_LEARN? */
 bool has_normal; /* Actions output to OFPP_NORMAL? */
 bool has_fin_timeout;/* Actions include NXAST_FIN_TIMEOUT? */
 tag_type tags;   /* Tags that would require revalidation. */
 mirror_mask_t mirrors;   /* Bitmap of dependent mirrors. */
 
+/* Datapath actions. */
+struct ofpbuf odp_actions;
+uint64_t odp_actions_stub[256 / 8];
+
+/* Initial values of the packet that may be needed later. */
+struct initial_vals initial_vals;
+
+enum slow_path_reason slow; /* 0 if fast path may be used. */
+
 /* Storage for a single subfacet, to reduce malloc() time and space
  * overhead.  (A facet always has at least one subfacet and in the common
  * case has exactly one subfacet.  However, 'one_subfacet' may not
@@ -505,8 +491,7 @@ struct facet {
 long long int learn_rl;  /* Rate limiter for facet_learn(). */
 };
 
-static struct facet *facet_create(struct rule_dpif *,
-  const struct flow *, uint32_t hash);
+static struct facet *facet_create(const struct flow_miss *, uint32_t hash);
 static void facet_remove(struct facet *);
 static void facet_free(struct facet *);
 
@@ -526,8 +511,6 @@ static void facet_learn(struct facet *);
 static void facet_account(struct facet *);
 static void push_all_stats(void);
 
-static struct subfacet *facet_get_subfacet(struct facet *);
-
 static bool facet_is_controller_flow(struct facet *);
 
 struct ofport_dpif {
@

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

[Ethan Jackson]

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 
---
 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
@@ -208,8 +208,8 @@ 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;
@@ -262,19 +262,19 @@ struct action_xlate_ctx {
  * resubmit or OFPP_TABLE action didn't find a matching rule.
  *
  * This is normally null so the client has to set it manually after
- * calling action_xlate_ctx_init(). */
-void (*resubmit_hook)(struct action_xlate_ctx *, struct rule_dpif *rule);
+ * calling xlate_ctx_init(). */
+void (*resubmit_hook)(struct xlate_ctx *, struct rule_dpif *rule);
 
 /* If nonnull, flow translation calls this function to report some
  * significant decision, e.g. to explain why OFPP_NORMAL translation
  * dropped a packet. */
-void (*report_hook)(struct action_xlate_ctx *, const char *s);
+void (*report_hook)(struct xlate_ctx *, const char *s);
 
 /* If nonnull, flow translation credits the specified statistics to each
  * rule reached through a resubmit or OFPP_TABLE action.
  *
  * This is normally null so the client has to set it manually after
- * calling action_xlate_ctx_init(). */
+ * calling xlate_ctx_init(). */
 const struct dpif_flow_stats *resubmit_stats;
 
 /* xlate_actions() initializes and uses these members.  The client might want
@@ -317,18 +317,17 @@ struct initial_vals {
 ovs_be16 vlan_tci;
 };
 
-static void action_xlate_ctx_init(struct action_xlate_ctx *,
-  struct ofproto_dpif *, const struct flow *,
-  const struct initial_vals *initial_vals,
-  struct rule_dpif *,
-  uint8_t tcp_flags, const struct ofpbuf *);
-static void xlate_actions(struct action_xlate_ctx *,
-  const struct ofpact *ofpacts, size_t ofpacts_len,
-  struct ofpbuf *odp_actions);
-static void xlate_actions_for_side_effects(struct action_xlate_ctx *,
+static void xlate_ctx_init(struct xlate_ctx *, struct ofproto_dpif *,
+   const struct flow *,
+   const struct initial_vals *initial_vals,
+   struct rule_dpif *, uint8_t tcp_flags,
+   const struct ofpbuf *);
+static void xlate_actions(struct xlate_ctx *, const struct ofpact *ofpacts,
+  size_t ofpacts_len, struct ofpbuf *odp_actions);
+static void xlate_actions_for_side_effects(struct xlate_ctx *,
const struct ofpact *ofpacts,
size_t ofpacts_len);
-static void xlate_table_action(struct action_xlate_ctx *, uint16_t in_port,
+static void xlate_table_action(struct xlate_ctx *, uint16_t in_port,
uint8_t table_id, bool may_packet_in);
 
 static size_t put_userspace_action(const struct ofproto_dpif *,
@@ -343,7 +342,7 @@ static void compose_slow_path(const struct ofproto_dpif *, 
const struct flow *,
   const struct nlattr **actionsp,
   size_t *actions_lenp);
 
-static void xlate_report(struct action_xlate_ctx *ctx, const char *s);
+static void xlate_report(struct xlate_ctx *ctx, const char *s);
 
 /* A subfacet (see "struct subfacet" below) has three possible installation
  * states:
@@ -816,7 +815,7 @@ static size_t compose_sflow_action(const struct 
ofproto_dpif *,
 static void compose_ipfix_action(const struct ofproto_dpif *,
  struct ofpbuf *odp_actions,
  const struct flow *);
-static void add_mirror_actions(struct action_xlate_ctx *ctx,
+static void add_mirror_actions(struct xlate_ctx *ctx,
const struct flow *flow);
 /* Global variables. */
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -3653,7 +3652,7 @@ handle_flow_miss_without_facet(struct flow_miss *miss,
 {
 struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
 long long int now = time_msec();
-struct action_xlate_ctx ctx;
+struct xlate_ctx ctx;
 struct ofpbuf *packet;
 
 LIST_FOR_EACH (packet, list_node, &miss->packets) {
@@ -3668,8 +3667,8 @@ handle_flow_miss_withou

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

[Ethan Jackson]

> Would you describe it a little?  It's a big patch so the fix isn't
> necessarily obvious (I haven't read on yet).  Is it something that we
> should break out separately so that we can backport it?

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.

> The new odp_actions_stub in struct facet makes facets huge!  This
> should not be necessary.  I'd use a much smaller stub there, if we
> need one at all.  I would consider an approach like the following.

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
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] BFD: Reconfigure BFD on port deletion

[Pavithra Ramesh]

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

Signed-off-by: Pavithra Ramesh 
---
 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
@@ -246,7 +246,7 @@ 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
@@ -597,6 +597,7 @@ 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 ofport *, const struct cfm_settings *);
+static int set_bfd(struct ofport *, const struct smap *);
 static void ofport_clear_priorities(struct ofport_dpif *);
 static void run_fast_rl(void);
 
@@ -1859,6 +1860,7 @@ port_destruct(struct ofport *port_)
 ofproto->backer->need_revalidate = REV_RECONFIGURE;
 bundle_remove(port_);
 set_cfm(port_, NULL);
+set_bfd(port_, NULL);
 if (ofproto->sflow) {
 dpif_sflow_del_port(ofproto->sflow, port->odp_port);
 }
-- 
1.7.5.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH next-next v4 1/2] net: Use 16bits for inner_*_headers fields of struct skbuff

[David Miller]

From: Simon Horman 
Date: Wed, 22 May 2013 15:57:15 +0900

> @@ -509,9 +509,9 @@ struct sk_buff {
>   __u32   reserved_tailroom;
>   };
>  
> - sk_buff_data_t  inner_transport_header;
> - sk_buff_data_t  inner_network_header;
> - sk_buff_data_t  inner_mac_header;
> + __u16   inner_transport_header;
> + __u16   inner_network_header;
> + __u16   inner_mac_header;
>   sk_buff_data_t  transport_header;
>   sk_buff_data_t  network_header;
>   sk_buff_data_t  mac_header;

As Pekka suggested, let's try to convert them allto __u16.

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

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

[Kyle Mestery (kmestery)]

On May 23, 2013, at 5:51 PM, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 


Looks fine to me Ben, the wording is quite clear here.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] OPENFLOW-1.1+: OFPCML_NO_BUFFER is effectively already implemented.

[Kyle Mestery (kmestery)]

On May 23, 2013, at 6:08 PM, Ben Pfaff  wrote:
> OpenFlow 1.2 and later define a value of 65535 (OFPCML_NO_BUFFER) for
> the max_len field in an output action to mean that the switch should send
> the entire packet without buffering it.  Open vSwitch never buffers packets
> sent via an output action, so it trivially satisfies this requirement.
> 
> Signed-off-by: Ben Pfaff 


Makes sense and looks ok.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2.30] datapath: Add basic MPLS support to kernel

[Simon Horman]

Allow datapath to recognize and extract MPLS labels into flow keys
and execute actions which push, pop, and set labels on packets.

Based heavily on work by Leo Alterman and Ravi K.

Cc: Ravi K 
Cc: Leo Alterman 
Reviewed-by: Isaku Yamahata 
Signed-off-by: Simon Horman 

---

This is the remaining patch of the series "MPLS actions and matches".
To aid review it is available in git at:

git://github.com/horms/openvswitch.git devel/mpls-v2.30

TODO:
* Finalise kernel network core side of GSO. That will likely require
  minor updates to this patch.

v2.30
* As suggested by Jesse Gross
  - Use skb_cow_head in push_mpls to ensure there is sufficient headroom for
skb_push
  - Call make_writable with skb->mac_len instead of skb->mac_len + MPLS_HLEN
in push_mpls as only the first skb->mac_len bytes of existing packet data
are modified.
  - Rename skb_mac_header_end as mac_header_end, this seems
to be a more appropriate name for a local function.
  - Remove OVS_CSUM_COMPLETE code from set_ethertype().
Inside OVS the ethernet header is not covered by OVS_CSUM_COMPLETE.
  - Use __skb_pull() instead of skb_pull() in pop_mpls()
  - Decrement and decrement skb->mac_len when poping and pushing VLAN tags.
Previously mac_len was reset, but this would result in forgetting
the MPLS label stack.
  - Remove spurious comment from before do_execute_actions().
  - Move OVS_KEY_ATTR_MPLS attribute to its final, upstreamable, location.
  - Correct ethertype check for OVS_ACTION_ATTR_POP_MPLS case in
validate_and_copy_actions() to check for MPLS ethertypes rather than
ETH_P_IP.
  - Rewrite tracking of eth types used to verify actions in the presence
of sample actions. There is a large comment above struct eth_types
describing the new implementation.

v2.29
* Break include/ and lib/ portions of the patch out into a
  separate patch "datapath: Add basic MPLS support to kernel"
* Update for new MPLS GSO scheme
  - skb->protocol is set to the new ethertype of the packet
on MPLS push and pop
  - When pushing the first MPLS LSE onto a previously non-MPLS
packet set skb->inner_protocol to the original ethertype.
  - skb->inner_protocol may be used by the network stack
for GSO of the inner-packet.
* Drop const from ethertype parameter of set_ethertype.
  This appears to be a legacy of this parameter being a pointer.
* Pass the ethertype patrameter of pop_mpls as a value rather
  than a pointer.

v2.28
* Kernel Datapath changes as suggested by Jarno Rajahalme
  + Correct the logic introduced in v2.27 to set the network_header
to after the MPLS label stack in the case of an MPLS packet.
- Increment stack_len offset so that label stacks of depth greater
  than two do not cause an infinite loop.
- Correct offset passed to check_header to include skb->mac len

v2.27
* Kernel Datapath changes as suggested by Jarno Rajahalme and Jesse Gross:
  + Previously the mac_len and network_header of an skb corresponded
to the end of the L2 header.  To support GSO, just before transmission,
do_output, with the results as follows:

Input: non-MPLS skb: Output: network header and mac_len correspond
 to the beginning of the L3 headers
Input: MPLS: Output: network header and mac_len correspond to the
 end of the L2 headers.

This is somewhat confusing.

  + The new scheme is as follows:
- The mac_len always corresponds to the end of the L2 header.
- The network header always corresponds to the beginning of the
  L3 header.

  + Note that in the case of MPLS output the end of the L2 headers and the
  beginning of the L3 headers will differ.

* Remove unused declaration of skb_cb_mpls_stack()

v2.26
* Rebase on master
* Kernel Datapath changes as suggested by Jarno Rajahalme
  - Use skb_network_header() instead of skb_mac_header() to locate
the ethertype to set in set_ethertype() as the latter will
be wrong in the presence of VLAN tags. This resolves
a regression introduced in v2.24.
  - Enhance comment in do_output()
  - do_execute_actions(): Do not alter mpls_stack_depth if
a MPLS push or pop action fail. This is achieved by altering
mpls_stack_depth at the end of push_mpls() and pop_mpls().

v2.25
* Rebase on master
* Pass big-endian value as the last argument of eth_types_set() in
  validate_and_copy_actions__()
* Use revised GSO support as provided by the patch series
  "[PATCH 0/2] Small Modifications to GSO to allow segmentation of MPLS"
  - Set skb->mac_len to the length of the l2 header + MPLS stack length
  - Update skb->network_header accordingly
  - Set skb->encapsulated_features

v2.24
* Use skb_mac_header() in set_ethertype()
* Set skb->encapsulation in set_ethertype() to support MPLS GSO.
  Also add a note about the other requirements for MPLS GSO.
  MPLS GSO support will be posted as a patch net-next (Linux mainline)
  "MPLS: Add limited GSO support"
* Do not add 

Re: [ovs-dev] skb_gso_segment() bad offloads warnings fixed?

[Rajahalme, Jarno (NSN - FI/Espoo)]

On May 23, 2013, at 20:28 , ext Jesse Gross wrote:
> The upstream commit is not really right - the check should depend on
> the origin of the packet, not the location in the networking stack
> processing it. Most of the time these are somewhat the same but
> obviously when bridging, received packets commonly go through the
> transmit path. In the OVS tunnel code, we know that we are doing
> bridging so I used the looser check.

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

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

Re: [ovs-dev] [PATCH] lib/dpif-netdev.c: Remove redundant call to flow_extract

[Rajahalme, Jarno (NSN - FI/Espoo)]

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 of 
these tasks, so it makes sense to do them at the same pass, when you want both.

One strategy I once tried (but did not pursue further at the time) was to allow 
the struct flow * to be passed in a NULL, in effect to signify that only the 
packet layer pointers should be set. A smart compiler should detect the case of 
constant NULL pointer parameter and actually make two versions of the code. If 
this happens, then all the new "if (flow)" littering the code would not cause 
any overhead in practice.

Regards,

  Jarno

Kind Regards,
Alex Wang


On Thu, May 23, 2013 at 10:08 AM, Ben Pfaff 
mailto:b...@nicira.com>> wrote:
On Tue, May 21, 2013 at 11:33:58AM -0700, Alex Wang wrote:
> This commit removes the redundant call to flow_extract in dpif_netdev_execute,
> since in the next line, the call to dpif_netdev_flow_from_nlattrs will reset
> the flow variable.
>
> Signed-off-by: Alex Wang mailto:al...@nicira.com>>

I think that the flow_extract() call is here because the
dpif_netdev_execute() interface only requires the caller to pass in
metadata fields (e.g. in_port).  The current caller actually passes in
everything, but the interface doesn't require it.

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

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

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

[Simon Horman]

On Thu, May 23, 2013 at 11:10:37AM -0700, Ben Pfaff wrote:
> On Wed, May 22, 2013 at 04:08:10PM +0900, Simon Horman wrote:
> > diff --git a/lib/execute-actions.c b/lib/execute-actions.c
> > index 5e05dc5..1036ede 100644
> > --- a/lib/execute-actions.c
> > +++ b/lib/execute-actions.c
> > @@ -170,7 +170,8 @@ 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)
> > +output(dp, packet, nl_attr_get_u32(a));
> 
> Needs {}.

Thanks, fixed.

> > +ovs_assert(!execute_actions(NULL, packet, &key, ctx->odp_actions->data,
> > +ctx->odp_actions->size, NULL, NULL));
> 
> Please don't put required side effects in the condition for
> ovs_assert().  (Even though there's no way to turn off ovs_assert()
> yet, I don't want to rule that out for the future.)

Thanks, I have removed the assert as I'm not entirely convinced of its value.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Simon Horman]

On Thu, May 23, 2013 at 11:03:50AM -0700, Ben Pfaff wrote:
> On Wed, May 22, 2013 at 04:08:08PM +0900, Simon Horman wrote:
> > +set_tunnel_action(const struct nlattr *a, struct flow_tnl *tun_key)
> > +{
> > +enum odp_key_fitness fitness = tun_key_from_attr(a, tun_key);
> > +
> > +memset(&tun_key, 0, sizeof tun_key);
> > +ovs_assert(fitness != ODP_FIT_ERROR);
> > +}
> 
> I don't understand the above code, at all.  The memset call apparently
> means to zero out '*tun_key' (and throw away the conversion work we
> just did) but the way it's written it only zeros out the pointer to
> '*tun_key'.  Either way, it doesn't make sense.

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.

> > +enum odp_key_fitness
> > +tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun);
> 
> We don't put the return type on a separate line for a prototype, and
> the parameter names shouldn't be used in this case, please see
> CodingStyle.

Thanks, I will fix that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v9 3/6] Add execute_actions

[Simon Horman]

On Thu, May 23, 2013 at 10:58:46AM -0700, Ben Pfaff wrote:
> Let's use a better name for the new files, say odp-execute.[ch], and
> use an appropriately prefixed name for the function,
> e.g. odp_actions_execute().

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

> It would also be acceptable to break
> action-related code out of odp-util.[ch] and put all of it in a new
> odp-actions.[ch].

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

> Some more comments:
> 
> On Wed, May 22, 2013 at 04:08:07PM +0900, Simon Horman wrote:
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index a4ac23f..1717602 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -37,6 +37,7 @@
> >  #include "dummy.h"
> >  #include "dynamic-string.h"
> >  #include "flow.h"
> > +#include "execute-actions.h"
> >  #include "hmap.h"
> 
> Please keep the #includes in alphabetical order.

Thanks, fixed.

> 
> >  const struct dpif_class dpif_netdev_class = {
> > diff --git a/lib/execute-actions.c b/lib/execute-actions.c
> > new file mode 100644
> > index 000..d1bbeee
> > --- /dev/null
> > +++ b/lib/execute-actions.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> > + * Copyright (c) 2013 Simon Horman
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "execute-actions.h"
> 
> #include "execute-actions.h" should go right after #include
> , please see CodingStyle.

Thanks, fixed.

> 
> > +case OVS_KEY_ATTR_ETHERNET:
> > +eth_set_src_and_dst(packet,
> > +   nl_attr_get_unspec(a, sizeof(struct ovs_key_ethernet)));
> 
> Please fix indentation here.

Thanks, fixed.

> 
> > +#ifndef EXECUTE_ACTIONS_H
> > +#define EXECUTE_ACTIONS_H 1
> > +
> > +#include "flow.h"
> > +#include "netlink-protocol.h"
> > +#include "ofpbuf.h"
> 
> It would be better to minimize the header dependencies, more like
> this:
> 
> #include 
> #include 
> 
> struct flow;
> struct nlattr;
> struct ofpbuf;

Sure, I have updated the patch accordingly.

> 
> > +void
> > +execute_actions(void *dp, struct ofpbuf *packet, struct flow *key,
> > +const struct nlattr *actions, size_t actions_len,
> > +void (*output)(void *dp, struct ofpbuf *packet,
> > +   uint32_t out_port),
> > +void (*userspace)(void *dp, struct ofpbuf *packet,
> > +  const struct flow *key,
> > +  const struct nlattr *a));
> > +#endif
> 
> Thanks,
> 
> Ben.
> 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3] gre: Restructure tunneling.

[Rajahalme, Jarno (NSN - FI/Espoo)]

Pravin,

Please find some review comments below,

  Jarno

On May 23, 2013, at 23:01 , ext Pravin B Shelar wrote:
...
> diff --git a/datapath/linux/compat/gre.c b/datapath/linux/compat/gre.c
> new file mode 100644
> index 000..c352ff8
> --- /dev/null
> +++ b/datapath/linux/compat/gre.c
> @@ -0,0 +1,352 @@
> +/*
> + * Copyright (c) 2007-2013 Nicira, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> + * 02110-1301, USA
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "gso.h"
> +
> +static struct gre_cisco_protocol __rcu *gre_cisco_proto;
> +
> +static void gre_csum_fix(struct sk_buff *skb)
> +{
> + struct gre_base_hdr *greh;
> + __be32 *options;
> + int gre_offset = skb_transport_offset(skb);
> +
> + greh = (struct gre_base_hdr *)skb_transport_header(skb);
> + options = ((__be32 *)greh + 1);
> +
> + *options = 0;
> + *(__sum16 *)options = csum_fold(skb_checksum(skb, gre_offset,
> +  skb->len - gre_offset, 0));
> +}
> +
> +struct sk_buff *gre_handle_offloads(struct sk_buff *skb, bool gre_csum)
> +{
> + int err;
> +
> + skb_reset_inner_headers(skb);
> +
> + if (skb_is_gso(skb)) {
> + if (gre_csum)
> + OVS_GSO_CB(skb)->fix_segment = gre_csum_fix;
> + } else {
> + if (skb->ip_summed == CHECKSUM_PARTIAL && gre_csum) {
> + err = skb_checksum_help(skb);
> + if (err)
> + goto error;
> +
> + } else if (skb->ip_summed != CHECKSUM_PARTIAL)
> + skb->ip_summed = CHECKSUM_NONE;
> + }
> + return skb;
> +error:
> + kfree_skb(skb);
> + return ERR_PTR(err);
> +}
> +
> +static bool is_gre_gso(struct sk_buff *skb)
> +{
> + return skb_is_gso(skb);
> +}

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

> +
> +void gre_build_header(struct sk_buff *skb, const struct tnl_ptk_info *tpi,
> +   int hdr_len)
> +{
> + struct gre_base_hdr *greh;
> +
> + __skb_push(skb, hdr_len);
> +
> + greh = (struct gre_base_hdr *)skb->data;
> + greh->flags = tnl_flags_to_gre_flags(tpi->flags);
> + greh->protocol = tpi->proto;
> +
> + if (tpi->flags&(TUNNEL_KEY|TUNNEL_CSUM|TUNNEL_SEQ)) {

Add some spaces? Also below.

> + __be32 *ptr = (__be32 *)(((u8 *)greh) + hdr_len - 4);
> +
> + if (tpi->flags&TUNNEL_SEQ) {
> + *ptr = tpi->seq;
> + ptr--;
> + }
> + if (tpi->flags&TUNNEL_KEY) {
> + *ptr = tpi->key;
> + ptr--;
> + }
> + if (tpi->flags&TUNNEL_CSUM && !is_gre_gso(skb)) {
> + *ptr = 0;
> + *(__sum16 *)ptr = csum_fold(skb_checksum(skb, 0,
> + skb->len, 0));
> + }
> + }
> +}
> +
> +static __sum16 check_checksum(struct sk_buff *skb)
> +{
> + __sum16 csum = 0;
> +
> + switch (skb->ip_summed) {
> + case CHECKSUM_COMPLETE:
> + csum = csum_fold(skb->csum);
> +
> + if (!csum)
> + break;
> + /* Fall through. */
> +
> + case CHECKSUM_NONE:
> + skb->csum = 0;
> + csum = __skb_checksum_complete(skb);
> + skb->ip_summed = CHECKSUM_COMPLETE;
> + break;
> + }
> +
> + return csum;
> +}
> +
> +static int parse_gre_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
> + bool *csum_err)

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

> +{
> + unsigned int ip_hlen = ip_hdrlen(skb);
> + struct gre_base_hdr *greh;
> + __be32 *options;
> + int hdr_len;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct gre_base_hdr
> + return -EINVAL;
> +
> + greh = (struct gre_base_hdr *)(skb_network_header(skb) + ip_hlen);
> + if (unlikely(greh->flags & (GRE_VERSION | GRE_ROUTING)))
> + return -EINVAL;
> +
> + tpi->flags = gre_flags_to_tnl_flags(

Re: [ovs-dev] [PATCH] Implement duration fields in OpenFlow 1.3 port stats.

[Rajahalme, Jarno (NSN - FI/Espoo)]

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:

> Signed-off-by: Ben Pfaff 
> ---
> OPENFLOW-1.1+  |3 ++-
> lib/ofp-print.c|6 ++
> lib/ofp-util.c |   17 +++--
> lib/ofp-util.h |2 ++
> ofproto/ofproto-provider.h |1 +
> ofproto/ofproto.c  |   17 +++--
> tests/ofp-print.at |   39 +++
> 7 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
> index b6a4222..d312dab 100644
> --- a/OPENFLOW-1.1+
> +++ b/OPENFLOW-1.1+
> @@ -162,7 +162,8 @@ didn't compare the specs carefully yet.)
> * Rework tag order.  I'm not sure whether we need to do anything
>   for this.
> 
> -* Duration for stats.
> +* Duration for queue stats.  (Duration for port stats is already
> +  implemented.)
> 
> * On-demand flow counters.  I think this might be a real
>   optimization in some cases for the software switch.
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index e899df3..0a9917a 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1218,6 +1218,12 @@ ofp_print_ofpst_port_reply(struct ds *string, const 
> struct ofp_header *oh,
> print_port_stat(string, "drop=", ps.stats.tx_dropped, 1);
> print_port_stat(string, "errs=", ps.stats.tx_errors, 1);
> print_port_stat(string, "coll=", ps.stats.collisions, 0);
> +
> +if (ps.duration_sec != UINT32_MAX) {
> +ds_put_cstr(string, "   duration=");
> +ofp_print_duration(string, ps.duration_sec, ps.duration_nsec);
> +ds_put_char(string, '\n');
> +}
> }
> }
> 
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index b4ff09b..3c8d5a2 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -4595,11 +4595,8 @@ ofputil_port_stats_to_ofp13(const struct 
> ofputil_port_stats *ops,
> struct ofp13_port_stats *ps13)
> {
> ofputil_port_stats_to_ofp11(ops, &ps13->ps);
> -
> -/* OF 1.3 adds duration fields */
> -/* FIXME: Need to implement port alive duration (sec + nsec) */
> -ps13->duration_sec = htonl(~0);
> -ps13->duration_nsec = htonl(~0);
> +ps13->duration_sec = htonl(ops->duration_sec);
> +ps13->duration_nsec = htonl(ops->duration_nsec);
> }
> 
> 
> @@ -4655,6 +4652,7 @@ ofputil_port_stats_from_ofp10(struct ofputil_port_stats 
> *ops,
> ops->stats.rx_over_errors = 
> ntohll(get_32aligned_be64(&ps10->rx_over_err));
> ops->stats.rx_crc_errors = ntohll(get_32aligned_be64(&ps10->rx_crc_err));
> ops->stats.collisions = ntohll(get_32aligned_be64(&ps10->collisions));
> +ops->duration_sec = ops->duration_nsec = UINT32_MAX;
> 
> return 0;
> }
> @@ -4683,6 +4681,7 @@ ofputil_port_stats_from_ofp11(struct ofputil_port_stats 
> *ops,
> ops->stats.rx_over_errors = ntohll(ps11->rx_over_err);
> ops->stats.rx_crc_errors = ntohll(ps11->rx_crc_err);
> ops->stats.collisions = ntohll(ps11->collisions);
> +ops->duration_sec = ops->duration_nsec = UINT32_MAX;
> 
> return 0;
> }
> @@ -4691,13 +4690,11 @@ static enum ofperr
> ofputil_port_stats_from_ofp13(struct ofputil_port_stats *ops,
>   const struct ofp13_port_stats *ps13)
> {
> -enum ofperr error =
> -ofputil_port_stats_from_ofp11(ops, &ps13->ps);
> +enum ofperr error = ofputil_port_stats_from_ofp11(ops, &ps13->ps);
> if (!error) {
> -/* FIXME: Get ps13->duration_sec and ps13->duration_nsec,
> - * Add to netdev_stats? */
> +ops->duration_sec = ntohl(ps13->duration_sec);
> +ops->duration_nsec = ntohl(ps13->duration_nsec);
> }
> -
> return error;
> }
> 
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 4d0d8ad..010c34d 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -689,6 +689,8 @@ bool ofputil_parse_key_value(char **stringp, char **keyp, 
> char **valuep);
> struct ofputil_port_stats {
> uint16_t port_no;
> struct netdev_stats stats;
> +uint32_t duration_sec;  /* UINT32_MAX if unknown. */
> +uint32_t duration_nsec;
> };
> 
> struct ofpbuf *ofputil_encode_dump_ports_request(enum ofp_version ofp_version,
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index b9d6f0d..db0d589 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -119,6 +119,7 @@ struct ofport {
> struct ofputil_phy_port pp;
> uint16_t ofp_port;  /* OpenFlow port number. */
> unsigned int change_seq;
> +long long int created;  /* Time created, in msec. */
> int mtu;
> };
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index ca1dc89..2f91d65 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -203,6 +203,8 @@ static bool handle_openflow(struct ofconn *, struct 
> 

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

[Simon Horman]

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/udp_offload.c  |   3 +-
 net/mpls/Kconfig|   9 +++
 net/mpls/Makefile   |   4 ++
 net/mpls/mpls_gso.c | 108 +++
 15 files changed, 149 insertions(+), 116 deletions(-)
 create mode 100644 net/mpls/Kconfig
 create mode 100644 net/mpls/Makefile
 create mode 100644 net/mpls/mpls_gso.c

-- 
1.8.2.1

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

[ovs-dev] [PATCH net-next v5 1/2] net: Use 16bits for *_headers fields of struct skbuff

[Simon Horman]

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 
---
 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
@@ -509,12 +509,12 @@ struct sk_buff {
__u32   reserved_tailroom;
};
 
-   sk_buff_data_t  inner_transport_header;
-   sk_buff_data_t  inner_network_header;
-   sk_buff_data_t  inner_mac_header;
-   sk_buff_data_t  transport_header;
-   sk_buff_data_t  network_header;
-   sk_buff_data_t  mac_header;
+   __u16   inner_transport_header;
+   __u16   inner_network_header;
+   __u16   inner_mac_header;
+   __u16   transport_header;
+   __u16   network_header;
+   __u16   mac_header;
/* These elements must be at the end, see alloc_skb() for details.  */
sk_buff_data_t  tail;
sk_buff_data_t  end;
@@ -1527,7 +1527,6 @@ static inline void skb_reset_mac_len(struct sk_buff *skb)
skb->mac_len = skb->network_header - skb->mac_header;
 }
 
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
 static inline unsigned char *skb_inner_transport_header(const struct sk_buff
*skb)
 {
@@ -1638,112 +1637,6 @@ static inline void skb_set_mac_header(struct sk_buff 
*skb, const int offset)
skb->mac_header += offset;
 }
 
-#else /* NET_SKBUFF_DATA_USES_OFFSET */
-static inline unsigned char *skb_inner_transport_header(const struct sk_buff
-   *skb)
-{
-   return skb->inner_transport_header;
-}
-
-static inline void skb_reset_inner_transport_header(struct sk_buff *skb)
-{
-   skb->inner_transport_header = skb->data;
-}
-
-static inline void skb_set_inner_transport_header(struct sk_buff *skb,
-  const int offset)
-{
-   skb->inner_transport_header = skb->data + offset;
-}
-
-static inline unsigned char *skb_inner_network_header(const struct sk_buff 
*skb)
-{
-   return skb->inner_network_header;
-}
-
-static inline void skb_reset_inner_network_header(struct sk_buff *skb)
-{
-   skb->inner_network_header = skb->data;
-}
-
-static inline void skb_set_inner_network_header(struct sk_buff *skb,
-   const int offset)
-{
-   skb->inner_network_header = skb->data + offset;
-}
-
-static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
-{
-   return skb->inner_mac_header;
-}
-
-static inline void skb_reset_inner_mac_header(struct sk_buff *skb)
-{
-   skb->inner_mac_header = skb->data;
-}
-
-static inline void skb_set_inner_mac_header(struct sk_buff *skb,
-   const int offset)
-{
-   skb->inner_mac_header = skb->data + offset;
-}
-static inline bool skb_transport_header_was_set(const struct sk_buff *skb)
-{
-   return skb->transport_header != NULL;
-}
-
-static inline unsigned char *skb_transport_header(const struct sk_buff *skb)
-{
-   return skb->transport_header;
-}
-
-static inline void skb_reset_transport_header(struct sk_buff *skb)
-{
-   skb->transport_header = skb->data;
-}
-
-static inline void skb_set_transport_header(struct sk_buff *skb,
-   const int offset)
-{
-   skb->transport_header = skb->data + offset;
-}
-
-static inline unsigned char *skb_network_header(const struct sk_buff *skb)
-{
-   return skb->network_header;
-}
-
-static inline void skb_reset_network_header(struct sk_buff *skb)
-{
-   skb->network_header = skb->data;
-}
-
-static inline void skb_set_network_header(struct sk_buff *skb, const int 
offset)
-{
-   skb->network_header = skb->data + offset;
-}
-
-static inline unsigned char *skb_mac_header(const struct sk_buff *skb)
-{
-   return skb->mac_header;
-}
-
-static inline int skb_mac_header_was_set(const struct sk_buff *skb)
-{
-   return skb->mac_header != NULL;
-}
-
-static inline void skb_reset_mac_header(struct sk_buff *skb)
-{
-   skb->mac_header = skb->data;
-}
-
-static inline void skb_set_mac_header(struct sk_buff *skb, const int offset)
-{
-   skb->mac_hea

[ovs-dev] [PATCH net-next v5 2/2] MPLS: Add limited GSO support

[Simon Horman]

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 code is to provide GSO in software for MPLS packets
whose skbs are GSO.

SKB Usage:

When an implementation adds an MPLS stack to a non-MPLS packet it should do
the following to skb metadata:

* Set skb->inner_protocol to the old non-MPLS ethertype of the packet.
  skb->inner_protocol is added by this patch.

* Set skb->protocol to the new MPLS ethertype of the packet.

* Set skb->network_header to correspond to the
  end of the L3 header, including the MPLS label stack.

I have posted a patch, "[PATCH v3.29] datapath: Add basic MPLS support to
kernel" which adds MPLS support to the kernel datapath of Open vSwtich.
That patch sets the above requirements in datapath/actions.c:push_mpls()
and was used to exercise this code.  The datapath patch is against the Open
vSwtich tree but it is intended that it be added to the Open vSwtich code
present in the mainline Linux kernel at some point.

Features:

I believe that the approach that I have taken is at least partially
consistent with the handling of other protocols.  Jesse, I understand that
you have some ideas here.  I am more than happy to change my implementation.

This patch adds dev->mpls_features which may be used by devices
to advertise features supported for MPLS packets.

A new NETIF_F_MPLS_GSO feature is added for devices which support
hardware MPLS GSO offload.  Currently no devices support this
and MPLS GSO always falls back to software.

Alternate Implementation:

One possible alternate implementation is to teach netif_skb_features()
and skb_network_protocol() about MPLS, in a similar way to their
understanding of VLANs. I believe this would avoid the need
for net/mpls/mpls_gso.c and in particular the calls to
__skb_push() and __skb_push() in mpls_gso_segment().

I have decided on the implementation in this patch as it should
not introduce any overhead in the case where mpls_gso is not compiled
into the kernel or inserted as a module.

MPLS GSO suggested by Jesse Gross.
Based in part on "v4 GRE: Add TCP segmentation offload for GRE"
by Pravin B Shelar.

Cc: Jesse Gross 
Cc: Pravin B Shelar 
Signed-off-by: Simon Horman 

---

MPLS GSO also requires "net: Loosen constraints for recalculating checksum
in skb_segment()" in order for segmentation of TCP/IP inner packets to
correctly recalculate checksums.

Tested using the bnx2 driver.

v5
* Move inner_protocol from below inner_mac_header to
  above inner_transport header to allow all *_header fields
  to be grouped together.
* Remove trailing blank line from mpls_gso.c

v4
* Add inner_protocol to kernel doc for struct skbuff
* As suggested by Jesse Gross
  - Refer to CONFIG_NET_MPLS_GSO rather than CONFIG_NET_GRE_GSO in
change log
  - Reword the changelog to reflect that mac_len is not set to correspond
to the end of l3 data. Its value is not important to the function of
GSO as a call to skb_reset_mac_len() is made in __skb_gso_segment().

* I considered guarding the presence of inner_protocol in struct skbuff with
  #ifdef CONFIG_NET_MPLS_GSO as it is otherwise unused. However, this
  makes accessing inner_protocol from out-of tree code somewhat tedious.
  And the reference usage of this code is the out-of tree version
  of the Open vSwitch datapath.

v3
* As suggested by Ben Hutchings
  - Update NETIF_F_GSO_LAST
* Update SKB usage
  - Add and use skb->inner_protocol
  - Set skb->protocol to MPLS ethertype of packet
  - Set skb->mac_len and skb->network_header to correspond to the
end of the L3 header.

v2
* As suggested by Jarno Rajahalme
  - Update NETIF_F_GSO_LAST
  - mpls_mc_offload to use ETH_P_MPLS_MC
* Remove NETIF_F_iP_CSUM|NETIF_F_IPV6_CSUM features hack in
  mpls_gso_segment()
---
 include/linux/netdev_features.h |   4 +-
 include/linux/netdevice.h   |   2 +
 include/linux/skbuff.h  |   4 ++
 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/udp_offload.c  |   3 +-
 net/mpls/Kconfig|   9 
 net/mpls/Makefile   |   4 ++
 net/mpls/mpls_gso.c | 108 
 15 files changed, 143 insertions(+), 3 deletions(-)
 create mode 100644 net/mpls/Kconfig
 create mode 100644 net/mpls/Makefile
 create mode 100644 net/mpls/mpls_gso.c

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 09906b7..a2a89a5 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -43,8 +43,9 @@ enum {
NETIF_F_FSO_BIT,/* ... FCoE segmentation