[ovs-dev] [PATCH v7 ovn 3/3] northd: add check_pkt_larger lflows for ingress traffic

Han Zhou hzhou at ovn.org
Thu Aug 5 16:17:03 UTC 2021


On Wed, Aug 4, 2021 at 1:30 AM Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
wrote:
>
> [...]
> >
> > I think REGBIT_EGRESS_LOOPBACK check is required so that the injected
icmp4
> > packet generated from ovn-controller is skipped in this stage.
> > Otherwise there will
> > be recursion of the packet.
> >
> > @Lorenzo Bianconi  Please correct me if I'm wrong.
>
> correct, REGBIT_EGRESS_LOOPBACK is used to skip re-injected packets
>
> >
> >
> > > >
> > > > (It would be good if this is intentional because it would make my
next
> > > rebasing easier, but I just want to make sure I understand it
correctly)
> > > >
> > >
> > > I checked further on this change, and it seems the outport is not
needed
> > > for this flow because in the previous stage (
s_ROUTER_IN_CHK_PKT_LEN) it
> > > is already matched. (although I am still not sure why adding the
LOOPBACK
> > > check)
> > > However, for the ingress part, check_pkt_larger() is added to the
stage
> > > s_ROUTER_IN_ADMISSION regardless of whether it is a DGP, but based on
> > > whether the gw_mtu option exists. I think that is reasonable. It
didn't
> > > need to distinguish DGP and ports on gateway routers. So I wonder if
we
> > > could do the same for the egress, i.e. combining the logic of DGP and
ports
> > > on gateway routers. This would simply the code and also the flows. I
can
> > > make that change if it sounds good. (It can also simplify my rebasing
for
> > > the multiple DGP support)
> >
> > I'm fine with it if you think it will not break the existing
functionality.
> >
> > @Lorenzo Bianconi  If you've any comments here please let us know.
>
> I am fine with the change if existing functionality is ok.
> Btw I am not able to trigger any failure in "router - check packet length
- icmp
> defrag" in unit-test (I tried multiple times running all the tests and
> each test individually).
>

Thanks Numan and Lorenzo. I figured out that it is in fact incorrect
without the outport matched. Please see the details in this patch I posted:
https://patchwork.ozlabs.org/project/ovn/patch/20210805153934.3865265-2-hzhou@ovn.org/

The above mentioned refactoring is also included in this patch. Please take
a look.

For the test case failure on my machine, it happens at the step below which
was added by this patch, and that's why it passed when I reverted the patch.

AS_BOX([testing ingress traffic mtu 100 - IPv6])
test_ip6_packet_larger_ext 100

But I didn't have the time to dig further yet.

Thanks,
Han

> >
> > >
> > > > > Regarding the test case,  it seems to me it's flaky.  When I run
the
> > > > > whole test suite with -j5, almost 100% of the
> > > > > time the test fails, but it passes when run individually for me.
> > > > >
> > > > Thanks for confirming. This is interesting, completely reverse
behavior
> > > on my machine :)
> > > >
> > > > > The test case added in this patch series, does try to make sure
that
> > > > > both northd-c and northd-ddlog generate
> > > > > the exact same flows.
> > > > >
> > > > > Thanks
> > > > > Numan
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Numan
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Han
> > > > > > > _______________________________________________
> > > > > > > dev mailing list
> > > > > > > dev at openvswitch.org
> > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >


More information about the dev mailing list