On 15/09/2020 12:12, Numan Siddique 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.

Cool, thanks. I will rebase my branch off that.


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.

Cool.

I found that in its absence, diff generates stuff that is not readable and impossible to rebase even after minimal changes.

In the meantime, I reworked the actual multi-threaded processing patch so that it unifies lswitch and lrouter processing and runs the worker trheadpool once per iteration. I am going to re-test that - it should improve performance and scalability a bit further.

A.


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