[GIT net] Open vSwitch

[GIT net] Open vSwitch

[Jesse Gross]

A handful of bug fixes for net/3.14. High level fixes are:
 * Regression introduced by the zerocopy changes, particularly with
   old userspaces.
 * A few bugs lingering from the introduction of megaflows.
 * Overly zealous error checking that is now being triggered frequently
   in common cases.

The following changes since commit 9895c503ef5b32c1ff4c4c224d6e8db2935dc3c0:

  ksz884x: delete useless variable (2014-01-15 13:43:03 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jesse/openvswitch.git fixes

for you to fetch changes up to dd3bceef108f99e21900c872d8b7ec632cfb88ac:

  openvswitch: Suppress error messages on megaflow updates (2014-02-02 16:39:48 -0800)

----------------------------------------------------------------
Andy Zhou (2):
      openvswitch: Fix kernel panic on ovs_flow_free
      openvswitch: Suppress error messages on megaflow updates

Daniele Di Proietto (1):
      openvswitch: Fix ovs_dp_cmd_msg_size()

Pravin B Shelar (1):
      openvswitch: Fix ovs_flow_free() ovs-lock assert.

Thomas Graf (1):
      openvswitch: Pad OVS_PACKET_ATTR_PACKET if linear copy was performed

 net/openvswitch/datapath.c   | 22 +++++++----
 net/openvswitch/flow_table.c | 87 +++++++++++++++++++++-----------------------
 net/openvswitch/flow_table.h |  2 +-
 3 files changed, 57 insertions(+), 54 deletions(-)

[PATCH net 1/5] openvswitch: Pad OVS_PACKET_ATTR_PACKET if linear copy was performed

[Jesse Gross]

From: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>

While the zerocopy method is correctly omitted if user space
does not support unaligned Netlink messages. The attribute is
still not padded correctly as skb_zerocopy() will not ensure
padding and the attribute size is no longer pre calculated
though nla_reserve() which ensured padding previously.

This patch applies appropriate padding if a linear data copy
was performed in skb_zerocopy().

Signed-off-by: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
Acked-by: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/datapath.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index df46928..3ca9121 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -396,7 +396,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 		.dst_sk = ovs_dp_get_net(dp)->genl_sock,
 		.snd_portid = upcall_info->portid,
 	};
-	size_t len;
+	size_t len, plen;
 	unsigned int hlen;
 	int err, dp_ifindex;
 
@@ -466,6 +466,11 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 
 	skb_zerocopy(user_skb, skb, skb->len, hlen);
 
+	/* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */
+	if (!(dp->user_features & OVS_DP_F_UNALIGNED) &&
+	    (plen = (ALIGN(user_skb->len, NLA_ALIGNTO) - user_skb->len)) > 0)
+		memset(skb_put(user_skb, plen), 0, plen);
+
 	((struct nlmsghdr *) user_skb->data)->nlmsg_len = user_skb->len;
 
 	err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb, upcall_info->portid);
-- 
1.8.3.2

Re: [PATCH net 1/5] openvswitch: Pad OVS_PACKET_ATTR_PACKET if linear copy was performed

[Sergei Shtylyov]

Hello.

On 03-02-2014 5:08, Jesse Gross wrote:

> From: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>

> While the zerocopy method is correctly omitted if user space
> does not support unaligned Netlink messages. The attribute is
> still not padded correctly as skb_zerocopy() will not ensure
> padding and the attribute size is no longer pre calculated
> though nla_reserve() which ensured padding previously.

> This patch applies appropriate padding if a linear data copy
> was performed in skb_zerocopy().

> Signed-off-by: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
> Acked-by: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> ---
>   net/openvswitch/datapath.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index df46928..3ca9121 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
[...]
> @@ -466,6 +466,11 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>
>   	skb_zerocopy(user_skb, skb, skb->len, hlen);
>
> +	/* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */
> +	if (!(dp->user_features & OVS_DP_F_UNALIGNED) &&
> +	    (plen = (ALIGN(user_skb->len, NLA_ALIGNTO) - user_skb->len)) > 0)

    This shouldn't pass checkpatch.pl which complains about assignments inside 
