[ovs-dev] [PATCH 05/16] datapath: Add generic virtual port layer.

Ben Pfaff blp at nicira.com
Wed Apr 14 22:39:28 UTC 2010


On Tue, Apr 13, 2010 at 10:41:07AM -0400, Jesse Gross wrote:
> Currently the datapath directly accesses devices through their
> Linux functions.  Obviously this doesn't work for virtual devices
> that are not backed by an actual Linux device.  This creates a
> new virtual port layer which handles all interaction with devices.
> 
> The existing support for Linux devices was then implemented on top
> of this layer as two device types.  It splits out and renames dp_dev
> to internal_dev.  There were several places where datapath devices
> had to handled in a special manner and this cleans that up by putting
> all the special casing in a single location.

Wow, this is major infrastructure.

I see a few lines that are indented with four spaces instead of one tab.

I think that the relationship between vport and net_bridge_port is this:
a vport may be attached to 0 or 1 net_bridge_ports; a net_bridge_port is
always attached to exactly one vport.  Is that right?  If it is, I would
be tempted to embed the net_bridge_port in the vport structure or to
otherwise combine them.  Is that feasible/sensible?

Kernel abstraction layers are usually very thin and transparent.  This
one is a little "thicker".  vport_init() seems more elaborate than usual
for kernel code.  I don't usually see a separation between client and
implementor headers (vport.h, vport-provider.h).  Trivial accessor
functions are usually omitted in favor of using members directly
(e.g. vport_get_class(), vport_get_nbp()).

The vport_class flags bother me a little.  VPORT_F_REQUIRED seems to be
a very special case, only there for mutual exclusion with the bridge
module.  My "feeling" is that something analogous to VPORT_F_GEN_STATS
would usually be implemented in the kernel as helper functions or
"generic" functions, e.g. like how many Linux file systems define their
aio functions:
	.aio_read	= generic_file_aio_read,
	.aio_write	= generic_file_aio_write,
VPORT_F_TUN_ID seems like maybe it should be replaced by a comment above
vport_receive() that says "Caller must initialize OVS_CB(skb)->tun_id."
Or perhaps vport_receive() could take a tun_id argument and initialize
OVS_CB(skb)->tun_id from that itself, which would really be foolproof.

Usually kernel "class" structures are named something_ops instead of
something_class.

I see some uses of "sizeof" on objects instead of on types, and uses
without parentheses.  Kernel style discourages both.

In vport_locate(), WARN_ON_ONCE would avoid spewing lots of errors if
something did go wrong.

Kernel style would call for __vport_add() to use ERR_PTR to report both
successful and error return values.  Similarly for the "create" member
function of vport_class.

Could vport_record_error() be made simpler, getting rid of VPORT_E_*
entirely, by passing in a pointer to the counter to increment?  (It's
hard to tell, because this commit does not add any callers.)  If
rx_errors and tx_errors are considered as the sum of their own values
plus the other errors that contribute to them, then there would only be
one value to increment each time.

I kept thinking that some functions don't have comments but need them,
but them I realized that the comments were above the prototypes in the
header files.  I don't think that the kernel uses this convention; I
don't remember seeing them there before.

It's probably not an appropriate change for this patch, but forcing the
internal device MTU to the minimum MTU of the other ports has always
bothered me.  Only userspace, or possibly even the OpenFlow controller,
can know whether a packet received on a given port might ever be sent
out another given port.  For example, a single datapath could be broken
up into two entirely independent bridges.  So it seems to me that
userspace should be allowed to set appropriate MTUs on internal
devices.  (dp_min_mtu() seems like a reasonable default though.)

The following:
  struct sk_buff *dummy_skb;
  int err;

  BUILD_BUG_ON(sizeof(struct ovs_skb_cb) > sizeof(dummy_skb->cb));
may be written as:
  BUILD_BUG_ON(sizeof(struct ovs_skb_cb) > sizeof(((struct sk_buff *)0)->cb));
Which you like better is up to you :-)

There are no dpif functions to call any of the new ODP_VPORT_* ioctls.
I guess those will be added later.




More information about the dev mailing list