On 6/26/20 3:51 PM, Numan Siddique wrote: > > > On Fri, Jun 26, 2020 at 7:02 PM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: > > On 6/26/20 3:05 PM, Numan Siddique wrote: > > > > > > On Wed, Jun 24, 2020 at 9:21 PM Dumitru Ceara <[email protected] > <mailto:[email protected]> > > <mailto:[email protected] <mailto:[email protected]>>> 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 <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> > > Reported-by: Girish Moodalbail <[email protected] > <mailto:[email protected]> > > <mailto:[email protected] <mailto:[email protected]>>> > > Reported-at: > > > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-June/050186.html > > Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]> > > <mailto:[email protected] <mailto:[email protected]>>> > > > > 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 > > [email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
