[ovs-dev] [PATCH ovs V2 01/21] tc: Add tc flower interface

Joe Stringer joe at ovn.org
Tue Jan 10 18:48:09 UTC 2017


On 10 January 2017 at 08:42, Paul Blakey <paulb at mellanox.com> wrote:
> On 05/01/2017 02:57, Joe Stringer wrote:
> Another question: What happens if someone manually configures
> additional flower (or non-flower?) filters on devices? Does OVS
> complain constantly in the logs, or handle it ok? Overwrite the user
> configuration?
>
> For now additional flower are interpreted back to OVS rules and will be
> generated a new UFID.
> Non flower filters are ignored by dump.

OK, so most likely ovs-vswitchd will delete the flow if it doesn't
match the OpenFlow tables.

A further question: Have you tried installing lots of flows to observe
the behaviour?

One thing that I will sometimes to for testing purposes is to disable
megaflows, then run portscan through OVS with a simple flow table.
This is something like 'worst case' behaviour if the flow table is
very complex and you have adverse traffic patterns. In such an
artifical test, you can easily get OVS into a state where it's trying
to install and delete tens (hundred?) of thousands of flows per second
into the datapath. There are some heuristics in the
ofproto-dpif-upcall logic for how to behave in such a situation, but
with hardware offloads this may break down. In particular, it may try
to add/delete flows from the hardware too aggressively.

Maybe this is more tied to the 'offload policy' question which was
tabled for this version. But something to be aware of and think about.

> +static int
> +__nl_parse_flower_actions(struct nlattr **attrs, struct tc_flow *tc_flow)
> +{
> +    const struct nlattr *actions = attrs[TCA_FLOWER_ACT];
> +    static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] = {
> };
> +    struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)];
> +
> +    for (int i = 0; i < TCA_ACT_MAX_PRIO + 1; i++) {
>
> Perhaps reuse ARRAY_SIZE(...)?
>
> +        actions_orders_policy[i].type = NL_A_NESTED;
> +        actions_orders_policy[i].optional = true;
> +    }
> +
> +    if (!nl_parse_nested(actions, actions_orders_policy, actions_orders,
> +                         ARRAY_SIZE(actions_orders_policy))) {
> +        VLOG_ERR("failed to parse flower order of actions");
> +        return EPROTO;
> +    }
> +
> +    for (int i = 1; i < TCA_ACT_MAX_PRIO + 1; i++) {
>
> Same here.
>
> Also, why start from offset 1?
>
> Since there is no action with priority/index 0 (see act_api.c +:617).

OK, please provide a constant something like TCA_ACT_MIN_PRIO and use
this instead of "1" to make this more clear. (You can rename to
something else if it doesn't exist in TC interface and only lives in
OVS).

Also, the two for() loops in the above hunk inconsistently use 0 or 1
to start with TCA_ACT_MAX_PRIO as the upper bound, please make these
more consistent.

> +static void
> +__nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flow *tc_flow)
> +{
> +    size_t offset;
> +    size_t act_offset;
> +
> +    offset = nl_msg_start_nested(request, TCA_FLOWER_ACT);
> +    {
> +        uint16_t act_index = 1;
> +        bool done = false;
> +
> +        while (!done) {
> +            act_offset = nl_msg_start_nested(request, act_index);
> +            {
>
> Why the extra brace/indentation?
>
> To clearly see what is nested (especially when its nested multiple times)
> and not forget to finish the nested part.
> We can change that if you'd like.

That's reasonable, feel free to leave it as is for now.

> +int
> +tc_replace_flower(struct tc_flow *tc_flow, uint16_t prio)
> +{
> +    struct ofpbuf request;
> +    struct tcmsg *tcmsg;
> +    struct ofpbuf *reply;
> +    int error = 0;
> +    size_t basic_offset;
> +
> +    tcmsg = tc_make_req(tc_flow->ifindex, RTM_NEWTFILTER,
> +                        NLM_F_CREATE | NLM_F_ECHO, &request);
> +    tcmsg->tcm_parent = tc_make_handle(0xffff, 0);
> +    tcmsg->tcm_info = tc_make_handle((OVS_FORCE uint16_t) prio,
> +                                     (OVS_FORCE uint16_t)
> tc_flow->key.eth_type);
> +    tcmsg->tcm_handle = tc_flow->handle;
> +
> +    /* flower */
> +    nl_msg_put_string(&request, TCA_KIND, "flower");
> +    basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS);
> +    {
> +        __nl_msg_put_flower_options(&request, tc_flow);
> +    }
> +    nl_msg_end_nested(&request, basic_offset);
> +
> +    error = tc_transact(&request, &reply);
> +    if (!error) {
> +        struct tcmsg *tc =
> +            ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tc);
> +
> +        tc_flow->prio = TC_H_MAJ(tc->tcm_info) >> 16;
> +        tc_flow->handle = tc->tcm_handle;
> +    }
> +
> +    return error;
> +}
>
> Again, maybe there's some common code to be shared.
>
> This is flower specific.

You're right, not sure what I was referring to before. At least
tc_make_req() and tc_make_request() should be shareable.

Extra feedback: this function populates tc_flow->prio and
tc_flow->handle, but netdev_tc_flow_put() only uses the handle and
reuses the priority that it provided to this function. Is that
intentional?

You might consider adapting this function to make inputs and outputs
more clear. It wasn't obvious to me at a glance that 'tc_flow' is
modified by this layer to return the handle/prio back up. tc_flow
could be const, showing that this function just takes it and installs
it, then if it's successful populates another parameter or two for the
return values - handle/prio. (I assume these are the only returned
values, but I didn't look too closely).


More information about the dev mailing list