[ovs-dev] [export netlink fix v2] datapath: fix bugs in exporting netlink attributes

2013-07-09 Thread Andy Zhou 
Reported-by: Justin Pettit 
Signed-off-by: Andy Zhou 


v1 -> v2
Address Jesse's review feedback:
o Fix the logic of exporting tun_id mask.
o Remove unnecessary check for swkey values.
o Kept a few optimizations of omitting export zero values
  whenever it is safe and un-complicated to do so.
---
 datapath/flow.c |   47 ---
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index adb918f..a756a15 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1273,23 +1273,25 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
if (!nla)
return -EMSGSIZE;
 
-   if (tun_key->tun_flags & TUNNEL_KEY &&
+   if (output->tun_flags & TUNNEL_KEY &&
nla_put_be64(skb, OVS_TUNNEL_KEY_ATTR_ID, output->tun_id))
return -EMSGSIZE;
-   if (tun_key->ipv4_src &&
-   nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, output->ipv4_src))
+   if (output->ipv4_src &&
+   nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, 
output->ipv4_src))
return -EMSGSIZE;
-   if (nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_IPV4_DST, output->ipv4_dst))
+   if (output->ipv4_dst &&
+   nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_IPV4_DST, 
output->ipv4_dst))
return -EMSGSIZE;
-   if (tun_key->ipv4_tos &&
-   nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos))
+   if (output->ipv4_tos &&
+   nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos))
return -EMSGSIZE;
-   if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
+   if (output->ipv4_ttl &&
+   nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
return -EMSGSIZE;
-   if ((tun_key->tun_flags & TUNNEL_DONT_FRAGMENT) &&
+   if ((output->tun_flags & TUNNEL_DONT_FRAGMENT) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
return -EMSGSIZE;
-   if ((tun_key->tun_flags & TUNNEL_CSUM) &&
+   if ((output->tun_flags & TUNNEL_CSUM) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_CSUM))
return -EMSGSIZE;
 
@@ -1686,16 +1688,15 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key *swkey,
struct ovs_key_ethernet *eth_key;
struct nlattr *nla, *encap;
 
-   if (swkey->phy.priority &&
-   nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, output->phy.priority))
+   if (output->phy.priority &&
+   nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, output->phy.priority))
goto nla_put_failure;
 
if (swkey->tun_key.ipv4_dst &&
ipv4_tun_to_nlattr(skb, &swkey->tun_key, &output->tun_key))
goto nla_put_failure;
 
-   if (swkey->phy.in_port != DP_MAX_PORTS) {
-   /* Exact match upper 16 bits. */
+   {
u16 upper_u16;
upper_u16 = (swkey == output) ? 0 : 0x;
 
@@ -1704,8 +1705,8 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key *swkey,
goto nla_put_failure;
}
 
-   if (swkey->phy.skb_mark &&
-   nla_put_u32(skb, OVS_KEY_ATTR_SKB_MARK, output->phy.skb_mark))
+   if (output->phy.skb_mark &&
+   nla_put_u32(skb, OVS_KEY_ATTR_SKB_MARK, output->phy.skb_mark))
goto nla_put_failure;
 
nla = nla_reserve(skb, OVS_KEY_ATTR_ETHERNET, sizeof(*eth_key));
@@ -1728,12 +1729,20 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key *swkey,
} else
encap = NULL;
 
-   if ((swkey == output) && (swkey->eth.type == htons(ETH_P_802_2)))
+   if (swkey->eth.type == htons(ETH_P_802_2)) {
+   /*
+* Ethertype 802.2 is represented in the netlink with omitted
+* OVS_KEY_ATTR_ETHERTYPE in the flow key attribute, and
+* 0x in the mask attribute.
+*/
+   if (swkey != output)
+   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, 
htons(0x)))
+   goto nla_put_failure;
goto unencap;
+   }
 
-   if (output->eth.type != 0)
-   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, output->eth.type))
-   goto nla_put_failure;
+   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, output->eth.type))
+   goto nla_put_failure;
 
if (swkey->eth.type == htons(ETH_P_IP)) {
struct ovs_key_ipv4 *ipv4_key;
-- 
1.7.9.5

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

Re: [ovs-dev] [PATCH V3] feature: Create specific types for ofp and odp port

2013-07-09 Thread YAMAMOTO Takashi 
hi,

> @@ -708,9 +711,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, 
> uint32_t key_len,
>  return EINVAL;
>  }
>  
> -if (flow->in_port < OFPP_MAX
> -? flow->in_port >= MAX_PORTS
> -: flow->in_port != OFPP_LOCAL && flow->in_port != OFPP_NONE) {
> +if (odp_to_u32(flow->in_port.odp_port) >= MAX_PORTS) {
>  return EINVAL;
>  }

it seems that this change broke packet_out with in_port=controller.

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

[ovs-dev] [PATCH] datapath: reset skb->pkt_type when receiving pkt_type of PACKET_OTHERHOST

2013-07-09 Thread Isaku Yamahata 
The upstream has the fix for dev_forward_skb() to reset pkt_type to
PACKET_HOST. the change set of 06a23fe31ca3992863721f21bdb0307af93da807
So the pkt_type of PACKET_OTHERHOST doesn't come in.
This patch is a workaround for older kernel, reset skb->pkt_type when
receiving pkt_type of PACKET_OTHERHOST.

When two tunnel ports created on two OVS bridges on same host and
two ports are the end point of the tunnel, packets are dropped as below.

If a packet has pkt_type of PACKET_OTHERHOST when coming to ovs bridge,
the packet is dropped by ip_rcv() if rules are installed and skb are
forwarded by loopback device.
Packet isn't dropped if flow rule isn't installed in kernel datapath,
OVS_ACTION_ATTR_OUTPUT is used to send the packet.

netns A |  root netns | netns B
   veth<->veth=ovs bridge=gre<-loop back->gre=ovs bridge=veth<->veth

arp packet ->
pkt_type
 BROADCAST>ip_rcv()>

 <- arp reply
pkt_type

 rule exists
   ip_rcv()<-OTHERHOST
   drop

  rx_bytes += skb->len;
u64_stats_update_end(&stats->syncp);
 
+   if (skb->pkt_type == PACKET_OTHERHOST) {
+   /*
+* Workaround:
+* Now dev_forward_skb() resets skb->packet_type to
+* PACKET_HOST by the change set of
+* 06a23fe31ca3992863721f21bdb0307af93da807
+* For older kernel, skb can have packet_type of
+* PACKET_OTHERHOST. In that case packet can be dropped
+* unexpectedly as follows.
+*
+* root netns | netns X
+* loop back<-gre=ovs bridge=veth<->veth  <--packet
+*
+*   pkt_type
+*   dev_forward_skb():veth
+*
+*   rule exists
+* ip_rcv()<-PACKET_OTHERHOST
+* drop
+*
+* ip_rcv()<--- PACKET_HOST  rule doesn't exist
+* pass ^  |
+*  |  |upcall
+*  |  V
+*  OVS_ACTION_ATTR_OUTPUT
+*  ovs-switchd
+*/
+   skb->pkt_type = PACKET_HOST;
+   }
OVS_CB(skb)->tun_key = tun_key;
ovs_dp_process_received_packet(vport, skb);
 }
-- 
1.7.10.4

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

[ovs-dev] [PATCH 0/3] RHEL Updates

2013-07-09 Thread Thomas Graf 
Thomas Graf (3):
  datapath: Rename skb_network_protocol() to __skb_network_protocol()
  datapath: rhel: Account for RHEL specific backports
  datapath: rhel: Move RHEL OVS hook registration to
netdev_rx_handler_register() backport

 acinclude.m4|  3 +++
 datapath/linux/compat/gso.c |  4 ++--
 datapath/linux/compat/include/linux/netdevice.h | 27 +++--
 datapath/vport-netdev.c | 17 +---
 4 files changed, 31 insertions(+), 20 deletions(-)

-- 
1.8.3.1

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

[ovs-dev] [PATCH] dpif-netdev: Make "packet-out" with in_port=OFPP_CONTROLLER work again.

2013-07-09 Thread Ben Pfaff 
Commit 4e022ec09e14 (Create specific types for ofp and odp port) broke
OpenFlow OFPP_PACKET_OUT requests that use in_port=OFPP_CONTROLLER.  This
commit fixes the problem and adds a regression test.

