On 9/8/20 8:51 AM, Anton Ivanov wrote: > > > On 07/09/2020 18:39, Ilya Maximets wrote: >> On 9/7/20 7:08 PM, Anton Ivanov wrote: >>> >>> >>> On 07/09/2020 12:51, Ilya Maximets wrote: >>>> On 9/4/20 8:22 PM, Anton Ivanov wrote: >>>>> >>>>> On 04/09/2020 15:00, Ilya Maximets wrote: >>>>>> On 9/2/20 4:59 PM, [email protected] wrote: >>>>>>> From: Anton Ivanov <[email protected]> >>>>>>> >>>>>>> Signed-off-by: Anton Ivanov <[email protected]> >>>>>>> --- >>>>>>> northd/ovn-northd.c | 73 >>>>>>> +++++++++++++++++++++++++-------------------- >>>>>>> 1 file changed, 40 insertions(+), 33 deletions(-) >>>>>>> >>>>>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>>>>>> index f2e3104ba..cb77296c4 100644 >>>>>>> --- a/northd/ovn-northd.c >>>>>>> +++ b/northd/ovn-northd.c >>>>>>> @@ -8416,57 +8416,40 @@ build_lrouter_force_snat_flows(struct hmap >>>>>>> *lflows, struct ovn_datapath *od, >>>>>>> } >>>>>>> static void >>>>>>> -build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >>>>>>> - struct hmap *lflows, struct shash *meter_groups, >>>>>>> - struct hmap *lbs) >>>>>>> +build_lrouter_flows_ingress_table_0_od( >>>>>>> + struct ovn_datapath *od, struct hmap *lflows) >>>>>>> { >>>>>>> - /* This flow table structure is documented in ovn-northd(8), so >>>>>>> please >>>>>>> - * update ovn-northd.8.xml if you change anything. */ >>>>>>> - >>>>>>> - struct ds match = DS_EMPTY_INITIALIZER; >>>>>>> - struct ds actions = DS_EMPTY_INITIALIZER; >>>>>>> - >>>>>>> /* Logical router ingress table 0: Admission control framework. >>>>>>> */ >>>>>>> - struct ovn_datapath *od; >>>>>>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>>>>>> - if (!od->nbr) { >>>>>>> - continue; >>>>>>> - } >>>>>>> - >>>>>>> + if (od->nbr) { >>>>>>> /* Logical VLANs not supported. >>>>>>> * Broadcast/multicast source address is invalid. */ >>>>>>> ovn_lflow_add(lflows, od, S_ROUTER_IN_ADMISSION, 100, >>>>>>> "vlan.present || eth.src[40]", "drop;"); >>>>>>> } >>>>>>> +} >>>>>>> - /* Logical router ingress table 0: match (priority 50). */ >>>>>>> - struct ovn_port *op; >>>>>>> - HMAP_FOR_EACH (op, key_node, ports) { >>>>>>> - if (!op->nbrp) { >>>>>>> - continue; >>>>>>> - } >>>>>>> +static void >>>>>>> +build_lrouter_flows_ingress_table_0_op( >>>>>>> + struct ovn_port *op, struct hmap *lflows) >>>>>>> +{ >>>>>>> + struct ds match = DS_EMPTY_INITIALIZER; >>>>>>> + struct ds actions = DS_EMPTY_INITIALIZER; >>>>>>> - if (!lrport_is_enabled(op->nbrp)) { >>>>>>> - /* Drop packets from disabled logical ports (since logical >>>>>>> flow >>>>>>> - * tables are default-drop). */ >>>>>>> - continue; >>>>>>> - } >>>>>>> + /* Logical router ingress table 0: match (priority 50). >>>>>>> + * Drop packets from disabled logical ports (since logical flow >>>>>>> + * tables are default-drop). >>>>>>> + * No ingress packets should be received on a chassisredirect >>>>>>> + * port. */ >>>>>>> - if (op->derived) { >>>>>>> - /* No ingress packets should be received on a >>>>>>> chassisredirect >>>>>>> - * port. */ >>>>>>> - continue; >>>>>>> - } >>>>>>> + if (op->nbrp && lrport_is_enabled(op->nbrp) && (!op->derived)) { >>>>>>> /* Store the ethernet address of the port receiving the >>>>>>> packet. >>>>>>> * This will save us from having to match on inport further >>>>>>> down in >>>>>>> * the pipeline. >>>>>>> */ >>>>>>> - ds_clear(&actions); >>>>>>> ds_put_format(&actions, REG_INPORT_ETH_ADDR " = %s; next;", >>>>>>> op->lrp_networks.ea_s); >>>>>>> - ds_clear(&match); >>>>>>> ds_put_format(&match, "eth.mcast && inport == %s", >>>>>>> op->json_key); >>>>>>> ovn_lflow_add_with_hint(lflows, op->od, >>>>>>> S_ROUTER_IN_ADMISSION, 50, >>>>>>> ds_cstr(&match), ds_cstr(&actions), >>>>>>> @@ -8486,6 +8469,30 @@ build_lrouter_flows(struct hmap *datapaths, >>>>>>> struct hmap *ports, >>>>>>> ds_cstr(&match), ds_cstr(&actions), >>>>>>> &op->nbrp->header_); >>>>>>> } >>>>>>> + ds_destroy(&match); >>>>>>> + ds_destroy(&actions); >>>>>> I didn't run any performance tests, but since you were recently fighting >>>>>> with >>>>>> number of memory allocations, I have to mention that this patch set has >>>>>> one >>>>>> side effect: you're factoring out code into functions and that makes you >>>>>> to >>>>>> re-allocate dynamic strings for 'match' and 'actions' for every function >>>>>> call >>>>>> instead of re-using with simple ds_clear() as it done right now. >>>>>> There are lots of function calls and most of them done inside loops that >>>>>> iterates >>>>>> over all ports or all datapaths, so I'm expecting thousands of extra >>>>>> memory >>>>>> allocations. This has a potential to impact performance at scale. >>>>> >>>>> Fair point, these should be allocated once and passed to all functions >>>>> which need them as "scratchpads". >>>> >>>> Might be better to make them static to avoid extra code complexity. >>>> Should not eat too much memory. >>> >>> While at it, there are quite a few pieces of code like this: >>> >>> ds_clear(&actions); >>> ds_put_cstr(&actions, >>> "ip6.dst <-> ip6.src; " >>> "ip.ttl = 255; " >>> "icmp6.type = 129; " >>> "flags.loopback = 1; " >>> "next; "); >>> >>> The cstr contents which are put are constant and do not change. >>> >>> This is inside a loop so it is done on every iteration. >>> >>> That is a realloc and memcpy every time instead of having a predefined >>> const struct ds or reusing this set of actions once they have been created. >> >> 1. There is no realloc since memory is not freed, but reused (see ds_clear). >> >> 2. I do not see a lot of places like this. Actually, on a quick look I >> see only this one place in section 'ICMPv6 echo reply'. And, yes, >> this case could be easily optimized by just using constant string >> directly, the same way as it done for 'UDP/TCP port unreachable' case >> few lines below. Feel free to send a separate patch for this. > > There are more. > > ds_put_cstr(match, "ip4 && ip.ttl == {0, 1}"); > > formats without any variable part in ds_put_format > > ds_put_format(actions, > "ip4.dst <-> ip4.src; " > "ip.ttl = 255; " > "icmp4.type = 0; " > "flags.loopback = 1; " > "next; "); > > and so on. > > It is not just that case.
OK. I see. If you know where all these places are it should not be hard to fix. Just assign the string to 'const char *' variable and use it. But this needs to be a separate patch to not mix up refactoring with performance optimizations. > >> >>> >>>> >>>>> >>>>> I will release a new version next week which takes this into account. >>>>> >>>>>> >>>>>> Best regards, Ilya Maximets. >>>>>> >>>> >>>> >>> >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
