On Wed, Apr 14, 2010 at 6:39 PM, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>&gt;</span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div>On Tue, Apr 13, 2010 at 10:41:07AM -0400, Jesse Gross wrote:<br>
&gt; Currently the datapath directly accesses devices through their<br>
&gt; Linux functions.  Obviously this doesn&#39;t work for virtual devices<br>
&gt; that are not backed by an actual Linux device.  This creates a<br>
&gt; new virtual port layer which handles all interaction with devices.<br>
&gt; The existing support for Linux devices was then implemented on top<br>
&gt; of this layer as two device types.  It splits out and renames dp_dev<br>
&gt; to internal_dev.  There were several places where datapath devices<br>
&gt; had to handled in a special manner and this cleans that up by putting<br>
&gt; all the special casing in a single location.<br>
</div>Wow, this is major infrastructure.<br>
I see a few lines that are indented with four spaces instead of one tab.<br></blockquote><div><br></div><div>I think I got all of them now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

I think that the relationship between vport and net_bridge_port is this:<br>
a vport may be attached to 0 or 1 net_bridge_ports; a net_bridge_port is<br>
always attached to exactly one vport.  Is that right?  If it is, I would<br>
be tempted to embed the net_bridge_port in the vport structure or to<br>
otherwise combine them.  Is that feasible/sensible?<br></blockquote><div><br></div><div>Yes, that&#39;s the correct relationship between them.</div><div><br></div><div>First of all, I realized that net_bridge_port is overloaded into two now unrelated things: the first is representing a port on a datapath and contains datapath specific state: a pointer to the datapath, port number, etc.  The second is to use to set dev-&gt;br_port so that packets will be received for Linux system devices.  Now that all the bridge hook stuff is in vport-netdev.c there is no reason that these should be the same structure.  I separated them so there is now a struct dp_port which contains all of the thing net_bridge_port previously contained and dev-&gt;br_port now points directly to the vport.  Now vport implementors don&#39;t have to care about dp_port at all, which is nice.</div>

<div><br></div><div>Back to the point of embedding net_bridge_port (now dp_port) into a vport: I don&#39;t think that it really makes sense.  As a practical matter, dp_port can change during the lifetime of a vport (during attach/detach).  This means that we need to do RCU, which means that we need another structure, which puts us back where we are now.  However, I also don&#39;t think that they belong together - struct dp_port is managed by the datapath (and protected by its locks) and struct vport is managed by vport (and protected by its locks).  It seems much easier to reason about if they aren&#39;t tightly bound.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Kernel abstraction layers are usually very thin and transparent.  This<br>
one is a little &quot;thicker&quot;.  vport_init() seems more elaborate than usual<br>
for kernel code.  I don&#39;t usually see a separation between client and<br>
implementor headers (vport.h, vport-provider.h).  Trivial accessor<br>
functions are usually omitted in favor of using members directly<br>
(e.g. vport_get_class(), vport_get_nbp()).<br></blockquote><div><br></div><div>The layer started out much thinner.  Part of the problem with having a thin layer is that the thickness is not consistent between functions and tends to grow over time. It&#39;s much nicer to work with a consistent abstraction and much easier to keep it that way as new features are added if the layer already inserts itself consistently between the participants.</div>

<div><br></div><div>vport_init() is a little more complicated than it currently needs to be but I&#39;m planning on supporting dynamically loading vport providers at which point it will need that extra complexity.  I think it&#39;s nicer if the client and implementor headers are separate but you are right they are usually together, so I merged those two files.  Using vport_get_class() is probably being a little overzealous, so I removed it and vport_get_nbp() is now no longer just a pass through.  However, there are still some other functions which directly call through that I left.  Having these give us more flexibility in the future to make changes without having to update lots of other code paths.  In general the layers in Linux are too thin - it&#39;s a large part of the reason why we need all the compatibility code.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The vport_class flags bother me a little.</blockquote><div><br></div><div>Is it the existence of the flags or the flags that are currently defined?  If it&#39;s the existence, it&#39;s not too different from dev-&gt;features.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  VPORT_F_REQUIRED seems to be<br>
a very special case, only there for mutual exclusion with the bridge<br>
module.</blockquote><div><br></div><div>Bridge exclusion is a special case and assuming that we want to keep the current behavior this seems the cleanest way to expose it.  Actually the only truly required type is the internal port but it has no init function so it can&#39;t fail.  However, you could reverse the importance of the netdev and GRE providers.  In this case you would only load if there was no GRE handler registered but could have a datapath with only GRE devices that would work even if the Linux bridge is in use.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  My &quot;feeling&quot; is that something analogous to VPORT_F_GEN_STATS<br>
would usually be implemented in the kernel as helper functions or<br>
&quot;generic&quot; functions, e.g. like how many Linux file systems define their<br>
aio functions:<br>
        .aio_read       = generic_file_aio_read,<br>
        .aio_write      = generic_file_aio_write,<br></blockquote><div><br></div><div>I don&#39;t think that these are quite analogous because aio can be implemented on top of existing synchronous operations while stats requires interposing on the send and receive operations.  Of course you could check whether the get_stats function points to the generic version but that seems kind of hacky.  In fact:</div>

