[ovs-dev] [PATCH ovn v3 1/6] ovn-northd: reorganize processing of lflows

[Ilya Maximets]

On 9/24/20 11:27 AM, Anton Ivanov wrote:
> 
> 
> On 23/09/2020 21:50, Ilya Maximets wrote:
>> On 9/23/20 8:00 PM, anton.ivanov at cambridgegreys.com wrote:
>>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>>>
>>> 1. Merge lrouter and lswitch processing.
>>> 2. Move lrouter and lswitch lflow generation which uses the
>>> same iterator variables into common helpers
>>> 3. Set up structures to be used in parallel and sequential mode
>>
>> This patch-set doesn't introduce any parallel procesing, so please,
>> do not mention it or make it clear that it's not yet implemented, so
>> the person who will look throught the commit history will not be
>> confused.
>>
>>>
>>> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>>> ---
>>
>> Some style commnets inline.
>> All of them are applicable, actually, to all other patches of the set.
>> So, I'm only reviewing this one.
>>
>> Best regards, Ilya Maximents.
>>
>>>   northd/ovn-northd.c | 199 +++++++++++++++++++++++++++-----------------
>>>   1 file changed, 123 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>>> index 3324c9e81..5faa6cee6 100644
>>> --- a/northd/ovn-northd.c
>>> +++ b/northd/ovn-northd.c
>>> @@ -8874,24 +8874,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>>>       struct ds actions = DS_EMPTY_INITIALIZER;
>>>         struct ovn_datapath *od;
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_adm_ctrl_flows_for_lrouter(od, lflows);
>>> -    }
>>> -
>>>       struct ovn_port *op;
>>> -    HMAP_FOR_EACH (op, key_node, ports) {
>>> -        build_adm_ctrl_flows_for_lrouter_port(op, lflows, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_neigh_learning_flows_for_lrouter(
>>> -                od, lflows, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (op, key_node, ports) {
>>> -        build_neigh_learning_flows_for_lrouter_port(
>>> -                op, lflows, &match, &actions);
>>> -    }
>>>         /* Drop IP traffic destined to router owned IPs. Part of it is dropped
>>>        * in stage "lr_in_ip_input" but traffic that could have been unSNATed
>>> @@ -9935,63 +9918,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>>>           sset_destroy(&nat_entries);
>>>       }
>>>   -    HMAP_FOR_EACH (op, key_node, ports) {
>>> -        build_ND_RA_flows_for_lrouter_port(op, lflows, &match, &actions);
>>> -    }
>>> -
>>> -    /* Logical router ingress table ND_RA_OPTIONS & ND_RA_RESPONSE: RS
>>> -     * responder, by default goto next. (priority 0). */
>>
>> You lost this comment.
> 
> All comments documenting functions are at the funciton declarations - this
> was a leftover duplicate.
> 
> This comment is on line 8776 in the patched version now to fit with all others.
> 
>>
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_ND_RA_flows_for_lrouter(od, lflows);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (op, key_node, ports) {
>>> -        build_ip_routing_flows_for_lrouter_port(op, lflows);
>>> -    }
>>> -
>>> -    /* Convert the static routes to flows. */
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_static_route_flows_for_lrouter(od, lflows, ports);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_mcast_lookup_flows_for_lrouter(od, lflows, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_ingress_policy_flows_for_lrouter(od, lflows, ports);
>>> -    }
>>> -
>>> -    /* XXX destination unreachable */
>>
>> And this comment also disappeared.
> 
> This one was unclear - it was stand alone and not relating to the actual code which followed and had its own comments.
> 
> I can restore it following the same convention as the previous one.
> 
>>
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_arp_resolve_flows_for_lrouter(od, lflows);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (op, key_node, ports) {
>>> -        build_arp_resolve_flows_for_lrouter_port(
>>> -                op, lflows, ports, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_check_pkt_len_flows_for_lrouter(
>>> -                od, lflows, ports, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_gateway_redirect_flows_for_lrouter(
>>> -                od, lflows, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (od, key_node, datapaths) {
>>> -        build_arp_request_flows_for_lrouter(od, lflows, &match, &actions);
>>> -    }
>>> -
>>> -    HMAP_FOR_EACH (op, key_node, ports) {
>>> -        build_egress_delivery_flows_for_lrouter_port(
>>> -                op, lflows, &match, &actions);
>>> -    }
>>> -
>>>       ds_destroy(&match);
>>>       ds_destroy(&actions);
>>>   }
>>> @@ -11389,6 +11315,128 @@ build_ipv6_input_flows_for_lrouter_port(
>>>     }
>>>   +struct lswitch_flow_build_info {
>>> +    struct hmap *datapaths;
>>> +    struct hmap *ports;
>>> +    struct hmap *port_groups;
>>> +    struct hmap *lflows;
>>> +    struct hmap *mcgroups;
>>> +    struct hmap *igmp_groups;
>>> +    struct shash *meter_groups;
>>> +    struct hmap *lbs;
>>> +    char *svc_check_match;
>>> +    struct ds match;
>>> +    struct ds actions;
>>> +};
>>> +
>>> +/* Helper function to combine all lflow generation which is iterated by
>>> + * datapath. It can be invoked with a lsi argument containing "all work"
>>> + * in single threaded mode or an lsi argument containing a "chunk of work"
>>> + * in parallel.
>>
>> Please, don't mention parallel execution anywhere in the code until we actually
>> have it.  This is really confusing for someone who reads that.
>>
>>> + */
>>> +
>>
>> Please, do not put an empty line between a comment and a function in belongs.
>>
>>> +static void
>>> +    build_converged_iterate_by_od(
>>
>> And, please, do not ever format lines like this.  There is a purpose why
>> function names are on their own line and starts right from the start of
>> the line.  And the reason is that you could grep for function definition,
>> i.e. git grep '^build_lrouter_flows' or to quickly find definition within
>> a text editor.
>>
>>> +            struct ovn_datapath *od, struct lswitch_flow_build_info *lsi)
>>
>> One of these arguments could be easily placed on the same line with the
>> function name.  So, please, place it there.  It's easier to read and
>> saves a line of code.
>>
>>> +{
>>> +
>>> +    /* Build Logical Router Flows */
>>
>> Please, add period to the end of the comment.  Same for all other comments
>> in all other patches.
>>
>>> +    build_adm_ctrl_flows_for_lrouter(od, lsi->lflows);
>>> +    build_neigh_learning_flows_for_lrouter(
>>> +            od, lsi->lflows, &lsi->match, &lsi->actions);
>>> +    build_ND_RA_flows_for_lrouter(od, lsi->lflows);
>>> +    build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->ports);
>>> +    build_mcast_lookup_flows_for_lrouter(
>>> +            od, lsi->lflows, &lsi->match, &lsi->actions);
>>> +    build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->ports);
>>> +    build_arp_resolve_flows_for_lrouter(od, lsi->lflows);
>>> +    build_check_pkt_len_flows_for_lrouter(
>>> +            od, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions);
>>> +    build_gateway_redirect_flows_for_lrouter(
>>> +            od, lsi->lflows, &lsi->match, &lsi->actions);
>>> +    build_arp_request_flows_for_lrouter(
>>> +            od, lsi->lflows, &lsi->match, &lsi->actions);
>>> +}
>>> +
>>> +/* Helper function to combine all lflow generation which is iterated by port.
>>> + * It can be invoked with a lsi argument containing "all work" in single
>>> + * threaded mode or an lsi argument containing a "chunk of work" in parallel.
>>
>> Ditto.
>>
>>> + */
>>> +
>>
>> Ditto.
>>
>>> +static void
>>> +    build_converged_iterate_by_op(
>>
>> Ditto.
>>
>>> +                    struct ovn_port *op,
>>> +                    struct lswitch_flow_build_info *lsi)
>>
>> Ditto.
>>
>>> +{
>>> +    /* Build Logical Router Flows */
>>
>> Ditto.
>>
>>> +
>>> +    build_adm_ctrl_flows_for_lrouter_port(
>>> +            op, lsi->lflows, &lsi->match, &lsi->actions);
>>> +    build_neigh_learning_flows_for_lrouter_port(
>>> +            op, lsi->lflows, &lsi->match, &lsi->actions);
>>> +    build_ip_routing_flows_for_lrouter_port(op, lsi->lflows);
>>> +    build_ND_RA_flows_for_lrouter_port(
>>> +            op, lsi->lflows, &lsi->match, &lsi->actions);
>>> +    build_arp_resolve_flows_for_lrouter_port(
>>> +            op, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions);
>>> +    build_egress_delivery_flows_for_lrouter_port(
>>> +            op, lsi->lflows, &lsi->match, &lsi->actions);
>>> +}
>>> +
>>> +/*
>>> + * Combined LFLOW processing function. Intended to iterate over
>>
>> Why LFLOW capitalized?
>>
>>> + * datapaths, ports, lbs and igmp_groups in single threaded mode
>>> + * or prepare and invoke a thread pool in multi-threaded mode.
>>
>> Ditto.
>>
>>> + * Must not contain any direct flow ops - all actual flow ops
>>> + * should be invoked out the per-iterable helper functions.
>>> + */
>>> +
>>> +static void
>>> +build_converged_flows(struct hmap *datapaths, struct hmap *ports,
>>
>> This function name looks really strange and also names of both other fucntions
>> in this patch.  I'd say we need a better name.  Current one makes an impression
>> that we have some kind of entity named 'converged flow'.
>>
>>> +                    struct hmap *port_groups, struct hmap *lflows,
>>> +                    struct hmap *mcgroups, struct hmap *igmp_groups,
>>> +                    struct shash *meter_groups,
>>> +                    struct hmap *lbs)
>>
>> Please, fix up intents.  Above lines should be shifted 2 spaces to the right.
>>
>>> +{
>>> +    struct lswitch_flow_build_info lsi;
>>> +
>>
>> Please, don't add extra empty lines between definitions.
>>
>>> +    struct ovn_datapath *od;
>>> +    struct ovn_port *op;
>>> +
>>> +    char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
>>> +
>>> +    lsi.datapaths = datapaths;
>>> +    lsi.ports = ports;
>>> +    lsi.port_groups = port_groups;
>>> +    lsi.lflows = lflows;
>>> +    lsi.mcgroups = mcgroups;
>>> +    lsi.igmp_groups = igmp_groups;
>>> +    lsi.meter_groups = meter_groups;
>>> +    lsi.lbs = lbs;
>>> +    lsi.svc_check_match = svc_check_match;
>>> +    lsi.match = (struct ds) DS_EMPTY_INITIALIZER;
>>> +    lsi.actions = (struct ds) DS_EMPTY_INITIALIZER;
>>
>> Why cast?
> 
> GCC being stupid - barfs without. Why? No idea
> 
> lsi.actions = DS_EMPTY_INITIALIZER;
> 
> is syntactically correct, but it does not want to consume it.

I see.  DS_EMPTY_INITIALIZER expands to '{ NULL, 0, 0 }' and this form is not
an expression, i.e. there is a syntax error.  Cast makes the right-hand value
to be a compound literal that is an expression and could be used in assignment.

Please, use a designated initializer instead like I showed below and it will
work fine without need to cast, i.e.

struct lswitch_flow_build_info lsi = {
    .datapaths = datapaths,
    ...
    .match     = DS_EMPTY_INITIALIZER,
    .actions   = DS_EMPTY_INITIALIZER,
};