CC: Alex Wang 
Reported-by: YAMAMOTO Takashi 
Signed-off-by: Ben Pfaff 
---
 lib/dpif-netdev.c |5 -
 tests/ofproto.at  |7 +--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f8f1f88..f7e3b1f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -694,6 +694,8 @@ static int
 dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
   struct flow *flow)
 {
+odp_port_t in_port;
+
 if (odp_flow_key_to_flow(key, key_len, flow) != ODP_FIT_PERFECT) {
 /* This should not happen: it indicates that odp_flow_key_from_flow()
  * and odp_flow_key_to_flow() disagree on the acceptable form of a
@@ -713,7 +715,8 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, 
uint32_t key_len,
 return EINVAL;
 }
 
-if (!is_valid_port_number(flow->in_port.odp_port)) {
+in_port = flow->in_port.odp_port;
+if (!is_valid_port_number(in_port) && in_port != ODPP_NONE) {
 return EINVAL;
 }
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index df98863..e2e6f1b 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -1487,6 +1487,7 @@ dnl controllers despite the spec) as meaning a packet 
that was generated
 dnl by the controller.
 AT_SETUP([ofproto - packet-out from controller (OpenFlow 1.0)])
 OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1])
 
 # Start a monitor listening for packet-ins.
 AT_CHECK([ovs-ofctl -P openflow10 monitor br0 --detach --no-chdir --pidfile])
@@ -1496,13 +1497,15 @@ ovs-appctl -t ovs-ofctl ofctl/set-output-file 
monitor.log
 AT_CAPTURE_FILE([monitor.log])
 
 # Send some packet-outs with OFPP_NONE and OFPP_CONTROLLER (65533) as in_port.
-AT_CHECK([ovs-ofctl packet-out br0 none controller 
'0001020304050010203040501234'])
-AT_CHECK([ovs-ofctl packet-out br0 controller controller 
'0001020304050010203040505678'])
+AT_CHECK([ovs-ofctl packet-out br0 none controller,1 
'0001020304050010203040501234'])
+AT_CHECK([ovs-ofctl packet-out br0 controller controller,1 
'0001020304050010203040505678'])
 
 # Stop the monitor and check its output.
 ovs-appctl -t ovs-ofctl ofctl/barrier
 ovs-appctl -t ovs-ofctl exit
 
+ovs-ofctl dump-ports br0
+
 AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
 OFPT_PACKET_IN: total_len=14 in_port=ANY (via action) data_len=14 (unbuffered)
 
metadata=0,in_port=0,vlan_tci=0x,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
-- 
1.7.2.5

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

[ovs-dev] [PATCH 3/3] datapath: rhel: Move RHEL OVS hook registration to netdev_rx_handler_register() backport

2013-07-09 Thread Thomas Graf 
Moves the registration of the RHEL specific OVS hook to the compat
backport of netdev_rx_handler_register(). This moves the hook
unregistration from the RCU callback to the netdev_destroy()
callback directly.

This is purely cosmetic though, the RHEL hook is only used if the
IFF_OVS_DATAPATH flag is present which was removed under RTNL
protection before the RCU callback.

Signed-off-by: Thomas Graf 
---
 datapath/linux/compat/include/linux/netdevice.h | 21 -
 datapath/vport-netdev.c | 17 +
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/datapath/linux/compat/include/linux/netdevice.h 
b/datapath/linux/compat/include/linux/netdevice.h
index b051baf..90edb7d 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -20,6 +20,11 @@ struct net;
 #define to_net_dev(class) container_of(class, struct net_device, 
NETDEV_DEV_MEMBER)
 #endif
 
+#ifdef HAVE_RHEL_OVS_HOOK
+extern struct sk_buff *(*openvswitch_handle_frame_hook)(struct sk_buff *skb);
+extern atomic_t nr_bridges;
+#endif
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,26)
 static inline
 struct net *dev_net(const struct net_device *dev)
@@ -84,19 +89,33 @@ extern int skb_checksum_help(struct sk_buff *skb, int);
 extern int skb_checksum_help(struct sk_buff *skb);
 #endif
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,36)
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,36) || \
+defined HAVE_RHEL_OVS_HOOK
 static inline int netdev_rx_handler_register(struct net_device *dev,
 void *rx_handler,
 void *rx_handler_data)
 {
+#ifdef HAVE_RHEL_OVS_HOOK
+   rcu_assign_pointer(dev->ax25_ptr, rx_handler_data);
+   atomic_inc(&nr_bridges);
+   rcu_assign_pointer(openvswitch_handle_frame_hook, rx_handler_data);
+#else
if (dev->br_port)
return -EBUSY;
rcu_assign_pointer(dev->br_port, rx_handler_data);
+#endif
return 0;
 }
 static inline void netdev_rx_handler_unregister(struct net_device *dev)
 {
+#ifdef HAVE_RHEL_OVS_HOOK
+   rcu_assign_pointer(dev->ax25_ptr, NULL);
+
+   if (atomic_dec_and_test(&nr_bridges))
+   rcu_assign_pointer(openvswitch_handle_frame_hook, NULL);
+#else
rcu_assign_pointer(dev->br_port, NULL);
+#endif
 }
 #endif
 
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index fe7e359..d043eae 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -46,9 +46,8 @@ MODULE_PARM_DESC(vlan_tso, "Enable TSO for VLAN packets");
 #endif
 
 #ifdef HAVE_RHEL_OVS_HOOK
-static atomic_t nr_bridges = ATOMIC_INIT(0);
+atomic_t nr_bridges = ATOMIC_INIT(0);
 
-extern struct sk_buff *(*openvswitch_handle_frame_hook)(struct sk_buff *skb);
 #endif
 
 static void netdev_port_receive(struct vport *vport, struct sk_buff *skb);
@@ -171,16 +170,10 @@ static struct vport *netdev_create(const struct 
vport_parms *parms)
}
 
rtnl_lock();
-#ifdef HAVE_RHEL_OVS_HOOK
-   rcu_assign_pointer(netdev_vport->dev->ax25_ptr, vport);
-   atomic_inc(&nr_bridges);
-   rcu_assign_pointer(openvswitch_handle_frame_hook, netdev_frame_hook);
-#else
err = netdev_rx_handler_register(netdev_vport->dev, netdev_frame_hook,
 vport);
if (err)
goto error_unlock;
-#endif
 
dev_set_promiscuity(netdev_vport->dev, 1);
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,24)
@@ -192,9 +185,7 @@ static struct vport *netdev_create(const struct vport_parms 
*parms)
netdev_init();
return vport;
 
-#ifndef HAVE_RHEL_OVS_HOOK
 error_unlock:
-#endif
rtnl_unlock();
 error_put:
dev_put(netdev_vport->dev);
@@ -209,12 +200,6 @@ static void free_port_rcu(struct rcu_head *rcu)
struct netdev_vport *netdev_vport = container_of(rcu,
struct netdev_vport, rcu);
 
-#ifdef HAVE_RHEL_OVS_HOOK
-   rcu_assign_pointer(netdev_vport->dev->ax25_ptr, NULL);
-
-   if (atomic_dec_and_test(&nr_bridges))
-   rcu_assign_pointer(openvswitch_handle_frame_hook, NULL);
-#endif
dev_put(netdev_vport->dev);
ovs_vport_free(vport_from_priv(netdev_vport));
 }
-- 
1.8.3.1

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

[ovs-dev] [PATCH 1/3] datapath: Rename skb_network_protocol() to __skb_network_protocol()

2013-07-09 Thread Thomas Graf 
The function skb_network_protocol() is already defined in
 upstream but not an exported symbol.

Rename the OVS internal implementation to work around this.

Signed-off-by: Thomas Graf 
---
 datapath/linux/compat/gso.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
index 3cadde9..43418d3 100644
--- a/datapath/linux/compat/gso.c
+++ b/datapath/linux/compat/gso.c
@@ -36,7 +36,7 @@
 
 #include "gso.h"
 
-static __be16 skb_network_protocol(struct sk_buff *skb)
+static __be16 __skb_network_protocol(struct sk_buff *skb)
 {
__be16 type = skb->protocol;
int vlan_depth = ETH_HLEN;
@@ -68,7 +68,7 @@ static struct sk_buff *tnl_skb_gso_segment(struct sk_buff 
*skb,
 
/* setup whole inner packet to get protocol. */
__skb_pull(skb, mac_offset);
-   skb->protocol = skb_network_protocol(skb);
+   skb->protocol = __skb_network_protocol(skb);
 
/* setup l3 packet to gso, to get around segmentation bug on older 
kernel.*/
__skb_pull(skb, (pkt_hlen - mac_offset));
-- 
1.8.3.1

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

[ovs-dev] [PATCH 2/3] datapath: rhel: Account for RHEL specific backports

2013-07-09 Thread Thomas Graf 
The following symbols have been backported to RHEL and the kernel
version is no longer an accurate indicator for their presence:

 - skb_gso_segment
 - netif_skb_features
 - netif_needs_gso

Signed-off-by: Thomas Graf 
---
 acinclude.m4| 3 +++
 datapath/linux/compat/include/linux/netdevice.h | 6 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 717c681..d9fddf8 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -225,7 +225,10 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [dev_get_stats])
   OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [dev_get_by_index_rcu])
   OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [__skb_gso_segment])
+  OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [skb_gso_segment])
   OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [can_checksum_protocol])
