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.
I will release a new version next week which takes this into account.
Best regards, Ilya Maximets.
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
