[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