gmane.network.openvswitch.devel

Re: [flow_metadata 0/5] improve and use flow_metadata more widely

Rajahalme, Jarno (NSN - FI/Espoo — 2013-05-19T13:45:32

Ben,

On my reading the first three patches seem fine. I thought that the fact that the semantics of the struct flow_metadata has changed from "exportable metadata" to "all metadata, some of which is exportable to controllers" deserves a note somewhere, but then the original comment on flow_metadata is even more valid after these changes ("Represents the metadata fields of struct flow.").

I don't think there has been a decision to not make the hole in flow_tnl explicit, even though existing code relies on them being cleared (as in commit_odp_tunnel_action()).

This might not be as clean as you'd like, but would it be portable to repeat the definition of flow_metadata as the first fields of the struct flow (maybe via a macro) and then cast a struct flow * to a struct flow_metadata * when needed?

  Jarno

On May 16, 2013, at 23:44 , ext Ben Pfaff wrote:

[PATCH v2] meta-flow: Add MFF_IN_PORT_OXM,a 32-bit in_port.

Jarno Rajahalme — 2013-05-19T12:44:53

This helps get rid of one special case in nx_pull_raw() and allows
loading of 32-bit values from/to OXM_OF_IN_PORT in NXAST_LEARN actions.
Previously the 16-bit limit acted the same on both NXM_OF_IN_PORT and
OXM_OF_IN_PORT, even though OF1.1+ controllers would expect OXM_OF_IN_PORT
to be 32 bits wide.

Signed-off-by: Jarno Rajahalme <jarno.rajahalme-OYasijW0DpE< at >public.gmane.org>
---
v2: Avoid duplicating existing code.

 lib/meta-flow.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/meta-flow.h |    2 ++
 lib/nx-match.c  |   13 +---------
 tests/learn.at  |   22 +++++++++++++++++
 4 files changed, 96 insertions(+), 12 deletions(-)

diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 16850ec..c59d82a 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
< at >< at > -116,6 +116,15 < at >< at > static const struct mf_field mf_fields[MFF_N_IDS] = {
         MFP_NONE,
         true,
         NXM_OF_IN_PORT, "NXM_OF_IN_PORT",
+        NXM_OF_IN_PORT, "NXM_OF_IN_PORT",
+    }, {
+        MFF_IN_PORT_OXM, "in_port_

[PATCH] ofproto/ofproto-dpif.c: Fix a typo in functionheader

Alex Wang — 2013-05-19T04:33:25

There is a typo in the "bundle_add_port()" function header.

This patch fixes it.

Signed-off-by: Alex Wang <alexw-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 ofproto/ofproto-dpif.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c4f7d25..5cc56d1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
< at >< at > -2472,7 +2472,7 < at >< at > bundle_del_port(struct ofport_dpif *port)
 }
 
 static bool
-bundle_add_port(struct ofbundle *bundle, uint32_t ofp_port,
+bundle_add_port(struct ofbundle *bundle, uint16_t ofp_port,
                 struct lacp_slave_settings *lacp)
 {
     struct ofport_dpif *port;

Re: [PATCH] netdev-dummy: Remove FreeBSD dependency.

Ben Pfaff — 2013-05-18T15:30:44

Thank you for the review.  I applied this to master.

Re: [PATCH] netdev-dummy: Remove FreeBSD dependency.

Ed Maste — 2013-05-18T04:12:12

Acked-by: Ed Maste <emaste-h+KGxgPPiopAfugRpC6u6w< at >public.gmane.org>

Looks good to me, and unit tests continue to pass on FreeBSD.

[PATCH] netdev-dummy: Remove FreeBSD dependency.

Ben Pfaff — 2013-05-17T22:57:20

There's no particular reason that netdev_dummy_register() has to care about
the particular OS, except that the tests like to use the special Linux-only
tunnel vport types.  But that can be done better, I think, by just always
registering them from netdev_dummy_register() and making that function
idempotent, so that calling it twice under Linux has no additional effect.
This commit implements that solution.

CC: Ed Maste <emaste-h+KGxgPPiopAfugRpC6u6w< at >public.gmane.org>
Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 lib/netdev-dummy.c |   10 +---------
 lib/netdev-vport.c |    8 ++++++--
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 908fef2..14b286b 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
< at >< at > -36,12 +36,6 < at >< at >
 
 VLOG_DEFINE_THIS_MODULE(netdev_dummy);
 
-#ifdef __FreeBSD__
-#define FREE_BSD 1
-#else
-#define FREE_BSD 0
-#endif
-
 struct netdev_dummy {
     struct netdev up;
     uint8_t hwaddr[ETH_ADDR_L

[oftest v3 5/5] tests: Add support for running OFTest.

Ben Pfaff — 2013-05-17T22:23:50

Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 Makefile.am       |    1 +
 NEWS              |    2 +
 README-OFTest     |   77 +++++++++++++++++++++++++++++++++++++++++++
 tests/automake.mk |    6 +++
 tests/run-oftest  |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 README-OFTest
 create mode 100755 tests/run-oftest

diff --git a/Makefile.am b/Makefile.am
index 36beb6c..193b19e 100644
--- a/Makefile.am
+++ b/Makefile.am
< at >< at > -55,6 +55,7 < at >< at > EXTRA_DIST = \
 NOTICE \
 OPENFLOW-1.1+ \
 PORTING \
+README-OFTest \
 README-gcov \
 README-lisp \
 REPORTING-BUGS \
diff --git a/NEWS b/NEWS
index 4cb4499..7340898 100644
--- a/NEWS
+++ b/NEWS
< at >< at > -4,6 +4,8 < at >< at > post-v1.11.0
       * New support for matching outer source and destination IP address
         of tunneled packets, for tunnel ports configured with the newly
 added "remote_ip=flow" and "local_ip=flow" options.
+    - New "check-oftest" Makefil

[oftest v3 4/5] netdev-dummy: Add "pstream" option forconnecting a dummy to a process.

Ben Pfaff — 2013-05-17T22:23:49

Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 lib/netdev-dummy.c |  244 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 232 insertions(+), 12 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 8c182a3..a617218 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
< at >< at > -31,6 +31,8 < at >< at >
 #include "poll-loop.h"
 #include "shash.h"
 #include "sset.h"
+#include "stream.h"
+#include "unaligned.h"
 #include "unixctl.h"
 #include "vlog.h"
 
< at >< at > -42,6 +44,12 < at >< at > VLOG_DEFINE_THIS_MODULE(netdev_dummy);
 #define FREE_BSD 0
 #endif
 
+struct dummy_stream {
+    struct stream *stream;
+    struct ofpbuf rxbuf;
+    struct list txq;
+};
+
 struct netdev_dummy {
     struct netdev up;
     uint8_t hwaddr[ETH_ADDR_LEN];
< at >< at > -51,6 +59,10 < at >< at > struct netdev_dummy {
     unsigned int change_seq;
     int ifindex;
 
+    struct pstream *pstream;
+    struct dummy_stream *streams;
+    size_t n_streams;
+
     struct list rxes;           /* List of child "netdev

[oftest v3 0/5] add support for OFTest

Ben Pfaff — 2013-05-17T22:23:45

This is the latest version of the oftest series.  The first version
was posted Oct. 25, 2012.  The second version was posted Feb. 1, 2013.
The only difference between v2 and v3 is considerable work rebasing
due to the restructuring of the netdev library interface.

Ben Pfaff (5):
  netdev-dummy: Limit receive queue length to 100 packets.
  netdev-dummy: Factor some netdev_dummy_receive() code out into new
    function.
  netdev-dummy: Drop "nobody listened" reply from netdev-dummy/receive.
  netdev-dummy: Add "pstream" option for connecting a dummy to a
    process.
  tests: Add support for running OFTest.

 Makefile.am           |    1 +
 NEWS                  |    2 +
 README-OFTest         |   77 ++++++++++++++
 lib/netdev-dummy.c    |  267 +++++++++++++++++++++++++++++++++++++++++++++---
 tests/automake.mk     |    6 +
 tests/ofproto-dpif.at |   24 ++---
 tests/run-oftest      |   94 +++++++++++++++++
 7 files changed, 438 insertions(+), 33 deletions(-)
 create mode 100644 README-OFTest
 create mode 1007

[oftest v3 2/5] netdev-dummy: Factor somenetdev_dummy_receive() code out into new function.

Ben Pfaff — 2013-05-17T22:23:47

An upcoming patch will add another user.

Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 lib/netdev-dummy.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 62d8a1b..ce84a0c 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
< at >< at > -441,6 +441,26 < at >< at > eth_from_packet_or_flow(const char *s)
     return packet;
 }
 
+static int
+netdev_dummy_queue_packet(struct netdev_dummy *dummy,
+                          const void *data, size_t size)
+{
+    struct netdev_rx_dummy *rx;
+    int n_listeners;
+
+    n_listeners = 0;
+    LIST_FOR_EACH (rx, node, &dummy->rxes) {
+        if (rx->recv_queue_len < NETDEV_DUMMY_MAX_QUEUE) {
+            struct ofpbuf *copy = ofpbuf_clone_data(data, size);
+            list_push_back(&rx->recv_queue, &copy->list_node);
+            rx->recv_queue_len++;
+            n_listeners++;
+        }
+    }
+
+    return n_listeners;
+}
+
 static void
 netde

[oftest v3 3/5] netdev-dummy: Drop "nobody listened"reply from netdev-dummy/receive.

Ben Pfaff — 2013-05-17T22:23:48

Ethan pointed out that this wasn't very useful.

Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 lib/netdev-dummy.c    |   16 +++-------------
 tests/ofproto-dpif.at |   24 ++++++++----------------
 2 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index ce84a0c..8c182a3 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
< at >< at > -441,24 +441,19 < at >< at > eth_from_packet_or_flow(const char *s)
     return packet;
 }
 
-static int
+static void
 netdev_dummy_queue_packet(struct netdev_dummy *dummy,
                           const void *data, size_t size)
 {
     struct netdev_rx_dummy *rx;
-    int n_listeners;
 
-    n_listeners = 0;
     LIST_FOR_EACH (rx, node, &dummy->rxes) {
         if (rx->recv_queue_len < NETDEV_DUMMY_MAX_QUEUE) {
             struct ofpbuf *copy = ofpbuf_clone_data(data, size);
             list_push_back(&rx->recv_queue, &copy->list_node);
             rx->recv_queue_len++;
-            n_listeners++;

[oftest v3 1/5] netdev-dummy: Limit receive queue lengthto 100 packets.

Ben Pfaff — 2013-05-17T22:23:46

It doesn't seem like a good idea to allow the queue length to grow without
bound, even for a test-only device.

Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
 lib/netdev-dummy.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 908fef2..62d8a1b 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
< at >< at > -54,10 +54,15 < at >< at > struct netdev_dummy {
     struct list rxes;           /* List of child "netdev_rx_dummy"s. */
 };
 
+/* Max 'recv_queue_len' in struct netdev_dummy. */
+#define NETDEV_DUMMY_MAX_QUEUE 100
+
 struct netdev_rx_dummy {
     struct netdev_rx up;
     struct list node;           /* In netdev_dummy's "rxes" list. */
     struct list recv_queue;
+    int recv_queue_len;         /* list_size(&recv_queue). */
+    bool listening;
 };
 
 static struct shash dummy_netdevs = SHASH_INITIALIZER(&dummy_netdevs);
< at >< at > -159,6 +164,7 < at >< at > netdev_dummy_rx_open(struct netdev *netdev_, struct netdev_rx **

Re: [PATCH v8 1/5] Add execute_actions

Jesse Gross — 2013-05-17T20:40:10

Here's a combined set of comments for all of the patches in the
execute-actions code:
 * The convention used other places in the code when passing around
void pointers and casting them is to have the variable named with an
underscore be the void *.
 * I would extract the userdata for userspace actions in
execute_actions() and pass it directly to the userspace function
pointer.
 * I don't think there is a lot of value in the asserts in
execute_actions(). I think it's valid to pass NULL for 'dp' and 'key'
in these situations if you want. The function pointers can't be NULL
but that will stop the program just as quickly as an assert anyways.
 * The later patches pass around a huge number of arguments. We should
find some way of encapsulating them in a struct. It might even be
possible to store them directly in the flow. It deviates slightly from
other behavior because we don't update the flow when we change the
packet but it would clean a lot of other things up.
 * We already do some action execution in usersp

Re: [xlate 4/4] ofproto-dpif: Revamp xlate_actions() interface.

Ben Pfaff — 2013-05-17T19:24:04

I'm concerned about putting a 512+ byte structure into a facet.  I
think the size of xlate_out should therefore be reduced.

I'd like to look at this series again when you've revised it.

Thanks,

Ben.

Re: [xlate 3/4] ofproto-dpif: Rename action_xlate_ctx.

Ben Pfaff — 2013-05-17T18:25:57

Sure, fine with me.

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

Ben Pfaff — 2013-05-17T18:21:38

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


I agree with Justin's comment.

subfacet_create() refers to subfacet_make_actions() in a comment, but
there is no longer such a function.

handle_flow_miss_with_facet() previously translated the actions
separately for each packet, if it was necessary due to slow-pathing.
The new version doesn't do that.  Are you sure that every packet for a
facet always has the same datapath actions, even if the facet is
slow-pathed?  We have a comment in facet_check_consistency() that says
that they can vary:

    /* The actions for slow-path flows may legitimately vary from one
     * packet to the next.  We're done. */

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

        - Pu

Re: [PATCH v2 0/2] Enhancements to formatting of PACKET_{IN, OUT} messages

Simon Horman — 2013-05-17T06:08:43

Thanks!

Re: [PATCH v2 0/2] Enhancements to formatting of PACKET_{IN, OUT} messages

Ben Pfaff — 2013-05-17T05:50:40

Both patches applied without change, thanks!

Re: [PATCH v3] bridge: Fix use of wild pointer in iface_do_create().

Ben Pfaff — 2013-05-17T05:50:09

Thanks!  Applied to master.

On Thu, May 16, 2013 at 10:48:56PM -0700, Justin Pettit wrote:

Re: [PATCH v3] bridge: Fix use of wild pointer iniface_do_create().

Justin Pettit — 2013-05-17T05:48:56

Looks good.

--Justin


On May 16, 2013, at 10:45 PM, Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org> wrote:

[PATCH v3] bridge: Fix use of wild pointer iniface_do_create().

Ben Pfaff — 2013-05-17T05:45:43

'netdev' was not initialized at this point in the function, so the cleanup
at the error label referenced a wild pointer.

Introduced in commit 94a538422d4b (netdev: Prevent using reserved names).

CC: Alex Wang <alexw-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
Signed-off-by: Ben Pfaff <blp-l0M0P4e3n4LQT0dZR+AlfA< at >public.gmane.org>
---
v1->v2: Fix Alex's email address.
v2->v3: Fix bug I introduced noted by Justin.

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 28e306e..e10036c 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
< at >< at > -1415,7 +1415,7 < at >< at > iface_do_create(const struct bridge *br,
 {
     const struct ovsrec_interface *iface_cfg = if_cfg->cfg;
     const struct ovsrec_port *port_cfg = if_cfg->parent;
-    struct netdev *netdev;
+    struct netdev *netdev = NULL;
     int error;
 
     if (netdev_is_reserved_name(iface_cfg->name)) {