[ovs-dev] [PATCH ovn v5 01/16] ovn-northd: Move out Table 0 (ingress) operations to functions

Anton Ivanov anton.ivanov at cambridgegreys.com
Tue Sep 15 18:25:03 UTC 2020


On 15/09/2020 13:02, Numan Siddique wrote:
>
>
> On Tue, Sep 15, 2020 at 4:42 PM Numan Siddique <numans at ovn.org 
> <mailto:numans at ovn.org>> wrote:
>
>
>
>
>     On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov at cambridgegreys.com
>     <mailto:anton.ivanov at cambridgegreys.com>> wrote:
>
>         From: Anton Ivanov <anton.ivanov at cambridgegreys.com
>         <mailto:anton.ivanov at cambridgegreys.com>>
>
>         Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com
>         <mailto: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.


Cool. Thanks.

I have pushed the parallelised ovn-northd branch on top of my changes (I 
have not rebased it yet).

https://github.com/kot-begemot-uk/ovn/tree/reintroduce-parallel-processing

You can see some of the rationale for the naming there - in the last few 
commits the actual processing is re-arranged and then the naming 
convention starts making a bit more sense as it fits the way things are 
iterated.

A.

> 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 <mailto:dev at openvswitch.org>
>         https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/



More information about the dev mailing list