+  OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [netif_skb_features])
+  OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [netif_needs_gso])
 
   OVS_GREP_IFELSE([$KSRC/include/linux/rcupdate.h], [rcu_read_lock_held], [],
   [OVS_GREP_IFELSE([$KSRC/include/linux/rtnetlink.h],
diff --git a/datapath/linux/compat/include/linux/netdevice.h 
b/datapath/linux/compat/include/linux/netdevice.h
index f2ced69..b051baf 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -142,13 +142,17 @@ static inline struct net_device 
*dev_get_by_index_rcu(struct net *net, int ifind
 #define NETIF_F_V6_CSUM(NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
 #endif
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,38)
+#ifndef HAVE_SKB_GSO_SEGMENT
 #define skb_gso_segment rpl_skb_gso_segment
 struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb, u32 features);
+#endif
 
+#ifndef HAVE_NETIF_SKB_FEATURES
 #define netif_skb_features rpl_netif_skb_features
 u32 rpl_netif_skb_features(struct sk_buff *skb);
+#endif
 
+#ifndef HAVE_NETIF_NEEDS_GSO
 #define netif_needs_gso rpl_netif_needs_gso
 static inline int rpl_netif_needs_gso(struct sk_buff *skb, int features)
 {
-- 
1.8.3.1

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

Re: [ovs-dev] [PATCH V3] feature: Create specific types for ofp and odp port

2013-07-09 Thread Ben Pfaff 
On Tue, Jul 09, 2013 at 05:18:34PM +0900, YAMAMOTO Takashi wrote:
> hi,
> 
> > @@ -708,9 +711,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, 
> > uint32_t key_len,
> >  return EINVAL;
> >  }
> >  
> > -if (flow->in_port < OFPP_MAX
> > -? flow->in_port >= MAX_PORTS
> > -: flow->in_port != OFPP_LOCAL && flow->in_port != OFPP_NONE) {
> > +if (odp_to_u32(flow->in_port.odp_port) >= MAX_PORTS) {
> >  return EINVAL;
> >  }
> 
> it seems that this change broke packet_out with in_port=controller.

Oops.

I sent out a fix for review:
http://openvswitch.org/pipermail/dev/2013-July/029436.html
Can you confirm that it fixes the problem for you?

Thanks,

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

Re: [ovs-dev] [PATCH 1/3] datapath: Rename skb_network_protocol() to __skb_network_protocol()

2013-07-09 Thread Pravin Shelar 
On Tue, Jul 9, 2013 at 9:00 AM, Thomas Graf  wrote:
> The function skb_network_protocol() is already defined in
>  upstream but not an exported symbol.
>
> Rename the OVS internal implementation to work around this.
>
Are you seeing problem with RHEL kernel backports?
OVS does not support upstream kernel so this is not problem at this time.


> Signed-off-by: Thomas Graf 
> ---
>  datapath/linux/compat/gso.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
> index 3cadde9..43418d3 100644
> --- a/datapath/linux/compat/gso.c
> +++ b/datapath/linux/compat/gso.c
> @@ -36,7 +36,7 @@
>
>  #include "gso.h"
>
> -static __be16 skb_network_protocol(struct sk_buff *skb)
> +static __be16 __skb_network_protocol(struct sk_buff *skb)
>  {
> __be16 type = skb->protocol;
> int vlan_depth = ETH_HLEN;
> @@ -68,7 +68,7 @@ static struct sk_buff *tnl_skb_gso_segment(struct sk_buff 
> *skb,
>
> /* setup whole inner packet to get protocol. */
> __skb_pull(skb, mac_offset);
> -   skb->protocol = skb_network_protocol(skb);
> +   skb->protocol = __skb_network_protocol(skb);
>
> /* setup l3 packet to gso, to get around segmentation bug on older 
> kernel.*/
> __skb_pull(skb, (pkt_hlen - mac_offset));
> --
> 1.8.3.1
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/3] datapath: Rename skb_network_protocol() to __skb_network_protocol()

2013-07-09 Thread Thomas Graf 

On 07/09/2013 06:38 PM, Pravin Shelar wrote:

On Tue, Jul 9, 2013 at 9:00 AM, Thomas Graf  wrote:

The function skb_network_protocol() is already defined in
 upstream but not an exported symbol.

Rename the OVS internal implementation to work around this.


Are you seeing problem with RHEL kernel backports?
OVS does not support upstream kernel so this is not problem at this time.


Yes, RHEL6 is affected as well.

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

Re: [ovs-dev] [PATCH] ovs-vsctl: Fix behavioral regression for "--if-exists del-port ".

2013-07-09 Thread Justin Pettit 
The behavior seems a little surprising, since the "--if-exists" sounds like it 
will be deleted if it exists, but it's actually just avoiding a fatal error and 
the port will still exist.  It would be nicer to give some sort of non-fatal 
warning in that case.  However, that's likely to cause breakage in scripts and 
most people know not to delete the local port, so I'm also fine if you prefer 
this approach.

--Justin


On Jul 8, 2013, at 10:48 AM, Ben Pfaff  wrote:

> Commit 89f3c258fe (ovs-vsctl: Improve error message for "ovs-vsctl del-port
> ".) changed the behavior of
>ovs-vsctl --if-exists del-port 
> from a silent no-op to a hard failure.  This commit fixes this regression.
> 
> This caused problems on XenServer, for which the Open vSwitch integration
> runs commands like:
>/usr/bin/ovs-vsctl --timeout=20 \
>-- --with-iface --if-exists del-port xapi103 \
>-- --if-exists del-br xapi103
> 
> Bug #18276.
> Reported-by: Michael Hu 
> Signed-off-by: Ben Pfaff 
> ---
> tests/ovs-vsctl.at|2 ++
> utilities/ovs-vsctl.c |9 ++---
> 2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index fa2c3ff..4449f7a 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -327,6 +327,8 @@ AT_CHECK([RUN_OVS_VSCTL([del-port a])], [1], [],
>   [ovs-vsctl: cannot delete port a because it is the local port for bridge a 
> (deleting this port requires deleting the entire bridge)
> ],
>   [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-port a])], [0], [], [],
> +  [OVS_VSCTL_CLEANUP])
> AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port a b1])], [1], [],
>   [ovs-vsctl: "--may-exist add-port a b1" but b1 is actually attached to 
> bridge b
> ],
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 2d8c7c7..e679e0d 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -2021,9 +2021,12 @@ cmd_del_port(struct vsctl_context *ctx)
> 
> vsctl_context_populate_cache(ctx);
> if (find_bridge(ctx, target, false)) {
> -vsctl_fatal("cannot delete port %s because it is the local port "
> -"for bridge %s (deleting this port requires deleting "
> -"the entire bridge)", target, target);
> +if (must_exist) {
> +vsctl_fatal("cannot delete port %s because it is the local port "
> +"for bridge %s (deleting this port requires deleting 
> "
> +"the entire bridge)", target, target);
> +}
> +port = NULL;
> } else if (!with_iface) {
> port = find_port(ctx, target, must_exist);
> } else {
> -- 
> 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 1/2] atomic: Suppress sparse warning.

2013-07-09 Thread Ethan Jackson 
With the latest version of sparse, the ATOMIC_VAR_INIT macro
generates the following warning.  This patch suppresses it.

warning: Using plain integer as NULL pointer

Signed-off-by: Ethan Jackson 
---
 lib/ovs-atomic-pthreads.h |4 
 1 file changed, 4 insertions(+)

diff --git a/lib/ovs-atomic-pthreads.h b/lib/ovs-atomic-pthreads.h
index 2f47a9c..e6f3f1a 100644
--- a/lib/ovs-atomic-pthreads.h
+++ b/lib/ovs-atomic-pthreads.h
@@ -80,7 +80,11 @@ typedef enum {
 memory_order_seq_cst
 } memory_order;
 
+#if __CHECKER__
+#define ATOMIC_VAR_INIT(VALUE) { .value = VALUE }
+#else
 #define ATOMIC_VAR_INIT(VALUE) { VALUE, PTHREAD_MUTEX_INITIALIZER }
+#endif
 #define atomic_init(OBJECT, VALUE)  \
 ((OBJECT)->value = (VALUE), \
  pthread_mutex_init(&(OBJECT)->mutex, NULL),\
-- 
1.7.9.5

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

[ovs-dev] [PATCH 2/2] csum: Suppress sparse warning.

2013-07-09 Thread Ethan Jackson 
The latest version of sparse triggers the following warning in the
checksum tests.  This patch suppresses it.

warning: too long initializer-string for array of char

Signed-off-by: Ethan Jackson 
---
 tests/test-csum.c |9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/test-csum.c b/tests/test-csum.c
index 5f736d6..b08f236 100644
--- a/tests/test-csum.c
+++ b/tests/test-csum.c
@@ -94,9 +94,12 @@ static void
 test_rfc1624(void)
 {
 /* "...an IP packet header in which a 16-bit field m = 0x..." */
-uint8_t data[32] =
-"\xfe\x8f\xc1\x14\x4b\x6f\x70\x2a\x80\x29\x78\xc0\x58\x81\x77\xaa"
-"\x66\x64\xfc\x96\x63\x97\x64\xee\x12\x53\x1d\xa9\x2d\xa9\x55\x55";
+uint8_t data[32] = {
+0xfe, 0x8f, 0xc1, 0x14, 0x4b, 0x6f, 0x70, 0x2a,
+0x80, 0x29, 0x78, 0xc0, 0x58, 0x81, 0x77, 0xaa,
+0x66, 0x64, 0xfc, 0x96, 0x63, 0x97, 0x64, 0xee,
+0x12, 0x53, 0x1d, 0xa9, 0x2d, 0xa9, 0x55, 0x55
+};
 
 /* "...the one's complement sum of all other header octets is 0xCD7A." */
 assert(ntohs(csum(data, sizeof data - 2)) == 0x - 0xcd7a);
-- 
1.7.9.5

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

Re: [ovs-dev] [PATCH V3] feature: Create specific types for ofp and odp port

2013-07-09 Thread Alex Wang 
Sorry Yamamoto and thanks Ben,

Please ignore my previous reply.


On Tue, Jul 9, 2013 at 9:24 AM, Ben Pfaff  wrote:

> On Tue, Jul 09, 2013 at 05:18:34PM +0900, YAMAMOTO Takashi wrote:
> > hi,
> >
> > > @@ -708,9 +711,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
> *key, uint32_t key_len,
> > >  return EINVAL;
> > >  }
> > >
> > > -if (flow->in_port < OFPP_MAX
> > > -? flow->in_port >= MAX_PORTS
> > > -: flow->in_port != OFPP_LOCAL && flow->in_port != OFPP_NONE) {
> > > +if (odp_to_u32(flow->in_port.odp_port) >= MAX_PORTS) {
> > >  return EINVAL;
> > >  }
> >
> > it seems that this change broke packet_out with in_port=controller.
>
> Oops.
>
> I sent out a fix for review:
> http://openvswitch.org/pipermail/dev/2013-July/029436.html
> Can you confirm that it fixes the problem for you?
>
> Thanks,
>
> Ben.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] datapath: rhel: Move RHEL OVS hook registration to netdev_rx_handler_register() backport

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 9:00 AM, Thomas Graf  wrote:
> diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
> index fe7e359..d043eae 100644
> --- a/datapath/vport-netdev.c
> +++ b/datapath/vport-netdev.c
> @@ -46,9 +46,8 @@ MODULE_PARM_DESC(vlan_tso, "Enable TSO for VLAN packets");
>  #endif
>
>  #ifdef HAVE_RHEL_OVS_HOOK
> -static atomic_t nr_bridges = ATOMIC_INIT(0);
> +atomic_t nr_bridges = ATOMIC_INIT(0);
>
> -extern struct sk_buff *(*openvswitch_handle_frame_hook)(struct sk_buff *skb);
>  #endif

Can we push this into netdev.c to even further encapsulate the
backport? Also, it doesn't need to be an atomic anymore, does it?
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/3] datapath: Rename skb_network_protocol() to __skb_network_protocol()

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 9:00 AM, Thomas Graf  wrote:
> The function skb_network_protocol() is already defined in
>  upstream but not an exported symbol.
>
> Rename the OVS internal implementation to work around this.
>
> Signed-off-by: Thomas Graf 

Applied, thanks.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/3] datapath: rhel: Account for RHEL specific backports

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 9:00 AM, Thomas Graf  wrote:
> The following symbols have been backported to RHEL and the kernel
> version is no longer an accurate indicator for their presence:
>
>  - skb_gso_segment
>  - netif_skb_features
>  - netif_needs_gso
>
> Signed-off-by: Thomas Graf 