*if* statements.

WBR, Sergei

Re: [PATCH net 1/5] openvswitch: Pad OVS_PACKET_ATTR_PACKET if linear copy was performed

[Jesse Gross]

On Mon, Feb 3, 2014 at 8:43 AM, Sergei Shtylyov
<sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> Hello.
>
>
> On 03-02-2014 5:08, Jesse Gross wrote:
>
>> From: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
>
>
>> While the zerocopy method is correctly omitted if user space
>> does not support unaligned Netlink messages. The attribute is
>> still not padded correctly as skb_zerocopy() will not ensure
>> padding and the attribute size is no longer pre calculated
>> though nla_reserve() which ensured padding previously.
>
>
>> This patch applies appropriate padding if a linear data copy
>> was performed in skb_zerocopy().
>
>
>> Signed-off-by: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
>> Acked-by: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
>> ---
>>   net/openvswitch/datapath.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>
>
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index df46928..3ca9121 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>
> [...]
>
>> @@ -466,6 +466,11 @@ static int queue_userspace_packet(struct datapath
>> *dp, struct sk_buff *skb,
>>
>>         skb_zerocopy(user_skb, skb, skb->len, hlen);
>>
>> +       /* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */
>> +       if (!(dp->user_features & OVS_DP_F_UNALIGNED) &&
>> +           (plen = (ALIGN(user_skb->len, NLA_ALIGNTO) - user_skb->len)) >
>> 0)
>
>
>    This shouldn't pass checkpatch.pl which complains about assignments
> inside *if* statements.

I've fixed both of these issues and will resubmit.

[PATCH net 2/5] openvswitch: Fix kernel panic on ovs_flow_free

[Jesse Gross]

From: Andy Zhou <azhou-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>

Both mega flow mask's reference counter and per flow table mask list
should only be accessed when holding ovs_mutex() lock. However
this is not true with ovs_flow_table_flush(). The patch fixes this bug.

Reported-by: Joe Stringer <joestringer-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andy Zhou <azhou-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/datapath.c   |  9 +++--
 net/openvswitch/flow_table.c | 84 +++++++++++++++++++++-----------------------
 net/openvswitch/flow_table.h |  2 +-
 3 files changed, 47 insertions(+), 48 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 3ca9121..ea0b30f 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -55,6 +55,7 @@
 
 #include "datapath.h"
 #include "flow.h"
+#include "flow_table.h"
 #include "flow_netlink.h"
 #include "vport-internal_dev.h"
 #include "vport-netdev.h"
@@ -160,7 +161,6 @@ static void destroy_dp_rcu(struct rcu_head *rcu)
 {
 	struct datapath *dp = container_of(rcu, struct datapath, rcu);
 
-	ovs_flow_tbl_destroy(&dp->table);
 	free_percpu(dp->stats_percpu);
 	release_net(ovs_dp_get_net(dp));
 	kfree(dp->ports);
@@ -1284,7 +1284,7 @@ err_destroy_ports_array:
 err_destroy_percpu:
 	free_percpu(dp->stats_percpu);
 err_destroy_table:
-	ovs_flow_tbl_destroy(&dp->table);
+	ovs_flow_tbl_destroy(&dp->table, false);
 err_free_dp:
 	release_net(ovs_dp_get_net(dp));
 	kfree(dp);
@@ -1311,10 +1311,13 @@ static void __dp_destroy(struct datapath *dp)
 	list_del_rcu(&dp->list_node);
 
 	/* OVSP_LOCAL is datapath internal port. We need to make sure that
-	 * all port in datapath are destroyed first before freeing datapath.
+	 * all ports in datapath are destroyed first before freeing datapath.
 	 */
 	ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
 
+	/* RCU destroy the flow table */
+	ovs_flow_tbl_destroy(&dp->table, true);
+
 	call_rcu(&dp->rcu, destroy_dp_rcu);
 }
 
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index c58a0fe..bd14052 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -153,29 +153,27 @@ static void rcu_free_flow_callback(struct rcu_head *rcu)
 	flow_free(flow);
 }
 
