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

Ilya Maximets i.maximets at ovn.org
Mon Sep 7 17:39:25 UTC 2020


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

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.

> 
>>
>>>
>>> I will release a new version next week which takes this into account.
>>>
>>>>
>>>> Best regards, Ilya Maximets.
>>>>
>>
>>
> 



More information about the dev mailing list