<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 25, 2017 at 10:51 PM, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@ovn.org" target="_blank">blp@ovn.org</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Oct 25, 2017 at 06:50:29PM +0530, Numan Siddique wrote:<br>
&gt; On Wed, Oct 25, 2017 at 3:09 PM, Daniel Alvarez Sanchez &lt;<a href="mailto:dalvarez@redhat.com">dalvarez@redhat.com</a><br>
&gt; &gt; wrote:<br>
&gt;<br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt; On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff &lt;<a href="mailto:blp@ovn.org">blp@ovn.org</a>&gt; wrote:<br>
&gt; &gt;<br>
&gt; &gt;&gt; On Tue, Oct 24, 2017 at 02:27:59PM -0700, Ben Pfaff wrote:<br>
&gt; &gt;&gt; &gt; On Tue, Oct 24, 2017 at 11:07:58PM +0200, Daniel Alvarez Sanchez wrote:<br>
&gt; &gt;&gt; &gt; &gt; Hi guys,<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt; Great job Numan!<br>
&gt; &gt;&gt; &gt; &gt; As we discussed over IRC, the patch below may make more sense.<br>
&gt; &gt;&gt; &gt; &gt; It essentially sets the dl_type so that when packet comes from the<br>
&gt; &gt;&gt; &gt; &gt; controller, it matches<br>
&gt; &gt;&gt; &gt; &gt; a valid type and OVS_KEY_ATTR_CT_ORIG_TUPLE_<wbr>IPV4 is not added.<br>
&gt; &gt;&gt; &gt; &gt; Maybe what Numan proposed and this patch could be a good combination<br>
&gt; &gt;&gt; but I<br>
&gt; &gt;&gt; &gt; &gt; think<br>
&gt; &gt;&gt; &gt; &gt; that we definitely need to set the dl_type as it&#39;s later checked in<br>
&gt; &gt;&gt; &gt; &gt; pkt_metadata_from_flow()<br>
&gt; &gt;&gt; &gt; &gt; and it&#39;ll be zero there otherwise.<br>
&gt; &gt;&gt; &gt; &gt; What do you guys think? I have tried the patch below and the kernel<br>
&gt; &gt;&gt; error<br>
&gt; &gt;&gt; &gt; &gt; is not shown<br>
&gt; &gt;&gt; &gt; &gt; anymore when issuing DHCP requests.<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt; diff --git a/lib/flow.c b/lib/flow.c<br>
&gt; &gt;&gt; &gt; &gt; index b2b10aa..62b948f 100644<br>
&gt; &gt;&gt; &gt; &gt; --- a/lib/flow.c<br>
&gt; &gt;&gt; &gt; &gt; +++ b/lib/flow.c<br>
&gt; &gt;&gt; &gt; &gt; @@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow,<br>
&gt; &gt;&gt; struct<br>
&gt; &gt;&gt; &gt; &gt; match *flow_metadata)<br>
&gt; &gt;&gt; &gt; &gt;<br>
&gt; &gt;&gt; &gt; &gt;      if (flow-&gt;ct_state != 0) {<br>
&gt; &gt;&gt; &gt; &gt;          match_set_ct_state(flow_<wbr>metadata, flow-&gt;ct_state);<br>
&gt; &gt;&gt; &gt; &gt; +        match_set_dl_type(flow_<wbr>metadata, flow-&gt;dl_type);<br>
&gt; &gt;&gt; &gt; &gt;          if (is_ct_valid(flow, NULL, NULL) &amp;&amp; flow-&gt;ct_nw_proto != 0)<br>
&gt; &gt;&gt; {<br>
&gt; &gt;&gt; &gt; &gt;              if (flow-&gt;dl_type == htons(ETH_TYPE_IP)) {<br>
&gt; &gt;&gt; &gt; &gt;                  match_set_ct_nw_src(flow_<wbr>metadata, flow-&gt;ct_nw_src);<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt; &gt; This patch seems reasonable too.<br>
&gt; &gt;&gt; &gt;<br>
&gt; &gt;&gt; &gt; I recommend adding a comment above it to explain what&#39;s going on,<br>
&gt; &gt;&gt; &gt; because dl_type is not a metadata field and it&#39;s confusing to deal with<br>
&gt; &gt;&gt; &gt; it in a context that&#39;s supposed to be all about metadata.  I guess the<br>
&gt; &gt;&gt; &gt; comment would essentially say that dl_type is essential to the<br>
&gt; &gt;&gt; &gt; interpretation of the conntrack metadata.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; Oh, and for this patch too I&#39;d welcome a formal patch proposal.<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
&gt; &gt; Done. Thanks a lot Ben.<br>
&gt; &gt; If this get merged, it would be great if we can get it into 2.8 branch and<br>
&gt; &gt; add a new tag (2.8.2).<br>
&gt; &gt;<br>
&gt; &gt; Thanks!!<br>
&gt; &gt;<br>
&gt;<br>
&gt; Ben, we have submitted both the patches. The patch from Daniel - (<br>
&gt; <a href="https://patchwork.ozlabs.org/patch/830160/" rel="noreferrer" target="_blank">https://patchwork.ozlabs.org/<wbr>patch/830160/</a>)  will fix the issue.<br>
&gt; Not sure if this patch  <a href="https://patchwork.ozlabs.org/patch/830132/" rel="noreferrer" target="_blank">https://patchwork.ozlabs.org/<wbr>patch/830132/</a> is<br>
&gt; needed any more.<br>
&gt;<br>
&gt; Request to review these patches if possible as RDO is blocked on these<br>
&gt; patches before we can  update from OVS 2.7.2 to OVS 2.8(.2)<br>
<br>
</div></div>I&#39;ve reviewed both.  I wasn&#39;t able to immediately apply either one, but<br>
they&#39;re obviously moving in the right direction, so I&#39;d appreciate<br>
follow-up from both of you so that we can get them in.<br>
</blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for the review. </div><div class="gmail_extra"><br></div><div class="gmail_extra">Numan</div><div class="gmail_extra"><br></div></div>