-static void flow_mask_del_ref(struct sw_flow_mask *mask, bool deferred)
+void ovs_flow_free(struct sw_flow *flow, bool deferred)
 {
-	if (!mask)
+	if (!flow)
 		return;
 
-	BUG_ON(!mask->ref_count);
-	mask->ref_count--;
+	ASSERT_OVSL();
 
-	if (!mask->ref_count) {
-		list_del_rcu(&mask->list);
-		if (deferred)
-			kfree_rcu(mask, rcu);
-		else
-			kfree(mask);
-	}
-}
+	if (flow->mask) {
+		struct sw_flow_mask *mask = flow->mask;
 
-void ovs_flow_free(struct sw_flow *flow, bool deferred)
-{
-	if (!flow)
-		return;
+		BUG_ON(!mask->ref_count);
+		mask->ref_count--;
 
-	flow_mask_del_ref(flow->mask, deferred);
+		if (!mask->ref_count) {
+			list_del_rcu(&mask->list);
+			if (deferred)
+				kfree_rcu(mask, rcu);
+			else
+				kfree(mask);
+		}
+	}
 
 	if (deferred)
 		call_rcu(&flow->rcu, rcu_free_flow_callback);
@@ -188,26 +186,9 @@ static void free_buckets(struct flex_array *buckets)
 	flex_array_free(buckets);
 }
 
+
 static void __table_instance_destroy(struct table_instance *ti)
 {
-	int i;
-
-	if (ti->keep_flows)
-		goto skip_flows;
-
-	for (i = 0; i < ti->n_buckets; i++) {
-		struct sw_flow *flow;
-		struct hlist_head *head = flex_array_get(ti->buckets, i);
-		struct hlist_node *n;
-		int ver = ti->node_ver;
-
-		hlist_for_each_entry_safe(flow, n, head, hash_node[ver]) {
-			hlist_del(&flow->hash_node[ver]);
-			ovs_flow_free(flow, false);
-		}
-	}
-
-skip_flows:
 	free_buckets(ti->buckets);
 	kfree(ti);
 }
@@ -258,20 +239,38 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
 
 static void table_instance_destroy(struct table_instance *ti, bool deferred)
 {
+	int i;
+
 	if (!ti)
 		return;
 
+	if (ti->keep_flows)
+		goto skip_flows;
+
+	for (i = 0; i < ti->n_buckets; i++) {
+		struct sw_flow *flow;
+		struct hlist_head *head = flex_array_get(ti->buckets, i);
+		struct hlist_node *n;
+		int ver = ti->node_ver;
+
+		hlist_for_each_entry_safe(flow, n, head, hash_node[ver]) {
+			hlist_del_rcu(&flow->hash_node[ver]);
+			ovs_flow_free(flow, deferred);
+		}
+	}
+
+skip_flows:
 	if (deferred)
 		call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
 	else
 		__table_instance_destroy(ti);
 }
 
-void ovs_flow_tbl_destroy(struct flow_table *table)
+void ovs_flow_tbl_destroy(struct flow_table *table, bool deferred)
 {
 	struct table_instance *ti = ovsl_dereference(table->ti);
 
-	table_instance_destroy(ti, false);
+	table_instance_destroy(ti, deferred);
 }
 
 struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
@@ -504,16 +503,11 @@ static struct sw_flow_mask *mask_alloc(void)
 
 	mask = kmalloc(sizeof(*mask), GFP_KERNEL);
 	if (mask)
-		mask->ref_count = 0;
+		mask->ref_count = 1;
 
 	return mask;
 }
 
-static void mask_add_ref(struct sw_flow_mask *mask)
-{
-	mask->ref_count++;
-}
-
 static bool mask_equal(const struct sw_flow_mask *a,
 		       const struct sw_flow_mask *b)
 {
@@ -554,9 +548,11 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
 		mask->key = new->key;
 		mask->range = new->range;
 		list_add_rcu(&mask->list, &tbl->mask_list);
+	} else {
+		BUG_ON(!mask->ref_count);
+		mask->ref_count++;
 	}
 
