[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:53:04 UTC 2020


On 6/26/20 3:51 PM, Numan Siddique wrote:
> 
> 
> On Fri, Jun 26, 2020 at 7:02 PM Dumitru Ceara <dceara at redhat.com
> <mailto: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>
>     > <mailto: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>
>     <mailto:hzhou at ovn.org <mailto:hzhou at ovn.org>>>
>     >     Reported-by: Girish Moodalbail <gmoodalbail at gmail.com
>     <mailto:gmoodalbail at gmail.com>
>     >     <mailto: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>
>     >     <mailto: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.
>  

Ah I had misunderstood, sorry. No problem, I'll add another patch to the
series to refactor this part too.

Thanks,
Dumitru

> 
> 
>     $ 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>
>     <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> <http://ovn.at>            |    8 -
>     >      3 files changed, 361 insertions(+), 250 deletions(-)
>     >
>     >
>     >     _______________________________________________
>     >     dev mailing list
>     >     dev at openvswitch.org <mailto:dev at openvswitch.org>
>     <mailto:dev at openvswitch.org <mailto:dev at openvswitch.org>>
>     >     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     >
> 
>     _______________________________________________
>     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