Also applied.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] datapath: rhel: Move RHEL OVS hook registration to netdev_rx_handler_register() backport

2013-07-09 Thread Thomas Graf 

On 07/09/2013 08:06 PM, Jesse Gross wrote:

On Tue, Jul 9, 2013 at 9:00 AM, Thomas Graf  wrote:

diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index fe7e359..d043eae 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -46,9 +46,8 @@ MODULE_PARM_DESC(vlan_tso, "Enable TSO for VLAN packets");
  #endif

  #ifdef HAVE_RHEL_OVS_HOOK
-static atomic_t nr_bridges = ATOMIC_INIT(0);
+atomic_t nr_bridges = ATOMIC_INIT(0);

-extern struct sk_buff *(*openvswitch_handle_frame_hook)(struct sk_buff *skb);
  #endif


Can we push this into netdev.c to even further encapsulate the


Sure


backport? Also, it doesn't need to be an atomic anymore, does it?


Correct, RTNL will serialize this. I'll respin the patch.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2] datapath: rhel: Move RHEL OVS hook registration to netdev_rx_handler_register() backport

2013-07-09 Thread Thomas Graf 
Moves the registration of the RHEL specific OVS hook to the compat
backport of netdev_rx_handler_register(). This moves the hook
unregistration from the RCU callback to the netdev_destroy()
callback directly.

This is purely cosmetic though, the RHEL hook is only used if the
IFF_OVS_DATAPATH flag is present which was removed under RTNL
protection before the RCU callback.

Signed-off-by: Thomas Graf 
---
V2: - Move nr_bridges to compat/netdevice.c
- Don't use atomic_t for nr_bridges

 datapath/linux/compat/include/linux/netdevice.h | 21 -
 datapath/linux/compat/netdevice.c   |  4 
 datapath/vport-netdev.c | 20 
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/datapath/linux/compat/include/linux/netdevice.h 
b/datapath/linux/compat/include/linux/netdevice.h
index b051baf..176d1e6 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -20,6 +20,11 @@ struct net;
 #define to_net_dev(class) container_of(class, struct net_device, 
NETDEV_DEV_MEMBER)
 #endif
 
+#ifdef HAVE_RHEL_OVS_HOOK
+extern struct sk_buff *(*openvswitch_handle_frame_hook)(struct sk_buff *skb);
+extern int nr_bridges;
+#endif
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,26)
 static inline
 struct net *dev_net(const struct net_device *dev)
@@ -84,19 +89,33 @@ extern int skb_checksum_help(struct sk_buff *skb, int);
 extern int skb_checksum_help(struct sk_buff *skb);
 #endif
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,36)
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,36) || \
+defined HAVE_RHEL_OVS_HOOK
 static inline int netdev_rx_handler_register(struct net_device *dev,
 void *rx_handler,
 void *rx_handler_data)
 {
+#ifdef HAVE_RHEL_OVS_HOOK
+   rcu_assign_pointer(dev->ax25_ptr, rx_handler_data);
+   nr_bridges++;
+   rcu_assign_pointer(openvswitch_handle_frame_hook, rx_handler_data);
+#else
if (dev->br_port)
return -EBUSY;
rcu_assign_pointer(dev->br_port, rx_handler_data);
+#endif
return 0;
 }
 static inline void netdev_rx_handler_unregister(struct net_device *dev)
 {
+#ifdef HAVE_RHEL_OVS_HOOK
+   rcu_assign_pointer(dev->ax25_ptr, NULL);
+
+   if (--nr_bridges <= 0)
+   rcu_assign_pointer(openvswitch_handle_frame_hook, NULL);
+#else
rcu_assign_pointer(dev->br_port, NULL);
+#endif
 }
 #endif
 
diff --git a/datapath/linux/compat/netdevice.c 
b/datapath/linux/compat/netdevice.c
index d26fb5e..f03efde 100644
--- a/datapath/linux/compat/netdevice.c
+++ b/datapath/linux/compat/netdevice.c
@@ -1,6 +1,10 @@
 #include 
 #include 
 
+#ifdef HAVE_RHEL_OVS_HOOK
+int nr_bridges = 0;
+#endif
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,38)
 #ifndef HAVE_CAN_CHECKSUM_PROTOCOL
 static bool can_checksum_protocol(unsigned long features, __be16 protocol)
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index fe7e359..06598fa 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -45,12 +45,6 @@ MODULE_PARM_DESC(vlan_tso, "Enable TSO for VLAN packets");
 #define vlan_tso true
 #endif
 
-#ifdef HAVE_RHEL_OVS_HOOK
-static atomic_t nr_bridges = ATOMIC_INIT(0);
-
-extern struct sk_buff *(*openvswitch_handle_frame_hook)(struct sk_buff *skb);
-#endif
-
 static void netdev_port_receive(struct vport *vport, struct sk_buff *skb);
 
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,39)
@@ -171,16 +165,10 @@ static struct vport *netdev_create(const struct 
vport_parms *parms)
}
 
rtnl_lock();
-#ifdef HAVE_RHEL_OVS_HOOK
-   rcu_assign_pointer(netdev_vport->dev->ax25_ptr, vport);
-   atomic_inc(&nr_bridges);
-   rcu_assign_pointer(openvswitch_handle_frame_hook, netdev_frame_hook);
-#else
err = netdev_rx_handler_register(netdev_vport->dev, netdev_frame_hook,
 vport);
if (err)
goto error_unlock;
-#endif
 
dev_set_promiscuity(netdev_vport->dev, 1);
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,24)
@@ -192,9 +180,7 @@ static struct vport *netdev_create(const struct vport_parms 
*parms)
netdev_init();
return vport;
 
-#ifndef HAVE_RHEL_OVS_HOOK
 error_unlock:
-#endif
rtnl_unlock();
 error_put:
dev_put(netdev_vport->dev);
@@ -209,12 +195,6 @@ static void free_port_rcu(struct rcu_head *rcu)
struct netdev_vport *netdev_vport = container_of(rcu,
struct netdev_vport, rcu);
 
-#ifdef HAVE_RHEL_OVS_HOOK
-   rcu_assign_pointer(netdev_vport->dev->ax25_ptr, NULL);
-
-   if (atomic_dec_and_test(&nr_bridges))
-   rcu_assign_pointer(openvswitch_handle_frame_hook, NULL);
-#endif
dev_put(netdev_vport->dev);
ovs_vport_free(vport_from_priv(netdev_vport));
 }
-- 
1.8.3.1

___

Re: [ovs-dev] [PATCH v2] datapath: rhel: Move RHEL OVS hook registration to netdev_rx_handler_register() backport

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 11:36 AM, Thomas Graf  wrote:
> Moves the registration of the RHEL specific OVS hook to the compat
> backport of netdev_rx_handler_register(). This moves the hook
> unregistration from the RCU callback to the netdev_destroy()
> callback directly.
>
> This is purely cosmetic though, the RHEL hook is only used if the
> IFF_OVS_DATAPATH flag is present which was removed under RTNL
> protection before the RCU callback.
>
> Signed-off-by: Thomas Graf 

Applied, thanks for the respin.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] atomic: Suppress sparse warning.

2013-07-09 Thread Ben Pfaff 
On Tue, Jul 09, 2013 at 10:16:35AM -0700, Ethan Jackson wrote:
> With the latest version of sparse, the ATOMIC_VAR_INIT macro
> generates the following warning.  This patch suppresses it.
> 
> warning: Using plain integer as NULL pointer
> 
> Signed-off-by: Ethan Jackson 

Does this actually occur for any use of PTHREAD_MUTEX_INITIALIZER?  If
so then perhaps we should fix it there instead, e.g. in
include/sparse/pthread.h.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] csum: Suppress sparse warning.

2013-07-09 Thread Ben Pfaff 
On Tue, Jul 09, 2013 at 10:16:36AM -0700, Ethan Jackson wrote:
> The latest version of sparse triggers the following warning in the
> checksum tests.  This patch suppresses it.
> 
> warning: too long initializer-string for array of char
> 
> Signed-off-by: Ethan Jackson 

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

Re: [ovs-dev] [export netlink fix v2] datapath: fix bugs in exporting netlink attributes

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 12:25 AM, Andy Zhou  wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index adb918f..a756a15 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -1686,16 +1688,15 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key 
> *swkey,
[...]
> -   if (swkey->phy.in_port != DP_MAX_PORTS) {
> -   /* Exact match upper 16 bits. */
> +   {
> u16 upper_u16;
> upper_u16 = (swkey == output) ? 0 : 0x;
>

DP_MAX_PORTS is an internal kernel value that has no meaning to
userspace so we shouldn't leak it. I think we need to do something
similar to 802.2 where the lack of a attribute implies a particular
value.

I also see another problem here: on flow translation from userspace we
don't set DP_MAX_PORTS when there is no attribute. Instead, it just
remains as zero, which is the internal port.

> @@ -1728,12 +1729,20 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key 
> *swkey,
> } else
> encap = NULL;
>
> -   if ((swkey == output) && (swkey->eth.type == htons(ETH_P_802_2)))
> +   if (swkey->eth.type == htons(ETH_P_802_2)) {
> +   /*
> +* Ethertype 802.2 is represented in the netlink with omitted
> +* OVS_KEY_ATTR_ETHERTYPE in the flow key attribute, and
> +* 0x in the mask attribute.
> +*/
> +   if (swkey != output)
> +   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, 
> htons(0x)))
> +   goto nla_put_failure;
> goto unencap;
> +   }

