On 15/09/2020 13:02, Numan Siddique wrote:


On Tue, Sep 15, 2020 at 4:42 PM Numan Siddique <[email protected] <mailto:[email protected]>> wrote:




    On Fri, Sep 11, 2020 at 3:11 PM <[email protected]
    <mailto:[email protected]>> wrote:

        From: Anton Ivanov <[email protected]
        <mailto:[email protected]>>

        Signed-off-by: Anton Ivanov <[email protected]
        <mailto:[email protected]>>


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


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

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to