[ovs-dev] [PATCH ovn RFC v4 01/24] Move out Table 0 (ingress) operations to functions

Ilya Maximets i.maximets at ovn.org
Fri Sep 4 14:00:12 UTC 2020


On 9/2/20 4:59 PM, anton.ivanov at cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
> 
> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>
> ---
>  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.

Best regards, Ilya Maximets.


More information about the dev mailing list