I'm not sure that this is right - it implies that we must always have
an exact match on the EtherType for 802.2 packets but I think it
really is that we must either have exact match or a complete wildcard
on it. For example, it should be possible to create a flow where the
original packet is 802.2 but you want to have a flow that just matches
on the MAC addresses.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: reset skb->pkt_type when receiving pkt_type of PACKET_OTHERHOST

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 4:07 AM, Isaku Yamahata  wrote:
> The upstream has the fix for dev_forward_skb() to reset pkt_type to
> PACKET_HOST. the change set of 06a23fe31ca3992863721f21bdb0307af93da807
> So the pkt_type of PACKET_OTHERHOST doesn't come in.
> This patch is a workaround for older kernel, reset skb->pkt_type when
> receiving pkt_type of PACKET_OTHERHOST.
>
> When two tunnel ports created on two OVS bridges on same host and
> two ports are the end point of the tunnel, packets are dropped as below.
>
> If a packet has pkt_type of PACKET_OTHERHOST when coming to ovs bridge,
> the packet is dropped by ip_rcv() if rules are installed and skb are
> forwarded by loopback device.
> Packet isn't dropped if flow rule isn't installed in kernel datapath,
> OVS_ACTION_ATTR_OUTPUT is used to send the packet.
>
> netns A |  root netns | netns B
>veth<->veth=ovs bridge=gre<-loop back->gre=ovs bridge=veth<->veth
>
> arp packet ->
> pkt_type
>  BROADCAST>ip_rcv()>
>
>  <- arp reply
> pkt_type
>
>  rule exists
>ip_rcv()<-OTHERHOST
>drop
>
>   pass ^  |
> |  |upcall
> |  V
> OVS_ACTION_ATTR_OUTPUT
> ovs-switchd
>
> Cc: Murphy McCauley 
> Cc: Jesse Gross 
> Signed-off-by: Isaku Yamahata 

Doesn't the upstream change only work if you go through a veth first?
What happens if the physical device is directly attached to OVS?
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] FAQ: Add supported kernel versions for newer OVS releases.

2013-07-09 Thread Jesse Gross 
Reported-by: Kris zhang 
Signed-off-by: Jesse Gross 
---
 FAQ | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/FAQ b/FAQ
index 0210477..6b5d8da 100644
--- a/FAQ
+++ b/FAQ
@@ -146,6 +146,9 @@ A: The following table lists the Linux kernel versions 
against which the
1.7.x  2.6.18 to 3.3
1.8.x  2.6.18 to 3.4
1.9.x  2.6.18 to 3.8
+   1.10.x 2.6.18 to 3.8
+   1.11.x 2.6.18 to 3.8
+   1.12.x 2.6.18 to 3.8
 
Open vSwitch userspace should also work with the Linux kernel module
built into Linux 3.3 and later.
-- 
1.8.1.2

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

[ovs-dev] [PATCH] datapath: Use DP_MAX_PORTS when no IN_PORT attribute is present.

2013-07-09 Thread Jesse Gross 
To indicate that a flow is not associated with any particular in port,
userspace may omit the IN_PORT attribute, which the kernel translates
internally to the special value DP_MAX_PORTS. After the megaflows
changes, this was no longer being done, resulting in it using port 0
(the internal port).

This also adopts a wildcarding scheme similar to 802.2 packets where
a mask can be specified for this non-existent key attribute but it
must either be completely wildcarded or completely exact match.

CC: Andy Zhou 
Signed-off-by: Jesse Gross 
---
 datapath/flow.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/datapath/flow.c b/datapath/flow.c
index adb918f..9ae94bc 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -133,6 +133,10 @@ static bool ovs_match_validate(const struct sw_flow_match 
*match,
| (1ULL << OVS_KEY_ATTR_ARP)
| (1ULL << OVS_KEY_ATTR_ND));
 
+   if (match->key->phy.in_port == DP_MAX_PORTS &&
+   match->mask && (match->mask->key.phy.in_port == 0x))
+   mask_allowed |= (1ULL << OVS_KEY_ATTR_IN_PORT);
+
if (match->key->eth.type == htons(ETH_P_802_2) &&
match->mask && (match->mask->key.eth.type == htons(0x)))
mask_allowed |= (1ULL << OVS_KEY_ATTR_ETHERTYPE);
@@ -1314,6 +1318,8 @@ static int metadata_from_nlattrs(struct sw_flow_match 
*match,  u64 *attrs,
return -EINVAL;
SW_FLOW_KEY_PUT(match, phy.in_port, in_port, is_mask);
*attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
+   } else if (!is_mask) {
+   SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS, is_mask);
}
 
if (*attrs & (1ULL << OVS_KEY_ATTR_SKB_MARK)) {
-- 
1.8.1.2

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

[ovs-dev] [PATCH] datapath: Fix Netlink error message header.

2013-07-09 Thread Jesse Gross 
CC: Andy Zhou 
Signed-off-by: Jesse Gross 
---
 datapath/datapath.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datapath/datapath.h b/datapath/datapath.h
index d14a162..eda87fd 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -206,6 +206,6 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff 
*skb);
 void ovs_dp_notify_wq(struct work_struct *work);
 
 #define OVS_NLERR(fmt, ...) \
-   pr_info_once(fmt "netlink: ", ##__VA_ARGS__)
+   pr_info_once("netlink: " fmt, ##__VA_ARGS__)
 
 #endif /* datapath.h */
-- 
1.8.1.2

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

Re: [ovs-dev] [PATCH] datapath: Use DP_MAX_PORTS when no IN_PORT attribute is present.

2013-07-09 Thread Andy Zhou 
Thanks for the patch. It looks good.


On Tue, Jul 9, 2013 at 2:27 PM, Jesse Gross  wrote:

> To indicate that a flow is not associated with any particular in port,
> userspace may omit the IN_PORT attribute, which the kernel translates
> internally to the special value DP_MAX_PORTS. After the megaflows
> changes, this was no longer being done, resulting in it using port 0
> (the internal port).
>
> This also adopts a wildcarding scheme similar to 802.2 packets where
> a mask can be specified for this non-existent key attribute but it
> must either be completely wildcarded or completely exact match.
>
> CC: Andy Zhou 
> Signed-off-by: Jesse Gross 
> ---
>  datapath/flow.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index adb918f..9ae94bc 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -133,6 +133,10 @@ static bool ovs_match_validate(const struct
> sw_flow_match *match,
> | (1ULL << OVS_KEY_ATTR_ARP)
> | (1ULL << OVS_KEY_ATTR_ND));
>
> +   if (match->key->phy.in_port == DP_MAX_PORTS &&
> +   match->mask && (match->mask->key.phy.in_port == 0x))
> +   mask_allowed |= (1ULL << OVS_KEY_ATTR_IN_PORT);
> +
> if (match->key->eth.type == htons(ETH_P_802_2) &&
> match->mask && (match->mask->key.eth.type == htons(0x)))
> mask_allowed |= (1ULL << OVS_KEY_ATTR_ETHERTYPE);
> @@ -1314,6 +1318,8 @@ static int metadata_from_nlattrs(struct
> sw_flow_match *match,  u64 *attrs,
> return -EINVAL;
> SW_FLOW_KEY_PUT(match, phy.in_port, in_port, is_mask);
> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
> +   } else if (!is_mask) {
> +   SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS, is_mask);
> }
>
> if (*attrs & (1ULL << OVS_KEY_ATTR_SKB_MARK)) {
> --
> 1.8.1.2
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Fix Netlink error message header.

2013-07-09 Thread Andy Zhou 
Looks good. Thanks.


On Tue, Jul 9, 2013 at 2:44 PM, Jesse Gross  wrote:

> CC: Andy Zhou 
> Signed-off-by: Jesse Gross 
> ---
>  datapath/datapath.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index d14a162..eda87fd 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -206,6 +206,6 @@ int ovs_execute_actions(struct datapath *dp, struct
> sk_buff *skb);
>  void ovs_dp_notify_wq(struct work_struct *work);
>
>  #define OVS_NLERR(fmt, ...) \
> -   pr_info_once(fmt "netlink: ", ##__VA_ARGS__)
> +   pr_info_once("netlink: " fmt, ##__VA_ARGS__)
>
>  #endif /* datapath.h */
> --
> 1.8.1.2
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] csum: Suppress sparse warning.

2013-07-09 Thread Ethan Jackson 
Thanks, I'll merge this shortly.

Ethan

On Tue, Jul 9, 2013 at 3:30 PM, Ben Pfaff  wrote:
> On Tue, Jul 09, 2013 at 10:16:36AM -0700, Ethan Jackson wrote:
>> The latest version of sparse triggers the following warning in the
>> checksum tests.  This patch suppresses it.
>>
>> warning: too long initializer-string for array of char
>>
>> Signed-off-by: Ethan Jackson 
>
> Acked-by: Ben Pfaff 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-vsctl: Fix behavioral regression for "--if-exists del-port ".

2013-07-09 Thread Ben Pfaff 
This is mostly historical.  Here's the whole story, as I remember it.
I guess that you remember at least some of this too.

Originally ovs-vswitchd had a text-based config file.  To reconfigure,
you edited the file and then told ovs-vswitchd to reread it.  When we
did the xenserver integration, we needed some better way, so we wrote
ovs-vsctl (a Python script initially) with a set of basic commands
that also edited the config file and told ovs-vswitchd to reread it.
Because XenServer also supported the Linux bridge, the ovs-vsctl
commands had to have semantics very similar to brctl commands.  brctl
commands to list bridge ports, etc., don't include the local port in
the list of ports, so ovs-vsctl didn't either.  That means that
"ovs-vsctl -- --if-exists del-port " never failed, because
ovs-vsctl didn't consider  to be a port (because brctl
didn't).  And we've preserved that behavior over major changes and
rewrites, until now, when we broke it.

So the short answer is that ovs-vsctl doesn't consider a bridge to be
a port in this context, and so --if-exists should just silently do
nothing.

Thanks for the review, I'll apply this to master soon.

On Tue, Jul 09, 2013 at 10:09:05AM -0700, Justin Pettit wrote:
> The behavior seems a little surprising, since the "--if-exists"
> sounds like it will be deleted if it exists, but it's actually just
> avoiding a fatal error and the port will still exist.  It would be
> nicer to give some sort of non-fatal warning in that case.  However,
> that's likely to cause breakage in scripts and most people know not
> to delete the local port, so I'm also fine if you prefer this
> approach.
> 
> --Justin
> 
> 
> On Jul 8, 2013, at 10:48 AM, Ben Pfaff  wrote:
> 
> > Commit 89f3c258fe (ovs-vsctl: Improve error message for "ovs-vsctl del-port
> > ".) changed the behavior of
> >ovs-vsctl --if-exists del-port 
> > from a silent no-op to a hard failure.  This commit fixes this regression.
> > 
> > This caused problems on XenServer, for which the Open vSwitch integration
> > runs commands like:
> >/usr/bin/ovs-vsctl --timeout=20 \
> >-- --with-iface --if-exists del-port xapi103 \
> >-- --if-exists del-br xapi103
> > 
> > Bug #18276.
> > Reported-by: Michael Hu 
> > Signed-off-by: Ben Pfaff 
> > ---
> > tests/ovs-vsctl.at|2 ++
> > utilities/ovs-vsctl.c |9 ++---
> > 2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index fa2c3ff..4449f7a 100644
> > --- a/tests/ovs-vsctl.at
> > +++ b/tests/ovs-vsctl.at
> > @@ -327,6 +327,8 @@ AT_CHECK([RUN_OVS_VSCTL([del-port a])], [1], [],
> >   [ovs-vsctl: cannot delete port a because it is the local port for bridge 
> > a (deleting this port requires deleting the entire bridge)
> > ],
> >   [OVS_VSCTL_CLEANUP])
> > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-port a])], [0], [], [],
> > +  [OVS_VSCTL_CLEANUP])
> > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port a b1])], [1], [],
> >   [ovs-vsctl: "--may-exist add-port a b1" but b1 is actually attached to 
> > bridge b
> > ],
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 2d8c7c7..e679e0d 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > @@ -2021,9 +2021,12 @@ cmd_del_port(struct vsctl_context *ctx)
> > 
> > vsctl_context_populate_cache(ctx);
> > if (find_bridge(ctx, target, false)) {
> > -vsctl_fatal("cannot delete port %s because it is the local port "
> > -"for bridge %s (deleting this port requires deleting "
> > -"the entire bridge)", target, target);
> > +if (must_exist) {
> > +vsctl_fatal("cannot delete port %s because it is the local 
> > port "
> > +"for bridge %s (deleting this port requires 
> > deleting "
> > +"the entire bridge)", target, target);
> > +}
> > +port = NULL;
> > } else if (!with_iface) {
> > port = find_port(ctx, target, must_exist);
> > } else {
> > -- 
> > 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

Re: [ovs-dev] [PATCH 1/2] atomic: Suppress sparse warning.

2013-07-09 Thread Ethan Jackson 
Good point.  I'll send out a cleaner patch in a bit.

Ethan

On Tue, Jul 9, 2013 at 3:29 PM, Ben Pfaff  wrote:
> On Tue, Jul 09, 2013 at 10:16:35AM -0700, Ethan Jackson wrote:
>> With the latest version of sparse, the ATOMIC_VAR_INIT macro
>> generates the following warning.  This patch suppresses it.
>>
>> warning: Using plain integer as NULL pointer
>>
>> Signed-off-by: Ethan Jackson 
>
> Does this actually occur for any use of PTHREAD_MUTEX_INITIALIZER?  If
> so then perhaps we should fix it there instead, e.g. in
> include/sparse/pthread.h.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] atomic: Suppress sparse warning.

2013-07-09 Thread Ethan Jackson 
With the latest version of sparse, the ATOMIC_VAR_INIT macro
generates the following warning.  This patch suppresses it.

warning: Using plain integer as NULL pointer

Signed-off-by: Ethan Jackson 
---
 include/sparse/pthread.h |3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index 7ba6a05..ae72f62 100644
--- a/include/sparse/pthread.h
+++ b/include/sparse/pthread.h
@@ -33,6 +33,9 @@ int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) 
OVS_RELEASES(rwlock);
 int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
 OVS_MUST_HOLD(mutex);
 
+#undef PTHREAD_MUTEX_INITIALIZER
+#define PTHREAD_MUTEX_INITIALIZER {}
+
 #define pthread_mutex_trylock(MUTEX)\
 ({  \
 int retval = pthread_mutex_trylock(mutex);  \
-- 
1.7.9.5

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

Re: [ovs-dev] [PATCH] datapath: Use DP_MAX_PORTS when no IN_PORT attribute is present.

2013-07-09 Thread Jesse Gross 
Thanks, I applied both of these patches.

On Tue, Jul 9, 2013 at 3:16 PM, Andy Zhou  wrote:
> Thanks for the patch. It looks good.
>
>
> On Tue, Jul 9, 2013 at 2:27 PM, Jesse Gross  wrote:
>>
>> To indicate that a flow is not associated with any particular in port,
>> userspace may omit the IN_PORT attribute, which the kernel translates
>> internally to the special value DP_MAX_PORTS. After the megaflows
>> changes, this was no longer being done, resulting in it using port 0
>> (the internal port).
>>
>> This also adopts a wildcarding scheme similar to 802.2 packets where
>> a mask can be specified for this non-existent key attribute but it
>> must either be completely wildcarded or completely exact match.
>>
>> CC: Andy Zhou 
>> Signed-off-by: Jesse Gross 
>> ---
>>  datapath/flow.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index adb918f..9ae94bc 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
>> @@ -133,6 +133,10 @@ static bool ovs_match_validate(const struct
>> sw_flow_match *match,
>> | (1ULL << OVS_KEY_ATTR_ARP)
>> | (1ULL << OVS_KEY_ATTR_ND));
>>
>> +   if (match->key->phy.in_port == DP_MAX_PORTS &&
>> +   match->mask && (match->mask->key.phy.in_port == 0x))
>> +   mask_allowed |= (1ULL << OVS_KEY_ATTR_IN_PORT);
>> +
>> if (match->key->eth.type == htons(ETH_P_802_2) &&
>> match->mask && (match->mask->key.eth.type == htons(0x)))
>> mask_allowed |= (1ULL << OVS_KEY_ATTR_ETHERTYPE);
>> @@ -1314,6 +1318,8 @@ static int metadata_from_nlattrs(struct
>> sw_flow_match *match,  u64 *attrs,
>> return -EINVAL;
>> SW_FLOW_KEY_PUT(match, phy.in_port, in_port, is_mask);
>> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
>> +   } else if (!is_mask) {
>> +   SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS,
>> is_mask);
>> }
>>
>> if (*attrs & (1ULL << OVS_KEY_ATTR_SKB_MARK)) {
>> --
>> 1.8.1.2
>>
>
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [export netlink fix v2] datapath: fix bugs in exporting netlink attributes

2013-07-09 Thread Andy Zhou 
Thanks. An incremental patch is attached that should fix both issues
raised.


On Tue, Jul 9, 2013 at 1:59 PM, Jesse Gross  wrote:

> On Tue, Jul 9, 2013 at 12:25 AM, Andy Zhou  wrote:
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index adb918f..a756a15 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -1686,16 +1688,15 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
> *swkey,
> [...]
> > -   if (swkey->phy.in_port != DP_MAX_PORTS) {
> > -   /* Exact match upper 16 bits. */
> > +   {
> > u16 upper_u16;
> > upper_u16 = (swkey == output) ? 0 : 0x;
> >
>
> DP_MAX_PORTS is an internal kernel value that has no meaning to
> userspace so we shouldn't leak it. I think we need to do something
> similar to 802.2 where the lack of a attribute implies a particular
> value.
>
> I also see another problem here: on flow translation from userspace we
> don't set DP_MAX_PORTS when there is no attribute. Instead, it just
> remains as zero, which is the internal port.
>
> > @@ -1728,12 +1729,20 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
> *swkey,
> > } else
> > encap = NULL;
> >
> > -   if ((swkey == output) && (swkey->eth.type == htons(ETH_P_802_2)))
> > +   if (swkey->eth.type == htons(ETH_P_802_2)) {
> > +   /*
> > +* Ethertype 802.2 is represented in the netlink with
> omitted
> > +* OVS_KEY_ATTR_ETHERTYPE in the flow key attribute, and
> > +* 0x in the mask attribute.
> > +*/
> > +   if (swkey != output)
> > +   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
> htons(0x)))
> > +   goto nla_put_failure;
> > goto unencap;
> > +   }
>
> I'm not sure that this is right - it implies that we must always have
> an exact match on the EtherType for 802.2 packets but I think it
> really is that we must either have exact match or a complete wildcard
> on it. For example, it should be possible to create a flow where the
> original packet is 802.2 but you want to have a flow that just matches
> on the MAC addresses.
>


0001-review-comment.patch
Description: Binary data
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] atomic: Suppress sparse warning.

2013-07-09 Thread Ben Pfaff 
On Tue, Jul 09, 2013 at 03:29:20PM -0700, Ethan Jackson wrote:
> With the latest version of sparse, the ATOMIC_VAR_INIT macro
> generates the following warning.  This patch suppresses it.
> 
> warning: Using plain integer as NULL pointer
> 
> Signed-off-by: Ethan Jackson 

Acked-by: Ben Pfaff 

Please do add a comment that explains why the #undef/#define is
needed.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [export netlink fix v2] datapath: fix bugs in exporting netlink attributes

2013-07-09 Thread Jesse Gross 
Sorry, I meant to say TTL instead of ToS.

