[ovs-dev] [PATCH ovn 0/4] Reduce number of flows in IN_IP_INPUT table for DNAT.
Dumitru Ceara
dceara at redhat.com
Tue Jun 30 21:48:16 UTC 2020
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 <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
>>
Hi Numan,
I sent a v2 of the series including your suggestions:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=186776
Thanks,
Dumitru
More information about the dev
mailing list