On Wed, Aug 18, 2010 at 12:49 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 Wed, Aug 18, 2010 at 12:08:49AM -0700, Jesse Gross wrote:<br>
&gt; Up until now it was assumed that encapsulated packets larger than<br>
&gt; the MTU would be fragmented by the IP stack.  However, some<br>
&gt; tunneling protocols provide their own fragmentation mechanism.  This<br>
&gt; adds the necessary support to the generic tunnel code to support<br>
&gt; fragmentation.<br>
&gt;<br>
&gt; Signed-off-by: Jesse Gross &lt;<a href="mailto:jesse@nicira.com" target="_blank">jesse@nicira.com</a>&gt;<br>
<br>
</div>It seems totally bizarre to me that &quot;local_df&quot; is set to 1 to *allow*<br>
fragmentation.  I never would have guessed without the comment that you<br>
added (which is not new in this patch).<br></blockquote><div><br></div><div>I agree, it&#39;s totally counterintuitive to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<br>
The build_header member in struct tnl_ops could use a comment.<br></blockquote><div><br></div><div>Sure.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<br>
get_random_bytes() is pretty expensive.  Most of the network stack uses<br>
net_random() or random32() (same thing), can you use that instead?<br></blockquote><div><br></div><div>I&#39;m not sure that the cost is really all that big a deal here since it only happens when we setup tunnels (we also use get_random_bytes() for generating a MAC address at this time).  Getting initial IP IDs, TCP ISNs, etc. all come from a more secure random pool (and as Steve Hemminger mentioned random32() is really for statistical simulation).  I would have used get_random_int() as the best solution but it isn&#39;t exported.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
It&#39;s hard to evaluate the use of the new &#39;frag_id&#39; member of struct<br>
tnl_vport, since it&#39;s treated as write-only in this commit.  atomic_t&#39;s<br>
are relatively expensive on SMP, but maybe fragmentation is inherently<br>
expensive enough that it doesn&#39;t matter?<br>
</blockquote></div><br><div>Fragmentation is already a very slow path, so if you go down it you&#39;ve already lost.  I thought about doing something better but it would blow up data structures, which has a cost for everybody.  It&#39;s just not worth it.  IP IDs also use atomic_t&#39;s, so we&#39;re in good company.</div>