On Tue, Jul 9, 2013 at 4:43 PM, Jesse Gross  wrote:
> I applied the combined result but after I did that I noticed that it
> introduces another bug: a ToS of 0 must be explicitly specified, there
> is no default value for this field. Therefore, when we serialize it
> back we should always include it.
>
> Please be more careful when changing code like this, particularly
> protocol code which tends to be large and hard to review. There have
> been a number of subtle bugs that have been introduced because
> behavior was silently changed in a larger patchset without being
> analyzed or called out.
>
> On Tue, Jul 9, 2013 at 4:01 PM, Andy Zhou  wrote:
>> Thanks. An incremental patch is attached that should fix both issues raised.
>>
>>
>> On Tue, Jul 9, 2013 at 1:59 PM, Jesse Gross  wrote:
>>>
>>> On Tue, Jul 9, 2013 at 12:25 AM, Andy Zhou  wrote:
>>> > diff --git a/datapath/flow.c b/datapath/flow.c
>>> > index adb918f..a756a15 100644
>>> > --- a/datapath/flow.c
>>> > +++ b/datapath/flow.c
>>> > @@ -1686,16 +1688,15 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
>>> > *swkey,
>>> [...]
>>> > -   if (swkey->phy.in_port != DP_MAX_PORTS) {
>>> > -   /* Exact match upper 16 bits. */
>>> > +   {
>>> > u16 upper_u16;
>>> > upper_u16 = (swkey == output) ? 0 : 0x;
>>> >
>>>
>>> DP_MAX_PORTS is an internal kernel value that has no meaning to
>>> userspace so we shouldn't leak it. I think we need to do something
>>> similar to 802.2 where the lack of a attribute implies a particular
>>> value.
>>>
>>> I also see another problem here: on flow translation from userspace we
>>> don't set DP_MAX_PORTS when there is no attribute. Instead, it just
>>> remains as zero, which is the internal port.
>>>
>>> > @@ -1728,12 +1729,20 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
>>> > *swkey,
>>> > } else
>>> > encap = NULL;
>>> >
>>> > -   if ((swkey == output) && (swkey->eth.type ==
>>> > htons(ETH_P_802_2)))
>>> > +   if (swkey->eth.type == htons(ETH_P_802_2)) {
>>> > +   /*
>>> > +* Ethertype 802.2 is represented in the netlink with
>>> > omitted
>>> > +* OVS_KEY_ATTR_ETHERTYPE in the flow key attribute, and
>>> > +* 0x in the mask attribute.
>>> > +*/
>>> > +   if (swkey != output)
>>> > +   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>>> > htons(0x)))
>>> > +   goto nla_put_failure;
>>> > goto unencap;
>>> > +   }
>>>
>>> I'm not sure that this is right - it implies that we must always have
>>> an exact match on the EtherType for 802.2 packets but I think it
>>> really is that we must either have exact match or a complete wildcard
>>> on it. For example, it should be possible to create a flow where the
>>> original packet is 802.2 but you want to have a flow that just matches
>>> on the MAC addresses.
>>
>>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [export netlink fix v2] datapath: fix bugs in exporting netlink attributes

2013-07-09 Thread Jesse Gross 
I applied the combined result but after I did that I noticed that it
introduces another bug: a ToS of 0 must be explicitly specified, there
is no default value for this field. Therefore, when we serialize it
back we should always include it.

Please be more careful when changing code like this, particularly
protocol code which tends to be large and hard to review. There have
been a number of subtle bugs that have been introduced because
behavior was silently changed in a larger patchset without being
analyzed or called out.

On Tue, Jul 9, 2013 at 4:01 PM, Andy Zhou  wrote:
> Thanks. An incremental patch is attached that should fix both issues raised.
>
>
> On Tue, Jul 9, 2013 at 1:59 PM, Jesse Gross  wrote:
>>
>> On Tue, Jul 9, 2013 at 12:25 AM, Andy Zhou  wrote:
>> > diff --git a/datapath/flow.c b/datapath/flow.c
>> > index adb918f..a756a15 100644
>> > --- a/datapath/flow.c
>> > +++ b/datapath/flow.c
>> > @@ -1686,16 +1688,15 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
>> > *swkey,
>> [...]
>> > -   if (swkey->phy.in_port != DP_MAX_PORTS) {
>> > -   /* Exact match upper 16 bits. */
>> > +   {
>> > u16 upper_u16;
>> > upper_u16 = (swkey == output) ? 0 : 0x;
>> >
>>
>> DP_MAX_PORTS is an internal kernel value that has no meaning to
>> userspace so we shouldn't leak it. I think we need to do something
>> similar to 802.2 where the lack of a attribute implies a particular
>> value.
>>
>> I also see another problem here: on flow translation from userspace we
>> don't set DP_MAX_PORTS when there is no attribute. Instead, it just
>> remains as zero, which is the internal port.
>>
>> > @@ -1728,12 +1729,20 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key
>> > *swkey,
>> > } else
>> > encap = NULL;
>> >
>> > -   if ((swkey == output) && (swkey->eth.type ==
>> > htons(ETH_P_802_2)))
>> > +   if (swkey->eth.type == htons(ETH_P_802_2)) {
>> > +   /*
>> > +* Ethertype 802.2 is represented in the netlink with
>> > omitted
>> > +* OVS_KEY_ATTR_ETHERTYPE in the flow key attribute, and
>> > +* 0x in the mask attribute.
>> > +*/
>> > +   if (swkey != output)
>> > +   if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>> > htons(0x)))
>> > +   goto nla_put_failure;
>> > goto unencap;
>> > +   }
>>
>> I'm not sure that this is right - it implies that we must always have
>> an exact match on the EtherType for 802.2 packets but I think it
>> really is that we must either have exact match or a complete wildcard
>> on it. For example, it should be possible to create a flow where the
>> original packet is 802.2 but you want to have a flow that just matches
>> on the MAC addresses.
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] datapath: Always include tunnel TTL in serialized Netlink attributes.

2013-07-09 Thread Jesse Gross 
There is no default value for the tunnel TTL so it must always be
included in flow keys sent from userspace to kernel. The kernel
should also respect this convertion when sending flows to userspace
by always including the TTL in tunnel flows.

CC: Andy Zhou 
Signed-off-by: Jesse Gross 
---
 datapath/flow.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index 45b85ab..a70b974 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1289,8 +1289,7 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