-	mask_add_ref(mask);
 	flow->mask = mask;
 	return 0;
 }
diff --git a/net/openvswitch/flow_table.h b/net/openvswitch/flow_table.h
index 1996e34..baaeb10 100644
--- a/net/openvswitch/flow_table.h
+++ b/net/openvswitch/flow_table.h
@@ -60,7 +60,7 @@ void ovs_flow_free(struct sw_flow *, bool deferred);
 
 int ovs_flow_tbl_init(struct flow_table *);
 int ovs_flow_tbl_count(struct flow_table *table);
-void ovs_flow_tbl_destroy(struct flow_table *table);
+void ovs_flow_tbl_destroy(struct flow_table *table, bool deferred);
 int ovs_flow_tbl_flush(struct flow_table *flow_table);
 
 int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
-- 
1.8.3.2

[PATCH net 3/5] openvswitch: Fix ovs_dp_cmd_msg_size()

[Jesse Gross]

From: Daniele Di Proietto <daniele.di.proietto-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

commit 43d4be9cb55f3bac5253e9289996fd9d735531db (openvswitch: Allow user space
to announce ability to accept unaligned Netlink messages) introduced
OVS_DP_ATTR_USER_FEATURES netlink attribute in datapath responses,
but the attribute size was not taken into account in ovs_dp_cmd_msg_size().

Signed-off-by: Daniele Di Proietto <daniele.di.proietto-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/datapath.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ea0b30f..8b392cc 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1084,6 +1084,7 @@ static size_t ovs_dp_cmd_msg_size(void)
 	msgsize += nla_total_size(IFNAMSIZ);
 	msgsize += nla_total_size(sizeof(struct ovs_dp_stats));
 	msgsize += nla_total_size(sizeof(struct ovs_dp_megaflow_stats));
+	msgsize += nla_total_size(sizeof(u32)); /* OVS_DP_ATTR_USER_FEATURES */
 
 	return msgsize;
 }
-- 
1.8.3.2

[PATCH net 4/5] openvswitch: Fix ovs_flow_free() ovs-lock assert.

[Jesse Gross]

From: Pravin B Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>

ovs_flow_free() is not called under ovs-lock during packet
execute path (ovs_packet_cmd_execute()). Since packet execute
does not touch flow->mask, there is no need to take that
lock either. So move assert in case where flow->mask is checked.

Found by code inspection.

Signed-off-by: Pravin B Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/flow_table.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index bd14052..ad0bda0 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -158,11 +158,12 @@ void ovs_flow_free(struct sw_flow *flow, bool deferred)
 	if (!flow)
 		return;
 
