| Message ID | 20200902145950.25875-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,RFC,v4,01/24] Move out Table 0 (ingress) operations to functions | expand |
On 9/2/20 4:59 PM, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > Signed-off-by: Anton Ivanov <anton.ivanov@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. Best regards, Ilya Maximets.
On 04/09/2020 15:00, Ilya Maximets wrote: > On 9/2/20 4:59 PM, anton.ivanov@cambridgegreys.com wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> Signed-off-by: Anton Ivanov <anton.ivanov@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. >
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@cambridgegreys.com wrote: >>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> >>> Signed-off-by: Anton Ivanov <anton.ivanov@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. > > I will release a new version next week which takes this into account. > >> >> Best regards, Ilya Maximets. >>
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@cambridgegreys.com wrote: >>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>>> >>>> Signed-off-by: Anton Ivanov <anton.ivanov@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. Then they have to become parameters again if this is split into multiple processing threads. It is better to make them a parameter day one. IIRC, there was some code in OVS which was written that way already. Brgds, > >> >> I will release a new version next week which takes this into account. >> >>> >>> Best regards, Ilya Maximets. >>> > >
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@cambridgegreys.com wrote: >>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>>> >>>> Signed-off-by: Anton Ivanov <anton.ivanov@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. > >> >> I will release a new version next week which takes this into account. >> >>> >>> Best regards, Ilya Maximets. >>> > >
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@cambridgegreys.com wrote: >>>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>>>> >>>>> Signed-off-by: Anton Ivanov <anton.ivanov@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. >>>> >> >> >
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@cambridgegreys.com wrote: >>>>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>>>>> >>>>>> Signed-off-by: Anton Ivanov <anton.ivanov@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. >>>>> >>> >>> >> > >
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@cambridgegreys.com wrote: >>>>>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>>>>>> >>>>>>> Signed-off-by: Anton Ivanov <anton.ivanov@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. >>>>>> >>>> >>>> >>> >> >> >
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); +} + +static void +build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, + struct hmap *lflows, struct shash *meter_groups, + struct hmap *lbs) +{ + /* 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; + + struct ovn_datapath *od; + HMAP_FOR_EACH (od, key_node, datapaths) { + build_lrouter_flows_ingress_table_0_od(od, lflows); + } + + struct ovn_port *op; + HMAP_FOR_EACH (op, key_node, ports) { + build_lrouter_flows_ingress_table_0_op(op, lflows); + } /* Logical router ingress table 1: LOOKUP_NEIGHBOR and * table 2: LEARN_NEIGHBOR. */