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

[Anton Ivanov]

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

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