[ovs-dev] [PATCH ovn v5 01/16] ovn-northd: Move out Table 0 (ingress) operations to functions
Numan Siddique
numans at ovn.org
Tue Sep 15 12:02:24 UTC 2020
On Tue, Sep 15, 2020 at 4:42 PM Numan Siddique <numans at ovn.org> wrote:
>
>
>
> On Fri, Sep 11, 2020 at 3:11 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>
>>
>
> Hi Anton,
>
> Thanks for this series.
>
> I found most of the patches to be splitting the code into functions into
> proper logical blocks.
> However, patches 3 to 7, split the code into functions which I think can
> be further reorganized properly.
> What I mean is that if we take ARP response flows for NAT entries as an
> example, the
> function build_lrouter_flows_ip_input_od() in the patch 3 adds ARP
> response flows for some scenarios
> and the function build_lrouter_flows_ipv4_input_table_3_op() in patch 4
> adds ARP response flows
> for other scenarios.
>
> I think it's better to revisit patches 3 to 7 and move out the code into
> functions which separates
> the lflow generation properly.
>
> I also think the function names are a bit odd and the naming can be
> improved.
>
> I found other patches in the series to be fine except for the function
> naming.
> Hence I reworked a bit renaming the functions and moving the comments from
> the function
> declarations to near the function definitions. I think this will be more
> helpful.
> And I applied the patches 1,2, 8 to 16 to master.
>
> Please note I also removed the marker comment while committing the patch
> 16. I think we can remove it
> as it's a bit odd to keep the comment.
>
Another thing which I forgot to mention is that I modified the commit
messages for most of the commits.
Thanks
Numan
> Thanks
> Numan
>
>
>> ---
>> northd/ovn-northd.c | 135 +++++++++++++++++++++++++++-----------------
>> 1 file changed, 82 insertions(+), 53 deletions(-)
>>
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index b95d6cd8a..14e4a6939 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -8482,6 +8482,26 @@ build_lrouter_force_snat_flows(struct hmap
>> *lflows, struct ovn_datapath *od,
>> ds_destroy(&actions);
>> }
>>
>> +
>> +/* Logical router ingress table 0: Admission control framework. */
>> +static void
>> +build_lrouter_flows_ingress_table_0_od(
>> + struct ovn_datapath *od, struct hmap *lflows);
>> +
>> +/* Logical router ingress table 0: match (priority 50). */
>> +static void
>> +build_lrouter_flows_ingress_table_0_op(
>> + struct ovn_port *op, struct hmap *lflows,
>> + struct ds *match, struct ds *actions);
>> +
>> +/*
>> + * Do not remove this comment - it is here on purpose
>> + * It serves as a marker so that pulling operations out
>> + * of build_lrouter_flows results in a clean diff with
>> + * a separate new operations function and clean changes
>> + * to build_lroute_flows
>> + */
>> +
>> static void
>> build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>> struct hmap *lflows, struct shash *meter_groups,
>> @@ -8493,65 +8513,14 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>> 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;
>> - }
>> -
>> - /* 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;");
>> + build_lrouter_flows_ingress_table_0_od(od, lflows);
>> }
>>
>> - /* Logical router ingress table 0: match (priority 50). */
>> struct ovn_port *op;
>> HMAP_FOR_EACH (op, key_node, ports) {
>> - if (!op->nbrp) {
>> - continue;
>> - }
>> -
>> - if (!lrport_is_enabled(op->nbrp)) {
>> - /* Drop packets from disabled logical ports (since logical
>> flow
>> - * tables are default-drop). */
>> - continue;
>> - }
>> -
>> - if (op->derived) {
>> - /* No ingress packets should be received on a chassisredirect
>> - * port. */
>> - continue;
>> - }
>> -
>> - /* 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),
>> - &op->nbrp->header_);
>> -
>> - ds_clear(&match);
>> - ds_put_format(&match, "eth.dst == %s && inport == %s",
>> - op->lrp_networks.ea_s, op->json_key);
>> - if (op->od->l3dgw_port && op == op->od->l3dgw_port
>> - && op->od->l3redirect_port) {
>> - /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>> - * should only be received on the "redirect-chassis". */
>> - ds_put_format(&match, " && is_chassis_resident(%s)",
>> - op->od->l3redirect_port->json_key);
>> - }
>> - ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION,
>> 50,
>> - ds_cstr(&match), ds_cstr(&actions),
>> - &op->nbrp->header_);
>> + build_lrouter_flows_ingress_table_0_op(op, lflows, &match,
>> &actions);
>> }
>>
>> /* Logical router ingress table 1: LOOKUP_NEIGHBOR and
>> @@ -10956,6 +10925,66 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>> ds_destroy(&actions);
>> }
>>
>> +static void
>> +build_lrouter_flows_ingress_table_0_od(
>> + struct ovn_datapath *od, struct hmap *lflows)
>> +{
>> + 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;");
>> + }
>> +}
>> +
>> +static void
>> +build_lrouter_flows_ingress_table_0_op(
>> + struct ovn_port *op, struct hmap *lflows,
>> + struct ds *match, struct ds *actions)
>> +{
>> + if (op->nbrp) {
>> + if (!lrport_is_enabled(op->nbrp)) {
>> + /* Drop packets from disabled logical ports (since logical
>> flow
>> + * tables are default-drop). */
>> + return;
>> + }
>> +
>> + if (op->derived) {
>> + /* No ingress packets should be received on a chassisredirect
>> + * port. */
>> + return;
>> + }
>> +
>> + /* 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),
>> + &op->nbrp->header_);
>> +
>> + ds_clear(match);
>> + ds_put_format(match, "eth.dst == %s && inport == %s",
>> + op->lrp_networks.ea_s, op->json_key);
>> + if (op->od->l3dgw_port && op == op->od->l3dgw_port
>> + && op->od->l3redirect_port) {
>> + /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>> + * should only be received on the "redirect-chassis". */
>> + ds_put_format(match, " && is_chassis_resident(%s)",
>> + op->od->l3redirect_port->json_key);
>> + }
>> + ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION,
>> 50,
>> + ds_cstr(match), ds_cstr(actions),
>> + &op->nbrp->header_);
>> + }
>> +}
>> +
>> /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB
>> database,
>> * constructing their contents based on the OVN_NB database. */
>> static void
>> --
>> 2.20.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
More information about the dev
mailing list