On Wed, Jan 09, 2013 at 03:43:47PM -0800, Ethan Jackson wrote:
> With this patch, ovs-vswitchd uses flow based tunneling
> exclusively. I.E. each kind of tunnel shares a single tunnel
> backer in the datapath. Tunnel headers are set by userspace using
> the ipv4_tunnel datapath action. There are still some significant
> pieces of work to do, but the basic building blocks are there to
> begin testing.
>
> Signed-off-by: Jesse Gross <[email protected]>
> Signed-off-by: Ethan Jackson <[email protected]>
The commit message could use more information on the high-level changes
in the patch. Some of it took me a while to figure out. Here's my
suggestion for text to add:
The configuration of individual tunnels is now a userspace
responsibility, so netdev-vport no longer marshals and unmarshals
Netlink attributes for tunnel configuration, instead only storing
the configuration internally.
> + - Tunneling requires the version of the kernel module paired Open vSwitch
> + 1.9.0 or later.
There's something missing from that sentence. Perhaps add "with"
following "paired"?
Looking at the set_tunnel_config() change, the reason you didn't bother
breaking it up into parse-then-assemble-Netlink in an earlier patch is
obvious. Thanks.
port_construct() makes a call to VLOG_ERR passing a new-line terminated
string. Our usual style is to omit the new-line, although it makes no
actual difference. (You didn't originate this but I noticed it anyway.)
I found may_dpif_port_del() a little hard to read because of the two
different types, ofport and ofport_dpif, used unnecessarily in this
case. I think it's easier if you always use ofport_dpif, as in this
incremental:
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 87f3332..36bb37a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2963,24 +2963,25 @@ static bool
may_dpif_port_del(struct ofproto_dpif *ofproto, struct ofport_dpif *ofport)
{
struct ofproto_dpif *ofproto_iter;
- struct ofport *ofport_up;
if (!netdev_get_tunnel_config(ofport->up.netdev)) {
return true;
}
HMAP_FOR_EACH (ofproto_iter, all_ofproto_dpifs_node, &all_ofproto_dpifs) {
+ struct ofport_dpif *iter;
+
if (ofproto->backer != ofproto_iter->backer) {
continue;
}
- HMAP_FOR_EACH (ofport_up, hmap_node, &ofproto_iter->up.ports) {
- if (&ofport->up == ofport_up) {
+ HMAP_FOR_EACH (iter, up.hmap_node, &ofproto_iter->up.ports) {
+ if (ofport == iter) {
continue;
}
if (!strcmp(netdev_get_dpif_port(ofport->up.netdev),
- netdev_get_dpif_port(ofport_up->netdev))) {
+ netdev_get_dpif_port(iter->up.netdev))) {
return false;
}
}
Also may_dpif_port_del() could use a comment explaining what it's up to.
Did you consider reference-counting the backer ports instead of doing a
search per-port-destruction?
The change to ofproto_receive() dropped the comment
/* Let the caller know that we can't reproduce 'key' from 'flow'. */
if (fitness == ODP_FIT_PERFECT) {
fitness = ODP_FIT_TOO_MUCH;
}
but in fact it answered a question I asked myself when I reviewed the
new code.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