<div><br></div><div><pre>const struct net_device_stats *dev_get_stats(struct net_device *dev)
        const struct net_device_ops *ops = dev-&gt;netdev_ops;

        if (ops-&gt;ndo_get_stats)
                return ops-&gt;ndo_get_stats(dev);

        dev_txq_stats_fold(dev, &amp;dev-&gt;stats);
        return &amp;dev-&gt;stats;
</pre><pre><font face="arial"><span style="white-space:normal">It looks fairly similar to mine though I think mine is a little more flexible.</span></font></pre>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
VPORT_F_TUN_ID seems like maybe it should be replaced by a comment above<br>
vport_receive() that says &quot;Caller must initialize OVS_CB(skb)-&gt;tun_id.&quot;<br>
Or perhaps vport_receive() could take a tun_id argument and initialize<br>
OVS_CB(skb)-&gt;tun_id from that itself, which would really be foolproof.<br>
<br></blockquote><div><br></div><div>I think expecting the caller to set tun_id is asking for trouble.  Really setting a flag if you support a feature and allow the generic layer to handle it seems a lot more convenient to me and similar to how most device features are handled (GSO, checksum offloading, etc).</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Usually kernel &quot;class&quot; structures are named something_ops instead of<br>
something_class.<br></blockquote><div><br></div><div>Yes, that&#39;s a bit nicer.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I see some uses of &quot;sizeof&quot; on objects instead of on types, and uses<br>
without parentheses.  Kernel style discourages both.<br></blockquote><div><br></div><div>This just seems like a step backwards but that is the more common style, so fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

In vport_locate(), WARN_ON_ONCE would avoid spewing lots of errors if<br>
something did go wrong.<br></blockquote><div><br></div><div>vport_locate() isn&#39;t called that frequently (not for every packet) plus it is nice to get stack traces of different paths that might cause the problem.  It&#39;s also a significant bug if it triggers (ASSERT_RTNL has the same behavior).</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Kernel style would call for __vport_add() to use ERR_PTR to report both<br>
successful and error return values.  Similarly for the &quot;create&quot; member<br>
function of vport_class.<br></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Could vport_record_error() be made simpler, getting rid of VPORT_E_*<br>
entirely, by passing in a pointer to the counter to increment?  (It&#39;s<br>
hard to tell, because this commit does not add any callers.)  If<br>
rx_errors and tx_errors are considered as the sum of their own values<br>
plus the other errors that contribute to them, then there would only be<br>
one value to increment each time.<br></blockquote><div><br></div><div>I now sum the error values when stats are requested instead of when the errors occur.  However, while it is possible to directly pass in a counter, this is the same issue with thiner vs. thicker layers.  I already changed the layout of the stats once and this design meant that I didn&#39;t have to changed anything else.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I kept thinking that some functions don&#39;t have comments but need them,<br>
but them I realized that the comments were above the prototypes in the<br>
header files.  I don&#39;t think that the kernel uses this convention; I<br>
don&#39;t remember seeing them there before.<br></blockquote><div><br></div><div>OK, I moved the comments above the implementation and documented all the public interfaces.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

It&#39;s probably not an appropriate change for this patch, but forcing the<br>
internal device MTU to the minimum MTU of the other ports has always<br>
bothered me.  Only userspace, or possibly even the OpenFlow controller,<br>
can know whether a packet received on a given port might ever be sent<br>
out another given port.  For example, a single datapath could be broken<br>
up into two entirely independent bridges.  So it seems to me that<br>
userspace should be allowed to set appropriate MTUs on internal<br>
devices.  (dp_min_mtu() seems like a reasonable default though.)<br></blockquote><div><br></div><div>It really depends on whether you see a datapath as a single bridge or not.  There also isn&#39;t currently anyway for userspace to set the MTU (it&#39;s not exposed in the netdev library).  In any case, it is definitely orthogonal to this patch.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The following:<br>
<div>  struct sk_buff *dummy_skb;<br>
  int err;<br>
</div><div>  BUILD_BUG_ON(sizeof(struct ovs_skb_cb) &gt; sizeof(dummy_skb-&gt;cb));<br>
</div>may be written as:<br>
  BUILD_BUG_ON(sizeof(struct ovs_skb_cb) &gt; sizeof(((struct sk_buff *)0)-&gt;cb));<br>
Which you like better is up to you :-)<br></blockquote><div><br></div><div>Hmm, they are both pretty ugly but I think I&#39;ll stick with my version - it&#39;s also the way it is done in other places.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

There are no dpif functions to call any of the new ODP_VPORT_* ioctls.<br>
I guess those will be added later.<br></blockquote><div><br></div><div>Yes, they&#39;re coming... </div></div><br>