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

Ilya Maximets i.maximets at ovn.org
Tue Sep 8 11:23:57 UTC 2020


On 9/8/20 8:51 AM, Anton Ivanov wrote:
> 
> 
> 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.

OK.  I see.  If you know where all these places are it should not be hard to fix.
Just assign the string to 'const char *' variable and use it.

But this needs to be a separate patch to not mix up refactoring with performance
optimizations.

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



More information about the dev mailing list