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

Anton Ivanov anton.ivanov at cambridgegreys.com
Fri Sep 4 18:22:47 UTC 2020


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

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