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

Dumitru Ceara dceara at redhat.com
Fri Jun 26 13:31:41 UTC 2020


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.

>    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?

$ 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,
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
> 



More information about the dev mailing list