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. > > 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
