[PATCH net-next 0/2] ovs: refresh a flow via netlink

[PATCH net-next 0/2] ovs: refresh a flow via netlink

[Samuel Gauthier]

This patchset adds a netlink api to refresh an existing flow in
openvswitch.

When a packet is sent in the openvswitch kernel datapath and no
flow is found, the packet is sent to the ovs-vswitchd daemon,
which will process the packet, and ask the kernel to create a new
flow. The next packets for this flow will be processed by the
kernel datapath. If a flow is not used for a (configurable)
period of time, ovs-vswitchd removes the flow from the kernel.

As a result, it can be tricky to test the kernel datapath against
packets, as the first packets of each flow will have to go
through the ovs-vswitchd daemon. For instance, to do a zeroloss
performance test, you establish the flows, and then you have to
perform your zeroloss test before the flow is removed by
ovs-vswitchd.

It is possible to configure a flow timeout in ovs-vswitchd (using
other_config:max-idle option), but it changes the behavior for
all the flows, which is not always what you want.

I tested this with a patch for the openvswitch tree of the
ovs-dpctl mod-flow command, which adds a --refresh flag. I will
submit the patch if this patchset is accepted.

Samuel Gauthier (2):
  ovs: split ovs_flow_stats_update into skb and stats
  ovs: support to refresh a flow via netlink

 net/openvswitch/datapath.c |  4 +++-
 net/openvswitch/flow.c     | 23 ++++++++++++++++++-----
 net/openvswitch/flow.h     |  5 +++--
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.2.1.62.g3f15098

[PATCH net-next 1/2] ovs: split ovs_flow_stats_update into skb and stats

[Samuel Gauthier]

The function to update statistics takes a skbuff as parameter. It
would be handy to have the statistics update part in one function, and
the skbuff part in another one.

The next commit will make use of the new ovs_flow_stats_update
function.

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
 net/openvswitch/datapath.c |  2 +-
 net/openvswitch/flow.c     | 17 ++++++++++++-----
 net/openvswitch/flow.h     |  4 ++--
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 0cc66a4e492d..8c6dcffe9b62 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -284,7 +284,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
 		goto out;
 	}
 
-	ovs_flow_stats_update(flow, key->tp.flags, skb);
+	ovs_flow_stats_update_skb(flow, key->tp.flags, skb);
 	sf_acts = rcu_dereference(flow->sf_acts);
 	ovs_execute_actions(dp, skb, sf_acts, key);
 
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 0ea128eeeab2..831db351fef9 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -67,12 +67,11 @@ u64 ovs_flow_used_time(unsigned long flow_jiffies)
 
 #define TCP_FLAGS_BE16(tp) (*(__be16 *)&tcp_flag_word(tp) & htons(0x0FFF))
 
