Ben Pfaff | 24 Apr 02:06 2014

[PATCH] bridge: When ports disappear from a datapath, add them back.

Before commit 2a73b1d73d4bdb (bridge: Reconfigure in single pass.), if a
port disappeared, for one reason or another, from a datapath, the next
bridge reconfiguration pass would notice and, if the port was still
configured in the database, add the port back to the datapath.  That
commit, however, removed the logic from bridge_refresh_ofp_port() that
did that and failed to add the same logic to the replacement function
bridge_delete_or_reconfigure_ports().  This commit fixes the problem.

To see this problem on a Linux kernel system:

ovs-vsctl add-br br0                             # 1
tunctl -t tap                                    # 2
ovs-vsctl add-port br0 tap                       # 3
ovs-dpctl show                                   # 4
tunctl -d tap                                    # 5
ovs-dpctl show                                   # 6
tunctl -t tap                                    # 7
ovs-vsctl del-port tap -- add-port br0 tap       # 8
ovs-dpctl show                                   # 9

Steps 1-4 create a bridge and a tap and add it to the bridge and
demonstrate that the tap is part of the datapath.  Step 5 and 6 delete
the tap and demonstrate that it has therefore disappeared from the
datapath.  Step 7 recreates a tap with the same name, and step 8
forces ovs-vswitchd to reconfigure.  Step 9 shows the effect of the
fix: without the fix, the new tap is not added back to the datapath;
with this fix, it is.

Special thanks to Gurucharan Shetty <gshetty@...> for finding a
simple reproduction case and then bisecting to find the commit that
(Continue reading)

Jarno Rajahalme | 24 Apr 01:20 2014

[PATCH 1/3] ofproto: RCU postpone rule destruction.

This allows rules to be used without taking references while RCU

The last step of destroying an ofproto also needs to be postponed, as
the rule destruction requires the class structure to be available at
the postponed destruction callback.

