<div dir="ltr">Hi guys,<div><br></div><div>Great job Numan!</div><div>As we discussed over IRC, the patch below may make more sense.</div><div>It essentially sets the dl_type so that when packet comes from the controller, it matches</div><div>a valid type and <span style="font-size:12.8px">OVS_KEY_ATTR_CT_ORIG_TUPLE_</span><wbr style="font-size:12.8px"><span style="font-size:12.8px">IPV4 is not added.</span></div><div>Maybe what Numan proposed and this patch could be a good combination but I think</div><div>that we definitely need to set the dl_type as it's later checked in <span style="font-size:12.8px">pkt_metadata_from_flow()</span></div><div><span style="font-size:12.8px">and it'll be zero there otherwise.</span></div><div><span style="font-size:12.8px">What do you guys think? I have tried the patch below and the kernel error is not shown</span></div><div><span style="font-size:12.8px">anymore when issuing DHCP requests.</span></div><div><br></div><div><br></div><div><div>diff --git a/lib/flow.c b/lib/flow.c</div><div>index b2b10aa..62b948f 100644</div><div>--- a/lib/flow.c</div><div>+++ b/lib/flow.c</div><div>@@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow, struct match *flow_metadata)</div><div> </div><div> if (flow->ct_state != 0) {</div><div> match_set_ct_state(flow_metadata, flow->ct_state);</div><div>+ match_set_dl_type(flow_metadata, flow->dl_type);</div><div> if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto != 0) {</div><div> if (flow->dl_type == htons(ETH_TYPE_IP)) {</div><div> match_set_ct_nw_src(flow_metadata, flow->ct_nw_src);</div></div><div><br></div><div><br></div><div>Thanks,</div><div>Daniel</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 24, 2017 at 10:38 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@ovn.org" target="_blank">blp@ovn.org</a>></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 Tue, Oct 24, 2017 at 09:04:22PM +0530, Numan Siddique wrote:<br>
> We did some more investigation. This issue is seen only when OVN native<br>
> dhcp is used and with kernel datapath which doesn't support<br>
> OVS_KEY_ATTR_CT_ORIG_TUPLE_<wbr>IPV4. The reason for this failure is because<br>
> ovs-vswitchd includes the attribute OVS_KEY_ATTR_CT_ORIG_TUPLE_<wbr>IPV4 when it<br>
> sends the packet back to the datapath after the dhcp reply packet is<br>
> resumed.<br>
><br>
> When the dhcp packet is sent to ovn-controller, the ct_state value is set<br>
> to 0x21 and dl_type is set to 0 in the flow metadata. When the packet is<br>
> resumed, the function nxt_resume() calls 'pkt_metadata_from_flow()' which<br>
> neither sets 'md->ct_orig_tuple' or memsets it [1] because is_ct_valid()<br>
> returns true and dl_type is 0. And the function odp_key_from_dp_packet()<br>
> adds OVS_KEY_ATTR_CT_ORIG_TUPLE_<wbr>IPV4 [2]<br>
><br>
> This issue is not seen in master because of this commit - "f6fabcc624<br>
> ofproto-dpif: Mark packets as "untracked" after call to ct()" [3]<br>
><br>
> This patch clears the conn track variables after the ct() action.<br>
><br>
> I suppose we cannot apply this patch to OVS 2.8 branch because it was<br>
> reverted [4] due to some issues.<br>
><br>
> I think we can solve this problem either with the below fixe or by setting<br>
> dl_type to proper value when the packet is sent to controller.<br>
><br>
> ******************************<wbr>*****<br>
> diff --git a/lib/flow.h b/lib/flow.h<br>
> index 6ae5a674d..076ce36f1 100644<br>
> --- a/lib/flow.h<br>
> +++ b/lib/flow.h<br>
> @@ -947,6 +947,8 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const<br>
> struct flow *flow)<br>
> flow->ct_tp_dst,<br>
> flow->ct_nw_proto,<br>
> };<br>
> + } else {<br>
> + memset(&md->ct_orig_tuple, 0, sizeof md->ct_orig_tuple);<br>
> }<br>
> } else {<br>
> memset(&md->ct_orig_tuple, 0, sizeof md->ct_orig_tuple);<br>
> ******************************<wbr>****<br>
><br>
> Please let me know if this fix makes sense ? Or if there is a better way to<br>
> solve it ?<br>
<br>
</div></div>I think that is a reasonable patch. Will you please propose it as a<br>
formal patch? Please include a thorough commit message.<br>
</blockquote></div><br></div>