-void ovs_flow_stats_update(struct sw_flow *flow, __be16 tcp_flags,
-			   const struct sk_buff *skb)
+static void ovs_flow_stats_update(struct sw_flow *flow, __be16 tcp_flags,
+				  unsigned int count, unsigned int len)
 {
 	struct flow_stats *stats;
 	int node = numa_node_id();
-	int len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
 
 	stats = rcu_dereference(flow->stats[node]);
 
@@ -109,7 +108,7 @@ void ovs_flow_stats_update(struct sw_flow *flow, __be16 tcp_flags,
 							      node);
 				if (likely(new_stats)) {
 					new_stats->used = jiffies;
-					new_stats->packet_count = 1;
+					new_stats->packet_count = count;
 					new_stats->byte_count = len;
 					new_stats->tcp_flags = tcp_flags;
 					spin_lock_init(&new_stats->lock);
@@ -124,13 +123,21 @@ void ovs_flow_stats_update(struct sw_flow *flow, __be16 tcp_flags,
 	}
 
 	stats->used = jiffies;
-	stats->packet_count++;
+	stats->packet_count += count;
 	stats->byte_count += len;
 	stats->tcp_flags |= tcp_flags;
 unlock:
 	spin_unlock(&stats->lock);
 }
 
+void ovs_flow_stats_update_skb(struct sw_flow *flow, __be16 tcp_flags,
+			       const struct sk_buff *skb)
+{
+	int len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
+
+	ovs_flow_stats_update(flow, tcp_flags, 1, len);
+}
+
 /* Must be called with rcu_read_lock or ovs_mutex. */
 void ovs_flow_stats_get(const struct sw_flow *flow,
 			struct ovs_flow_stats *ovs_stats,
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 1d055c559eaf..51e10c4b1ce6 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -210,8 +210,8 @@ static inline bool ovs_identifier_is_key(const struct sw_flow_id *sfid)
 	return !ovs_identifier_is_ufid(sfid);
 }
 
-void ovs_flow_stats_update(struct sw_flow *, __be16 tcp_flags,
-			   const struct sk_buff *);
+void ovs_flow_stats_update_skb(struct sw_flow *, __be16 tcp_flags,
+			       const struct sk_buff *);
 void ovs_flow_stats_get(const struct sw_flow *, struct ovs_flow_stats *,
 			unsigned long *used, __be16 *tcp_flags);
 void ovs_flow_stats_clear(struct sw_flow *);
-- 
2.2.1.62.g3f15098

[PATCH net-next 2/2] ovs: support to refresh a flow via netlink

[Samuel Gauthier]

The used parameter of a flow tells us when it was used for the last
time. It is possible to set this parameter to 0 using the
OVS_FLOW_ATTR_CLEAR attribute, which means 'never used'. But it is not
possible to set this parameter to 'now'.

With this commit, adding OVS_FLOW_ATTR_USED to a 'set flow' netlink
message refreshes the flow used time to the current time. The value in
OVS_FLOW_ATTR_USED attribute is not used in this case.

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
 net/openvswitch/datapath.c | 2 ++
 net/openvswitch/flow.c     | 6 ++++++
 net/openvswitch/flow.h     | 1 +
 3 files changed, 9 insertions(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 8c6dcffe9b62..f2050af3965a 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1183,6 +1183,8 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
 	/* Clear stats. */
 	if (a[OVS_FLOW_ATTR_CLEAR])
 		ovs_flow_stats_clear(flow);
+	if (a[OVS_FLOW_ATTR_USED])
+		ovs_flow_refresh(flow);
 	ovs_unlock();
 
 	if (reply)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 831db351fef9..602795dd3656 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -167,6 +167,12 @@ void ovs_flow_stats_get(const struct sw_flow *flow,
 	}
 }
 
+/* Must be called with rcu_read_lock or ovs_mutex. */
+void ovs_flow_refresh(struct sw_flow *flow)
+{
+	ovs_flow_stats_update(flow, 0, 0, 0);
+}
+
 /* Called with ovs_mutex. */
 void ovs_flow_stats_clear(struct sw_flow *flow)
 {
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 51e10c4b1ce6..4b6b64c999ed 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -214,6 +214,7 @@ void ovs_flow_stats_update_skb(struct sw_flow *, __be16 tcp_flags,
 			       const struct sk_buff *);
 void ovs_flow_stats_get(const struct sw_flow *, struct ovs_flow_stats *,
 			unsigned long *used, __be16 *tcp_flags);
+void ovs_flow_refresh(struct sw_flow *);
 void ovs_flow_stats_clear(struct sw_flow *);
 u64 ovs_flow_used_time(unsigned long flow_jiffies);
 
-- 
2.2.1.62.g3f15098

Re: [PATCH net-next 0/2] ovs: refresh a flow via netlink

[Jesse Gross]

On Wed, Mar 16, 2016 at 8:07 AM, Samuel Gauthier
<samuel.gauthier@6wind.com> wrote:
> This patchset adds a netlink api to refresh an existing flow in
> openvswitch.
>
> When a packet is sent in the openvswitch kernel datapath and no
> flow is found, the packet is sent to the ovs-vswitchd daemon,
> which will process the packet, and ask the kernel to create a new
> flow. The next packets for this flow will be processed by the
> kernel datapath. If a flow is not used for a (configurable)
> period of time, ovs-vswitchd removes the flow from the kernel.
>
> As a result, it can be tricky to test the kernel datapath against
> packets, as the first packets of each flow will have to go
> through the ovs-vswitchd daemon. For instance, to do a zeroloss
> performance test, you establish the flows, and then you have to
> perform your zeroloss test before the flow is removed by
> ovs-vswitchd.
>
> It is possible to configure a flow timeout in ovs-vswitchd (using
> other_config:max-idle option), but it changes the behavior for
> all the flows, which is not always what you want.

It seems to me that it would be preferable to implement the necessary
behavior in userspace to handle this directly. The logic that is
removing the flow is in userspace, so rather than asking the kernel to
lie about the current state of things, we can just modify the logic to
handle this case.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net-next 0/2] ovs: refresh a flow via netlink

[Samuel Gauthier]

2016-03-17 0:23 GMT+01:00 Jesse Gross <jesse@kernel.org>:
> On Wed, Mar 16, 2016 at 8:07 AM, Samuel Gauthier
> <samuel.gauthier@6wind.com> wrote:
>> This patchset adds a netlink api to refresh an existing flow in
>> openvswitch.
>>
>> When a packet is sent in the openvswitch kernel datapath and no
>> flow is found, the packet is sent to the ovs-vswitchd daemon,
>> which will process the packet, and ask the kernel to create a new
>> flow. The next packets for this flow will be processed by the
>> kernel datapath. If a flow is not used for a (configurable)
>> period of time, ovs-vswitchd removes the flow from the kernel.
>>
>> As a result, it can be tricky to test the kernel datapath against
>> packets, as the first packets of each flow will have to go
>> through the ovs-vswitchd daemon. For instance, to do a zeroloss
>> performance test, you establish the flows, and then you have to
>> perform your zeroloss test before the flow is removed by
>> ovs-vswitchd.
>>
>> It is possible to configure a flow timeout in ovs-vswitchd (using
>> other_config:max-idle option), but it changes the behavior for
>> all the flows, which is not always what you want.
>
> It seems to me that it would be preferable to implement the necessary
> behavior in userspace to handle this directly. The logic that is
> removing the flow is in userspace, so rather than asking the kernel to
> lie about the current state of things, we can just modify the logic to
> handle this case.

It seemed like a problem limited to the kernel datapath (i.e.: not to
the other ovs datapaths), so it made sense to me to fix it by a
netlink API.
The idea was to do something similar to the OVS_FLOW_ATTR_CLEAR
attribute (which sets the flow statistics and used field to 0).

This said, I could have a look to a pure userland solution, but I am
not sure how to do it. Could you elaborate what you have in mind?

Thanks.

Re: [PATCH net-next 0/2] ovs: refresh a flow via netlink

[Jesse Gross]

On Thu, Mar 17, 2016 at 9:24 AM, Samuel Gauthier
<samuel.gauthier@6wind.com> wrote:
> 2016-03-17 0:23 GMT+01:00 Jesse Gross <jesse@kernel.org>:
>> On Wed, Mar 16, 2016 at 8:07 AM, Samuel Gauthier
>> <samuel.gauthier@6wind.com> wrote:
>>> This patchset adds a netlink api to refresh an existing flow in
>>> openvswitch.
>>>
>>> When a packet is sent in the openvswitch kernel datapath and no
>>> flow is found, the packet is sent to the ovs-vswitchd daemon,
>>> which will process the packet, and ask the kernel to create a new
>>> flow. The next packets for this flow will be processed by the
>>> kernel datapath. If a flow is not used for a (configurable)
>>> period of time, ovs-vswitchd removes the flow from the kernel.
>>>
>>> As a result, it can be tricky to test the kernel datapath against
>>> packets, as the first packets of each flow will have to go
>>> through the ovs-vswitchd daemon. For instance, to do a zeroloss
>>> performance test, you establish the flows, and then you have to
>>> perform your zeroloss test before the flow is removed by
>>> ovs-vswitchd.
>>>
>>> It is possible to configure a flow timeout in ovs-vswitchd (using
>>> other_config:max-idle option), but it changes the behavior for
>>> all the flows, which is not always what you want.
>>
>> It seems to me that it would be preferable to implement the necessary
>> behavior in userspace to handle this directly. The logic that is
>> removing the flow is in userspace, so rather than asking the kernel to
>> lie about the current state of things, we can just modify the logic to
>> handle this case.
>
> It seemed like a problem limited to the kernel datapath (i.e.: not to
> the other ovs datapaths), so it made sense to me to fix it by a
> netlink API.

I don't think that is true - the flow eviction logic is in userspace,
so it should be common to all datapaths. What the kernel is providing
is really very simple, just the last used time.

> The idea was to do something similar to the OVS_FLOW_ATTR_CLEAR
> attribute (which sets the flow statistics and used field to 0).
>
> This said, I could have a look to a pure userland solution, but I am
> not sure how to do it. Could you elaborate what you have in mind?

Well, truthfully, I would just use the existing max-idle option that
you mentioned. I'm not entirely sure what the issue is with it
applying to all flows, however, presumably it could be modified to
only apply to a subset if necessary. max-idle is implemented purely in
userspace, so I would start by looking at that and see how to tailor
it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net-next 0/2] ovs: refresh a flow via netlink

[David Miller]

From: Samuel Gauthier <samuel.gauthier@6wind.com>
Date: Wed, 16 Mar 2016 16:07:50 +0100

> This patchset adds a netlink api to refresh an existing flow in
> openvswitch.

This is too late for net-next, please resubmit this after the
merge window.

Thanks.