-	ASSERT_OVSL();
-
 	if (flow->mask) {
 		struct sw_flow_mask *mask = flow->mask;
 
+		/* ovs-lock is required to protect mask-refcount and
+		 * mask list. */
+		ASSERT_OVSL();
 		BUG_ON(!mask->ref_count);
 		mask->ref_count--;
 
-- 
1.8.3.2

Re: [PATCH net 4/5] openvswitch: Fix ovs_flow_free() ovs-lock assert.

[Sergei Shtylyov]

Hello.

On 03-02-2014 5:08, Jesse Gross wrote:

> From: Pravin B Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>

> ovs_flow_free() is not called under ovs-lock during packet
> execute path (ovs_packet_cmd_execute()). Since packet execute
> does not touch flow->mask, there is no need to take that
> lock either. So move assert in case where flow->mask is checked.

> Found by code inspection.

> Signed-off-by: Pravin B Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> ---
>   net/openvswitch/flow_table.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index bd14052..ad0bda0 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -158,11 +158,12 @@ void ovs_flow_free(struct sw_flow *flow, bool deferred)
>   	if (!flow)
>   		return;
>
> -	ASSERT_OVSL();
> -
>   	if (flow->mask) {
>   		struct sw_flow_mask *mask = flow->mask;
>
> +		/* ovs-lock is required to protect mask-refcount and
> +		 * mask list. */

    Networking multi-line comment style is:

/* bla
  * bla
  */

WBR, Sergei

[PATCH net 5/5] openvswitch: Suppress error messages on megaflow updates

[Jesse Gross]

From: Andy Zhou <azhou-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>

With subfacets, we'd expect megaflow updates message to carry
the original micro flow. If not, EINVAL is returned and kernel
logs an error message.  Now that the user space subfacet layer is
removed, it is expected that flow updates can arrive with a
micro flow other than the original. Change the return code to
EEXIST and remove the kernel error log message.

Reported-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andy Zhou <azhou-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/datapath.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 8b392cc..947a9a8 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -857,11 +857,8 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
 			goto err_unlock_ovs;
 
 		/* The unmasked key has to be the same for flow updates. */
-		error = -EINVAL;
-		if (!ovs_flow_cmp_unmasked_key(flow, &match)) {
-			OVS_NLERR("Flow modification message rejected, unmasked key does not match.\n");
+		if (!ovs_flow_cmp_unmasked_key(flow, &match))
 			goto err_unlock_ovs;
-		}
 
 		/* Update actions. */
 		old_acts = ovsl_dereference(flow->sf_acts);
-- 
1.8.3.2

[RFC PATCH] openvswitch: Use net_ratelimit in OVS_NLERR

[Joe Perches]

Perhaps it'd make sense to use net_ratelimit()
instead of printk_once for OVS_NLERR
---
 net/openvswitch/datapath.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 6be9fbb..0f5e77c 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -194,7 +194,9 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *, u32 pid, u32 seq,
 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("netlink: " fmt, ##__VA_ARGS__)
-
+#define OVS_NLERR(fmt, ...)					\
+do {								\
+	if (net_ratelimit())					\
+		pr_info("netlink: " fmt, ##__VA_ARGS__);	\
+} while (0)
 #endif /* datapath.h */

Re: [RFC PATCH] openvswitch: Use net_ratelimit in OVS_NLERR

[Jesse Gross]

On Sun, Feb 2, 2014 at 10:55 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> Perhaps it'd make sense to use net_ratelimit()
> instead of printk_once for OVS_NLERR

I guess I could see it going either way but I'm not sure I see a
strong argument for changing.

Re: [RFC PATCH] openvswitch: Use net_ratelimit in OVS_NLERR

[Joe Perches]

On Mon, 2014-02-03 at 13:46 -0800, Jesse Gross wrote:
> On Sun, Feb 2, 2014 at 10:55 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> > Perhaps it'd make sense to use net_ratelimit()
> > instead of printk_once for OVS_NLERR
> 
> I guess I could see it going either way but I'm not sure I see a
> strong argument for changing.

pr_<level>_once is a per-site flag.

Some of these messages look as if seeing them
multiple times could be useful.

Re: [RFC PATCH] openvswitch: Use net_ratelimit in OVS_NLERR

[Jesse Gross]

On Mon, Feb 3, 2014 at 2:08 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Mon, 2014-02-03 at 13:46 -0800, Jesse Gross wrote:
>> On Sun, Feb 2, 2014 at 10:55 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > Perhaps it'd make sense to use net_ratelimit()
>> > instead of printk_once for OVS_NLERR
>>
>> I guess I could see it going either way but I'm not sure I see a
>> strong argument for changing.
>
> pr_<level>_once is a per-site flag.
>
> Some of these messages look as if seeing them
> multiple times could be useful.

OK, I guess it's fine. Can you provide a signed-off-by line?

[PATCH] openvswitch: Use net_ratelimit in OVS_NLERR

[Joe Perches]

Each use of pr_<level>_once has a per-site flag.

Some of the OVS_NLERR messages look as if seeing them
multiple times could be useful, so use net_ratelimit()
instead of pr_info_once.

Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> 
---
> OK, I guess it's fine. Can you provide a signed-off-by line?

 net/openvswitch/datapath.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 6be9fbb..0f5e77c 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -194,7 +194,9 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *, u32 pid, u32 seq,
 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("netlink: " fmt, ##__VA_ARGS__)
-
+#define OVS_NLERR(fmt, ...)					\
+do {								\
+	if (net_ratelimit())					\
+		pr_info("netlink: " fmt, ##__VA_ARGS__);	\
+} while (0)
 #endif /* datapath.h */

Re: [PATCH] openvswitch: flow_netlink: Use pr_fmt to OVS_NLERR output

[Joe Perches]

Add "openvswitch: " prefix to OVS_NLERR output
to match the other OVS_NLERR output of datapath.c

Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 net/openvswitch/flow_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 4d000ac..c4e1326 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -16,6 +16,8 @@
  * 02110-1301, USA
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include "flow.h"
 #include "datapath.h"
 #include <linux/uaccess.h>

Re: [PATCH] openvswitch: flow_netlink: Use pr_fmt to OVS_NLERR output

[Jesse Gross]

On Mon, Feb 3, 2014 at 5:18 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> Add "openvswitch: " prefix to OVS_NLERR output
> to match the other OVS_NLERR output of datapath.c
>
> Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>

Also applied.

Re: [PATCH] openvswitch: Use net_ratelimit in OVS_NLERR

[Jesse Gross]

On Mon, Feb 3, 2014 at 5:06 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> Each use of pr_<level>_once has a per-site flag.
>
> Some of the OVS_NLERR messages look as if seeing them
> multiple times could be useful, so use net_ratelimit()
> instead of pr_info_once.
>
> Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>

Applied, thanks.

[GIT net] Open vSwitch

[Jesse Gross]

A handful of bug fixes for net/3.14. High level fixes are:
 * Regressions introduced by the zerocopy changes, particularly with
   old userspaces.
 * A few bugs lingering from the introduction of megaflows.
 * Overly zealous error checking that is now being triggered frequently
   in common cases.

The following changes since commit bce3ea81d5b2a33ed0e275d58a45f10ce95cbd50:

  xen-netfront: handle backend CLOSED without CLOSING (2014-02-04 20:42:36 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jesse/openvswitch.git fixes

for you to fetch changes up to c14e0953ca51dbcb8d1ac92acbdcff23d0caa158:

  openvswitch: Suppress error messages on megaflow updates (2014-02-04 22:32:38 -0800)

----------------------------------------------------------------
Andy Zhou (2):
      openvswitch: Fix kernel panic on ovs_flow_free
      openvswitch: Suppress error messages on megaflow updates

Daniele Di Proietto (1):
      openvswitch: Fix ovs_dp_cmd_msg_size()

Pravin B Shelar (1):
      openvswitch: Fix ovs_flow_free() ovs-lock assert.

Thomas Graf (1):
      openvswitch: Pad OVS_PACKET_ATTR_PACKET if linear copy was performed

 net/openvswitch/datapath.c   | 23 ++++++++----
 net/openvswitch/flow_table.c | 88 ++++++++++++++++++++++----------------------
 net/openvswitch/flow_table.h |  2 +-
 3 files changed, 60 insertions(+), 53 deletions(-)

[PATCH net 4/5] openvswitch: Fix ovs_flow_free() ovs-lock assert.

[Jesse Gross]

From: Pravin B Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>

ovs_flow_free() is not called under ovs-lock during packet
execute path (ovs_packet_cmd_execute()). Since packet execute
does not touch flow->mask, there is no need to take that
lock either. So move assert in case where flow->mask is checked.

Found by code inspection.

Signed-off-by: Pravin B Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/flow_table.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index bd14052..3c268b3 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -158,11 +158,13 @@ void ovs_flow_free(struct sw_flow *flow, bool deferred)
 	if (!flow)
 		return;
 
-	ASSERT_OVSL();
-
 	if (flow->mask) {
 		struct sw_flow_mask *mask = flow->mask;
 
+		/* ovs-lock is required to protect mask-refcount and
+		 * mask list.
+		 */
+		ASSERT_OVSL();
 		BUG_ON(!mask->ref_count);
 		mask->ref_count--;
 
-- 
1.8.3.2