Signed-off-by: Jarno Rajahalme <jrajahalme@...>
 ofproto/ofproto.c |   40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f16005c..f10d3ae 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
 <at>  <at>  -259,7 +259,6  <at>  <at>  struct ofport_usage {

 /* rule. */
-static void ofproto_rule_destroy__(struct rule *);
 static void ofproto_rule_send_removed(struct rule *, uint8_t reason);
 static bool rule_is_modifiable(const struct rule *rule,
                                enum ofputil_flow_mod_flags flag);
 <at>  <at>  -1372,7 +1371,8  <at>  <at>  ofproto_destroy(struct ofproto *p)

-    ofproto_destroy__(p);
+    /* Destroying rules is deferred, must have 'ofproto' around for them. */
(Continue reading)

Andy Zhou | 23 Apr 22:26 2014

[PATCH] ofproto-dpif: Improve code clarity and comments on recirc changes to rule_dpif_lookup

This patch improves the code readability and comments on the
recirculation related changes to rule_dpif_lookup() base on off-line
discussions with Jarno.  There is no behavior changes.

Signed-off-by: Andy Zhou <azhou@...>
 ofproto/ofproto-dpif.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6a725e4..983cad5 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
 <at>  <at>  -3199,14 +3199,21  <at>  <at>  rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
     uint8_t table_id;

     if (ofproto_dpif_get_enable_recirc(ofproto)) {
-        if (flow->recirc_id == 0) {
-            if (wc) {
-                wc->masks.recirc_id = UINT32_MAX;
-            }
-            table_id = 0;
-        } else {
-            table_id = TBL_INTERNAL;
+        /* Always exactly match recirc_id since datapath supports
+         * recirculation.  */
+        if (wc) {
+            wc->masks.recirc_id = UINT32_MAX;
(Continue reading)

Zoltan Kiss | 23 Apr 22:05 2014

Userspace Netlink MMAP status


I would like to ask, what's the status of enabling Netlink MMAP in the 
userspace? I'm interested to see this progressing, but digging the mail 
archive I've found it run into scalability issues:

And also it can't handle frags at the moment properly:

I was thinking about this scalability issue, and I think maybe we 
shouldn't stick to the 16 KB frame size. I think in most cases the 
packets we are actually sending up are small ones, in case of TCP the 
packets of the handshakes are less than 100 bytes. And for the rest, we 
can call genlmsg_new_unicast() with the full packet size, so it will 
fall back to non-mmaped communication. And this would solve the 
frag-handling problem as well.
Another approach to keep the frame size lower is to pass references to 
the frags, which would point outside the ring buffer, to the actual page 
(which need to be mapped for the userspace). I don't know how feasible 
would that be, but huge linear buffers also has to be sliced up to frags.


Ben Pfaff | 23 Apr 18:21 2014

Re: [PATCH 07/11] daemon-windows: Recognize --no-chdir option for windows.

On Wed, Apr 23, 2014 at 8:47 AM, Gurucharan Shetty <shettyg@...> wrote:
> On Tue, Apr 22, 2014 at 2:58 PM, Ben Pfaff <blp@...> wrote:
>> On Tue, Apr 22, 2014 at 01:40:55PM -0700, Gurucharan Shetty wrote:
>>> > Should OVS chdir to the root (on all drives?) on Windows
>>> > also?
>>> Looks like chdir("/") on windows takes me to C:/ . So doing that on
>>> windows should be good. I will re-spin this.
>> OK.
>> If Windows works like it used to, there's an independent notion of the
>> current working directory on every drive, so one would have to do
>> something like:
>>     char c;
>>     for (c = 'a'; c <= 'z'; c++) {
>>         char dir[] = {c, ':', '/', 0};
>>         chdir(dir);
>>     }
>> to really chdir to the root everywhere.  But I do not know whether it
>> matters.
> I see that this notion is true in current versions of windows too.
> I suppose a working directory for every drive is useful if a program
> changes its drive midway in the program. (This is what I understood
> after trying reading some msdn documentation, though there are
> probably other reasons too.)
(Continue reading)

Pravin B Shelar | 23 Apr 17:49 2014

[PATCH V2 2/4] datapath: Add flow mask cache.

On every packet OVS needs to lookup flow-table with every mask
until it finds a match. The packet flow-key is first masked
with mask in the list and then the masked key is looked up in
flow-table.  Therefore number of masks can affect packet
processing performance.

Following patch adds mask index to mask cache from last
pakcet lookup in same flow.  Index of mask is stored in
this cache. This cache is searched by 5 tuple hash (skb rxhash).

Signed-off-by: Pravin B Shelar <pshelar@...>
Fixed according to comments from Thomas Graf
- Added comment for cache lookup.
- check for zero hash
- break of loop in case of hash collision.
 datapath/datapath.c   |    3 +-
 datapath/flow_table.c |  104 +++++++++++++++++++++++++++++++++++++++++++------
 datapath/flow_table.h |   11 +++++-
 3 files changed, 103 insertions(+), 15 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index fcaafd1..10706f5 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
 <at>  <at>  -253,7 +253,8  <at>  <at>  void ovs_dp_process_packet_with_key(struct sk_buff *skb,
 	stats = this_cpu_ptr(dp->stats_percpu);

 	/* Look up flow. */
(Continue reading)

Pravin B Shelar | 23 Apr 17:49 2014

[PATCH V2 4/4] datapath: Compact mask list array.

Along with flow-table rehashing OVS can compact masks array.  This
allows us to calculate highest index for mask array.

Signed-off-by: Pravin B Shelar <pshelar@...>
 datapath/flow_table.c |   24 ++++++++++++++++++++----
 datapath/flow_table.h |    2 +-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index c8bd9d1..ea4100f 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
 <at>  <at>  -230,6 +230,7  <at>  <at>  static struct mask_array *tbl_mask_array_alloc(int size)

 	new->count = 0;
 	new->max = size;
+	new->hi_index = 0;

 	return new;
 <at>  <at>  -252,6 +253,8  <at>  <at>  static int tbl_mask_array_realloc(struct flow_table *tbl, int size)
 				new->masks[new->count++] = old->masks[i];
+	new->hi_index = new->count;
 	rcu_assign_pointer(tbl->mask_array, new);

 	if (old)
(Continue reading)

Pravin B Shelar | 23 Apr 17:49 2014

[PATCH V2 3/4] datapath: Convert mask list in mask array.

mask caches index of mask in mask_list.  On packet recv OVS
need to traverse mask-list to get cached mask.  Therefore array
is better for retrieving cached mask.  This also allows better
cache replacement algorithm by directly checking mask's existence.

Signed-off-by: Pravin B Shelar <pshelar@...>
 datapath/flow.h       |    1 -
 datapath/flow_table.c |  200 ++++++++++++++++++++++++++++++++++++++-----------
 datapath/flow_table.h |    8 +-
 3 files changed, 163 insertions(+), 46 deletions(-)

diff --git a/datapath/flow.h b/datapath/flow.h
index d05a9f4..2018691 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
 <at>  <at>  -127,7 +127,6  <at>  <at>  struct sw_flow_key_range {
 struct sw_flow_mask {
 	int ref_count;
 	struct rcu_head rcu;
-	struct list_head list;
 	struct sw_flow_key_range range;
 	struct sw_flow_key key;
diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index cc0eaf2..c8bd9d1 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
 <at>  <at>  -47,6 +47,7  <at>  <at> 
 #include "vlan.h"
(Continue reading)

Pravin B Shelar | 23 Apr 17:49 2014

[PATCH V2 1/4] datapath: Move table destroy to dp-rcu callback.

Ths simplifies flow-table-destroy API.  This change is required
for following patches.

Signed-off-by: Pravin B Shelar <pshelar@...>
Added comment
 datapath/datapath.c   |    5 ++---
 datapath/flow_table.c |    8 +++++---
 datapath/flow_table.h |    2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index aa4c109..fcaafd1 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
 <at>  <at>  -187,6 +187,7  <at>  <at>  static void destroy_dp_rcu(struct rcu_head *rcu)
 	struct datapath *dp = container_of(rcu, struct datapath, rcu);

+	ovs_flow_tbl_destroy(&dp->table);
 <at>  <at>  -1447,7 +1448,7  <at>  <at>  err_destroy_ports_array:
-	ovs_flow_tbl_destroy(&dp->table, false);
+	ovs_flow_tbl_destroy(&dp->table);
(Continue reading)

Zoltan Kiss | 23 Apr 15:45 2014

[PATCH v2] lib/util: Input validation in str_to_uint

This function returns true when 's' is negative or greater than UINT_MAX. Also,
the representation of 'int' and 'unsigned int' is implementation dependent, so
converting [INT_MAX..UINT_MAX] values with str_to_int is fragile.
Instead, we should convert straight to 'long long' and do a boundary check
before returning the converted value.
This patch also move the function to the .c file as it's not-trivial now, and
deletes the other str_to_u* functions as they are not used.

Signed-off-by: Zoltan Kiss <zoltan.kiss@...>
- remove OVS_UNLIKELY and unnecessary ()
- move the function to util.c as it became not-trivial
- remove the other str_to_u* functions as they are not used

diff --git a/lib/util.c b/lib/util.c
index 3f08c4a..54065fe 100644
--- a/lib/util.c
+++ b/lib/util.c
 <at>  <at>  -613,6 +613,21  <at>  <at>  str_to_llong(const char *s, int base, long long *x)

+str_to_uint(const char *s, int base, unsigned int *u)
+    long long ll;
+    bool ok = str_to_llong(s, base, &ll);
+    if (!ok || ll < 0 || ll > UINT_MAX) {
+	*u = 0;
(Continue reading)

Simon Horman | 23 Apr 07:06 2014

[PATCH] run-ryu: Use unix socket rather than patch ports

My understanding of the implementation of patch ports is that they
are rather special, being handled as a special case inside
compose_output_action__() and do not exist in the datapath.

A side effect of the implementation of patch ports (though perhaps not the
portion mentioned above) is that the OFPUTIL_PC_PORT_DOWN bit may not be
set via a port mod message. In particular, the call to
netdev_turn_flags_on() in update_port_config() fails.

There is a test provided by Ryu that test this via port mod and thus fails.

While that test could be modified or the results ignored it seems to me
that it would be best if ryu-check used ports which were more fully
featured and not special cases.

Thus this patch moves run-ryu to use unix socket backed ports rather than
patch ports.

Signed-off-by: Simon Horman <horms@...>


* I believe a more significant problem with the use of patch ports
  is that they will require (more) special case code in order to correctly
  handle recirculation. As Ryu provides many tests that exercise
  recirculation for MPLS it would be nice if they could be used to exercise
  recirculation for MPLS (which I have provided patches for separately[1])
  without the need to add more special-case code for that purpose.

  I believe that patch ports are also incompatible with recirculation for
(Continue reading)