if (output->ipv4_tos &&
nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos))
return -EMSGSIZE;
-   if (output->ipv4_ttl &&
-   nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
+   if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
return -EMSGSIZE;
if ((output->tun_flags & TUNNEL_DONT_FRAGMENT) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
-- 
1.8.1.2

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

[ovs-dev] [PATCH] datpath: Fix tunnel TTL flow rejection message.

2013-07-09 Thread Jesse Gross 
There is no default value for the tunnel TTL, so it must be
specified when setting up a new flow. However, the flow rejection
log message indicates that the TTL must be non-zero, which is not
true.

CC: Andy Zhou 
Signed-off-by: Jesse Gross 
---
 datapath/flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index 45b85ab..eb99bab 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1260,7 +1260,7 @@ int ipv4_tun_from_nlattr(const struct nlattr *attr,
}
 
if (!ttl) {
-   OVS_NLERR("IPv4 tunnel TTL is zero.\n");
+   OVS_NLERR("IPv4 tunnel TTL not specified.\n");
return -EINVAL;
}
 
-- 
1.8.1.2

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

Re: [ovs-dev] [PATCH] datapath: Always include tunnel TTL in serialized Netlink attributes.

2013-07-09 Thread Andy Zhou 
Looks good. Did not know TTL=0 is allowed.


On Tue, Jul 9, 2013 at 4:51 PM, Jesse Gross  wrote:

> There is no default value for the tunnel TTL so it must always be
> included in flow keys sent from userspace to kernel. The kernel
> should also respect this convertion when sending flows to userspace
> by always including the TTL in tunnel flows.
>
> CC: Andy Zhou 
> Signed-off-by: Jesse Gross 
> ---
>  datapath/flow.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 45b85ab..a70b974 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -1289,8 +1289,7 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
> if (output->ipv4_tos &&
> nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos))
> return -EMSGSIZE;
> -   if (output->ipv4_ttl &&
> -   nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
> +   if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
> return -EMSGSIZE;
> if ((output->tun_flags & TUNNEL_DONT_FRAGMENT) &&
> nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
> --
> 1.8.1.2
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Always include tunnel TTL in serialized Netlink attributes.

2013-07-09 Thread Jesse Gross 
Thanks, I applied both of these patches.

On Tue, Jul 9, 2013 at 5:11 PM, Andy Zhou  wrote:
> Looks good. Did not know TTL=0 is allowed.
>
>
> On Tue, Jul 9, 2013 at 4:51 PM, Jesse Gross  wrote:
>>
>> There is no default value for the tunnel TTL so it must always be
>> included in flow keys sent from userspace to kernel. The kernel
>> should also respect this convertion when sending flows to userspace
>> by always including the TTL in tunnel flows.
>>
>> CC: Andy Zhou 
>> Signed-off-by: Jesse Gross 
>> ---
>>  datapath/flow.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index 45b85ab..a70b974 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
>> @@ -1289,8 +1289,7 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
>> if (output->ipv4_tos &&
>> nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS,
>> output->ipv4_tos))
>> return -EMSGSIZE;
>> -   if (output->ipv4_ttl &&
>> -   nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL,
>> output->ipv4_ttl))
>> +   if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
>> return -EMSGSIZE;
>> if ((output->tun_flags & TUNNEL_DONT_FRAGMENT) &&
>> nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
>> --
>> 1.8.1.2
>>
>
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: reset skb->pkt_type when receiving pkt_type of PACKET_OTHERHOST

2013-07-09 Thread Isaku Yamahata 
On Tue, Jul 09, 2013 at 02:08:03PM -0700, Jesse Gross wrote:
> On Tue, Jul 9, 2013 at 4:07 AM, Isaku Yamahata  wrote:
> > The upstream has the fix for dev_forward_skb() to reset pkt_type to
> > PACKET_HOST. the change set of 06a23fe31ca3992863721f21bdb0307af93da807
> > So the pkt_type of PACKET_OTHERHOST doesn't come in.
> > This patch is a workaround for older kernel, reset skb->pkt_type when
> > receiving pkt_type of PACKET_OTHERHOST.
> >
> > When two tunnel ports created on two OVS bridges on same host and
> > two ports are the end point of the tunnel, packets are dropped as below.
> >
> > If a packet has pkt_type of PACKET_OTHERHOST when coming to ovs bridge,
> > the packet is dropped by ip_rcv() if rules are installed and skb are
> > forwarded by loopback device.
> > Packet isn't dropped if flow rule isn't installed in kernel datapath,
> > OVS_ACTION_ATTR_OUTPUT is used to send the packet.
> >
> > netns A |  root netns | netns B
> >veth<->veth=ovs bridge=gre<-loop back->gre=ovs bridge=veth<->veth
> >
> > arp packet ->
> > pkt_type
> >  BROADCAST>ip_rcv()>
> >
> >  <- arp reply
> > pkt_type
> >
> >  rule exists
> >ip_rcv()<-OTHERHOST
> >drop
> >
> >    >pass ^  |
> > |  |upcall
> > |  V
> > OVS_ACTION_ATTR_OUTPUT
> > ovs-switchd
> >
> > Cc: Murphy McCauley 
> > Cc: Jesse Gross 
> > Signed-off-by: Isaku Yamahata 
> 
> Doesn't the upstream change only work if you go through a veth first?

Basically right. macvlan/macvtap and l2tp are also covered.
This patch is consistent with iptunnel_pull_header() which sets pkt_type
to PACKET_HOST when receiving skb.


> What happens if the physical device is directly attached to OVS?

If physical device is promiscuous mode off, skb with pkt_type=OTHERHOST
doesn't come in usually. So the behavior is not changed.
If promiscuous mode is on, packets of OTHERHOST is affected with this
patch. And such packets will go through tunnel port and be handled
by host ip layer. This is an expected behavior, I think.
-- 
yamahata
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] A question on the design of OVS GRE tunnel

2013-07-09 Thread Cong Wang 
On Mon, 2013-07-08 at 23:26 -0700, Jesse Gross wrote:
> On Mon, Jul 8, 2013 at 7:41 PM, Cong Wang  wrote:
> > On Mon, 2013-07-08 at 09:28 -0700, Pravin Shelar wrote:
> >> On Mon, Jul 8, 2013 at 2:51 AM, Cong Wang  wrote:
> >> > However, I noticed there is some problem with such design:
> >> >
> >> > I saw very bad performance with the _default_ setup with OVS GRE. After
> >> > digging it a little bit, clearly the cause is that OVS GRE tunnel adds
> >> > an outer IP header and a GRE header for every packet that passed to it,
> >> > which could result in a packet whose length is larger than the MTU of
> >> > the uplink, therefore after the packet goes through OVS, it has to be
> >> > fragmented by IP before going to the wire.
> >> >
> >> I do not understand what do you mean, gre packets greater than MTU
> >> must be fragmented before sent on wire and it is done by GRE-GSO code.
> >>
> >
> > Well, I said fragment, not segment. This is exactly why performance is
> > so bad.
> >
> > In my _default_ setup, every net device on the path has MTU=1500,
> > therefore, the packets coming out of a KVM guest can have length=1500,
> > after they go through OVS GRE tunnel, their length becomes 1538 because
> > of the added GRE header and IP header.
> >
> > After that, since the packets are not GSO (unless you pass vnet_hdr=on
> > to KVM guest), the packets with length=1538 will be _fragmented_ by IP
> > layer, since the dest uplink has MTU=1500 too. This is why I proposed to
> > reuse GRO cell to merge the packets, which requires a netdev...
> 
> Large packets coming from a modern KVM guest will use TSO because this
> is a huge performance win regardless of whether any tunneling is used.
> It doesn't make any sense for the guest IP stack to take a stream of
> packets, split them apart, merge them in the hypervisor stack, and
> split them again before transmission. Any packets potentially worth
> merging will almost certainly have originated as a single buffer in
> the guest, so we should keep them together all the way from the guest
> to the GSO/TSO layer.
> 
> The real problem is that the requested MSS size is not correct. In the
> "best" situation we would first segment the packet to the requested
> size, add the tunnel headers, and then fragment. However, it looks to
> me like the original size is being carried all the way to the GSO
> code, which will then generate packets that are greater than the MTU.
> Both of these can likely be improved upon by either convincing the
> guest to automatically use a lower MSS or adjusting it ourselves.

Yeah, unfortunately this is not easy to discover, people need some
knowledge and some time to find the problem and "fix" it. This is why I
think we should find a way to fix it if possible, or at least document
it.

Thanks!

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

Re: [ovs-dev] [PATCH] datapath: reset skb->pkt_type when receiving pkt_type of PACKET_OTHERHOST

2013-07-09 Thread Jesse Gross 
On Tue, Jul 9, 2013 at 7:48 PM, Isaku Yamahata  wrote:
> On Tue, Jul 09, 2013 at 02:08:03PM -0700, Jesse Gross wrote:
>> On Tue, Jul 9, 2013 at 4:07 AM, Isaku Yamahata  
>> wrote:
>> > The upstream has the fix for dev_forward_skb() to reset pkt_type to
>> > PACKET_HOST. the change set of 06a23fe31ca3992863721f21bdb0307af93da807
>> > So the pkt_type of PACKET_OTHERHOST doesn't come in.
>> > This patch is a workaround for older kernel, reset skb->pkt_type when
>> > receiving pkt_type of PACKET_OTHERHOST.
>> >
>> > When two tunnel ports created on two OVS bridges on same host and
>> > two ports are the end point of the tunnel, packets are dropped as below.
>> >
>> > If a packet has pkt_type of PACKET_OTHERHOST when coming to ovs bridge,
>> > the packet is dropped by ip_rcv() if rules are installed and skb are
>> > forwarded by loopback device.
>> > Packet isn't dropped if flow rule isn't installed in kernel datapath,
>> > OVS_ACTION_ATTR_OUTPUT is used to send the packet.
>> >
>> > netns A |  root netns | netns B
>> >veth<->veth=ovs bridge=gre<-loop back->gre=ovs bridge=veth<->veth
>> >
>> > arp packet ->
>> > pkt_type
>> >  BROADCAST>ip_rcv()>
>> >
>> >  <- arp reply
>> > pkt_type
>> >
>> >  rule exists
>> >ip_rcv()<-OTHERHOST
>> >drop
>> >
>> >   > > exists
>> >pass ^  |
>> > |  |upcall
>> > |  V
>> > OVS_ACTION_ATTR_OUTPUT
>> > ovs-switchd
>> >
>> > Cc: Murphy McCauley 
>> > Cc: Jesse Gross 
>> > Signed-off-by: Isaku Yamahata 
>>
>> Doesn't the upstream change only work if you go through a veth first?
>
> Basically right. macvlan/macvtap and l2tp are also covered.
> This patch is consistent with iptunnel_pull_header() which sets pkt_type
> to PACKET_HOST when receiving skb.
>
>
>> What happens if the physical device is directly attached to OVS?
>
> If physical device is promiscuous mode off, skb with pkt_type=OTHERHOST
> doesn't come in usually. So the behavior is not changed.
> If promiscuous mode is on, packets of OTHERHOST is affected with this
> patch. And such packets will go through tunnel port and be handled
> by host ip layer. This is an expected behavior, I think.

My impression is that this is intended to be a backport and not needed
on new kernels, right? However, in that case won't we have different
behavior in the case of promiscuous mode?
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: reset skb->pkt_type when receiving pkt_type of PACKET_OTHERHOST

2013-07-09 Thread Isaku Yamahata 
On Tue, Jul 09, 2013 at 10:23:42PM -0700, Jesse Gross wrote:
> On Tue, Jul 9, 2013 at 7:48 PM, Isaku Yamahata  wrote:
> > On Tue, Jul 09, 2013 at 02:08:03PM -0700, Jesse Gross wrote:
> >> On Tue, Jul 9, 2013 at 4:07 AM, Isaku Yamahata  
> >> wrote:
> >> > The upstream has the fix for dev_forward_skb() to reset pkt_type to
> >> > PACKET_HOST. the change set of 06a23fe31ca3992863721f21bdb0307af93da807
> >> > So the pkt_type of PACKET_OTHERHOST doesn't come in.
> >> > This patch is a workaround for older kernel, reset skb->pkt_type when
> >> > receiving pkt_type of PACKET_OTHERHOST.
> >> >
> >> > When two tunnel ports created on two OVS bridges on same host and
> >> > two ports are the end point of the tunnel, packets are dropped as below.
> >> >
> >> > If a packet has pkt_type of PACKET_OTHERHOST when coming to ovs bridge,
> >> > the packet is dropped by ip_rcv() if rules are installed and skb are
> >> > forwarded by loopback device.
> >> > Packet isn't dropped if flow rule isn't installed in kernel datapath,
> >> > OVS_ACTION_ATTR_OUTPUT is used to send the packet.
> >> >
> >> > netns A |  root netns | netns B
> >> >veth<->veth=ovs bridge=gre<-loop back->gre=ovs bridge=veth<->veth
> >> >
> >> > arp packet ->
> >> > pkt_type
> >> >  BROADCAST>ip_rcv()>
> >> >
> >> >  <- arp reply
> >> > pkt_type
> >> >
> >> >  rule exists
> >> >ip_rcv()<-OTHERHOST
> >> >drop
> >> >
> >> >    >> > exists
> >> >pass ^  |
> >> > |  |upcall
> >> > |  V
> >> > OVS_ACTION_ATTR_OUTPUT
> >> > ovs-switchd
> >> >
> >> > Cc: Murphy McCauley 
> >> > Cc: Jesse Gross 
> >> > Signed-off-by: Isaku Yamahata 
> >>
> >> Doesn't the upstream change only work if you go through a veth first?
> >
> > Basically right. macvlan/macvtap and l2tp are also covered.
> > This patch is consistent with iptunnel_pull_header() which sets pkt_type
> > to PACKET_HOST when receiving skb.
> >
> >
> >> What happens if the physical device is directly attached to OVS?
> >
> > If physical device is promiscuous mode off, skb with pkt_type=OTHERHOST
> > doesn't come in usually. So the behavior is not changed.
> > If promiscuous mode is on, packets of OTHERHOST is affected with this
> > patch. And such packets will go through tunnel port and be handled
> > by host ip layer. This is an expected behavior, I think.
> 
> My impression is that this is intended to be a backport and not needed
> on new kernels, right? However, in that case won't we have different
> behavior in the case of promiscuous mode?

Ah, that's right. The commit message/comments are not appropriate as you point
out.
Can you please give me suggestion to make progress?

- change commit message/comment?
  as this patch is also necessary for new kernels.

- change patch itself?
  What approach do you prefer?
  I don't think I will be able to persuade netdev people to modify loop
  back device.

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