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

Anton Ivanov anton.ivanov at cambridgegreys.com
Tue Sep 8 06:51:22 UTC 2020



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

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/


More information about the dev mailing list