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