[ovs-dev] [PATCH ovn 0/4] Reduce number of flows in IN_IP_INPUT table for DNAT.

Numan Siddique numans at ovn.org
Fri Jun 26 13:51:38 UTC 2020


On Fri, Jun 26, 2020 at 7:02 PM Dumitru Ceara <dceara at redhat.com> wrote:

> On 6/26/20 3:05 PM, Numan Siddique wrote:
> >
> >
> > On Wed, Jun 24, 2020 at 9:21 PM Dumitru Ceara <dceara at redhat.com
> > <mailto:dceara at redhat.com>> wrote:
> >
> >     The first three patches refactor the ARP/NS responder code for
> logical
> >     routers in order to make it easier for patch #4 to configure the
> flows
> >     with different priorities depending on logical port type.
> >
> >     Suggested-by: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>>
> >     Reported-by: Girish Moodalbail <gmoodalbail at gmail.com
> >     <mailto:gmoodalbail at gmail.com>>
> >     Reported-at:
> >
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-June/050186.html
> >     Signed-off-by: Dumitru Ceara <dceara at redhat.com
> >     <mailto:dceara at redhat.com>>
> >
> >     Dumitru Ceara (4):
> >           ovn-northd: Store ETH address of router inport in xxreg0.
> >           ovn-northd: Refactor ARP/NS responder in router pipeline.
> >           ovn-northd: Refactor NAT address parsing.
> >           ovn-northd: Minimize number of ARP/NS responder flows for DNAT.
> >
> >
> > Hi Dumitru,
> >
> > Thanks for this patch series.
> >
> > I didn't review them thoroughly. I've few comments.
> >
>
> Hi Numan,
>
> Thanks for looking at these patches.
>
> > 1. Looks like reg0 and xxreg0 are already used in the router pipeline. I
> > think it's better to use a different register (xxreg1 may be).
>
> Yes, xxreg0 is already used in the router pipeline, but only in later
> stages. However, I'll use xxreg1 instead just to make sure there's no
> confusion.
>

Thanks.


>
> >    I'd suggest adding a new macro REG_IP_ROUTING (or something more
> > meaningful) instead of using reg0/xxreg0 in the existing
> >    code in the lr_in_ip_input/lr_in_arp_resolve stages.
> >
>
> I'm not really sure I follow. I added a macro:
>
> #define REG_INPORT_ETH_ADDR "xxreg0[80..127]"
>
> And there's no bare reference to xxreg0 in my patches, I always used the
> macro. Did I miss anything?
>

Sorry for not being very clear.
I mean the existing logical flows (not related to your patch).
One eg -
https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L9749

I know it's not exactly related to your patch set, but if you're fine
adding a new macro
for existing xxreg0 and using the macro instead that would be great :)

Feel free to ignore this if it's too much work.


>
> $ git diff origin/master northd/ovn-northd.c | grep xxreg
> +#define REG_INPORT_ETH_ADDR "xxreg0[80..127]"
> $
>
> > 2. Does patch 2 require updating the ovn-northd.8.xml ?
> >
>
> It shouldn't have to because it's just a refactoring that doesn't change
> the way flows are generated.
>
> > 3. For patches which modify the logical flows, can you please add a few
> > test cases in ovn-northd.at <http://ovn-northd.at>. That would also help
> > me while
> >    reviewing the code :)
> >
>
> Sure, will do in v2.
>
>
Thanks
Numan


> Thanks,
> Dumitru
>
> > Thanks
> > Numan
> >
> >
> >
> >      northd/ovn-northd.8.xml |   38 ++-
> >      northd/ovn-northd.c     |  565
> >     ++++++++++++++++++++++++++++-------------------
> >      tests/ovn.at <http://ovn.at>            |    8 -
> >      3 files changed, 361 insertions(+), 250 deletions(-)
> >
> >
> >     _______________________________________________
> >     dev mailing list
> >     dev at openvswitch.org <mailto: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