On 6/26/20 3:53 PM, Dumitru Ceara wrote: > On 6/26/20 3:51 PM, Numan Siddique wrote: >> >> >> On Fri, Jun 26, 2020 at 7:02 PM Dumitru Ceara <dce...@redhat.com >> <mailto:dce...@redhat.com>> wrote: >> >> On 6/26/20 3:05 PM, Numan Siddique wrote: >> > >> > >> > On Wed, Jun 24, 2020 at 9:21 PM Dumitru Ceara <dce...@redhat.com >> <mailto:dce...@redhat.com> >> > <mailto:dce...@redhat.com <mailto:dce...@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 <hz...@ovn.org <mailto:hz...@ovn.org> >> <mailto:hz...@ovn.org <mailto:hz...@ovn.org>>> >> > Reported-by: Girish Moodalbail <gmoodalb...@gmail.com >> <mailto:gmoodalb...@gmail.com> >> > <mailto:gmoodalb...@gmail.com <mailto:gmoodalb...@gmail.com>>> >> > Reported-at: >> > >> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-June/050186.html >> > Signed-off-by: Dumitru Ceara <dce...@redhat.com >> <mailto:dce...@redhat.com> >> > <mailto:dce...@redhat.com <mailto:dce...@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 >>
Hi Numan, I sent a v2 of the series including your suggestions: https://patchwork.ozlabs.org/project/openvswitch/list/?series=186776 Thanks, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev