On 17 Feb 2022, at 8:41, Roi Dayan wrote:
> On 2022-02-16 12:15 PM, Eelco Chaudron wrote: >> >> >> On 16 Feb 2022, at 10:48, Roi Dayan wrote: >> >>> On 2022-02-16 10:37 AM, Eelco Chaudron wrote: >>>> >>>> >>>> On 15 Feb 2022, at 10:52, Roi Dayan wrote: >>>> >>>>> From: Chris Mi <[email protected]> >>>>> >>>>> Currently, tc merges all header rewrite actions into one tc pedit >>>>> action. So the header rewrite actions order is lost. Save each header >>>>> rewrite action into one tc pedit action to keep the order. And only >>>>> append one tc csum action to the last pedit action of a series. >>>> >>>> I’m on training all of this week, so will try to look at this later. >>>> But in the meantime, do you think it’s worth adding a test case for this? >>>> >>>> //Eelco >>> >>> maybe. but the fix is already ready for some time while it looks like we >>> could add many potential cases related to tc. >>> We should and will find the time later to look at adding more unit tests >>> into the repo. >>> thanks >> >> The reason for asking for a test case is that when reviewing TC-related >> patches I no longer just do a visual review, I test them. This is because a >> lot of times a small change results in the introduction of problems, for >> example with the dpctl/dump-flows output. So without any test cases, I need >> to reverse engineer how to replicate the problem, and then test it. >> >> So my request would be if you are not supplying a test case, at least supply >> a simple replicator in the commit description. >> > > I understand. I thought in this case it's clear. > > before this commit all header rewrite actions are saved into > flower->rewrite and from there translated to single tc pedit action. > > This patch saves header rewrite in action->rewrite per action instance > so translating actions to tc ends up with multiple/separated tc pedit > actions. > > as we already agreed we will try to improve the testsuite > in the ovs repo in near future. > > We can reproduce the issue like this: > Config ovs with hw-offload=true. > Create bridge with 2 ports. > > execute: > > ovs-appctl dpctl/add-flow > "ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),in_port(2),eth_type(0x0800),eth(),ipv4()" > "set(eth(dst=76:59:99:fb:aa:aa)),3,set(eth(dst=76:59:99:fb:ff:ff))" ; tc > filter show dev enp8s0f0 ingress; > > > before this commit you will see in TC pedit,mirred and after the commit > you will see pedit,mirred,pedit. > > do you want me to add the steps to the commit msg? Thanks for the details, and yes, it might be handy to have this in the commit message. Let's say this commit is causing a problem (there might be one, see below). Someone trying to understand why this commit was added, has a quick way to test it. Now for the problem I see when doing some simple testing (I did not review the code, so maybe it's obvious), but with the patch applied I, execute the following: ovs-appctl dpctl/add-flow "ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),in_port(2),eth_type(0x0800),eth(),ipv4(proto=6),tcp()" "set(ipv4(ttl=3)),3,set(ipv4(ttl=4))" && ovs-appctl dpctl/dump-flows type=tc && tc filter show dev enp4s0f0 ingress This is leaving the revalidators in some kind of a loop. Without the patch, I do not see the problem. This is a log snippet: 2022-02-17T15:38:52.990Z|00050|tc|WARN|expected act csum with flags: 0x9 2022-02-17T15:38:52.990Z|00051|tc|WARN|Kernel flower acknowledgment does not match request! Set dpif_netlink to dbg to see which rule caused this error. 2022-02-17T15:38:52.993Z|00052|tc|WARN|expected act csum with flags: 0x9 2022-02-17T15:38:53.052Z|00001|tc(revalidator18)|WARN|expected act csum with flags: 0x9 2022-02-17T15:38:53.052Z|00002|netdev_offload_tc(revalidator18)|ERR|flow get failed (dev ovs-system prio 3 handle 1): Invalid argument 2022-02-17T15:38:53.052Z|00003|tc(revalidator18)|WARN|expected act csum with flags: 0x9 2022-02-17T15:38:53.052Z|00004|netdev_offload_tc(revalidator18)|ERR|flow get failed (dev enp4s0f1 prio 3 handle 1): Invalid argument 2022-02-17T15:38:53.052Z|00005|netdev_offload_tc(revalidator18)|ERR|flow get failed (dev enp4s0f0 prio 3 handle 1): Invalid argument 2022-02-17T15:38:53.052Z|00006|netdev_offload_tc(revalidator18)|ERR|flow get failed (dev ovs_pvp_br0 prio 3 handle 1): Invalid argument 2022-02-17T15:38:53.052Z|00007|dpif(revalidator18)|WARN|system@ovs-system: failed to flow_get (No such file or directory) ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5 <empty>, packets:0, bytes:0, used:never 2022-02-17T15:38:53.052Z|00008|ofproto_dpif_upcall(revalidator18)|WARN|Failed to acquire udpif_key corresponding to unexpected flow (No such file or directory): ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5 So I guess this patch needs some fixing… I will do a popper code review for v3, and maybe you can add those test cases :) //Eelco >>>>> Signed-off-by: Chris Mi <[email protected]> >>>>> Reviewed-by: Roi Dayan <[email protected]> >>>>> --- >>>>> >>>>> v2: >>>>> - rebased to fix conflict. >>>>> >>>>> lib/netdev-offload-tc.c | 22 ++++++++------------ >>>>> lib/tc.c | 54 >>>>> +++++++++++++++++++++++++++++++++---------------- >>>>> lib/tc.h | 25 +++++++++++------------ >>>>> 3 files changed, 57 insertions(+), 44 deletions(-) >>>>> >>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>>> index 9845e8d3feae..ab2a678c6923 100644 >>>>> --- a/lib/netdev-offload-tc.c >>>>> +++ b/lib/netdev-offload-tc.c >>>>> @@ -481,10 +481,10 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump >>>>> *dump) >>>>> >>>>> static void >>>>> parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf, >>>>> - struct tc_flower *flower) >>>>> + struct tc_action *action) >>>>> { >>>>> - char *mask = (char *) &flower->rewrite.mask; >>>>> - char *data = (char *) &flower->rewrite.key; >>>>> + char *mask = (char *) &action->rewrite.mask; >>>>> + char *data = (char *) &action->rewrite.key; >>>>> >>>>> for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) { >>>>> char *put = NULL; >>>>> @@ -877,7 +877,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, >>>>> } >>>>> break; >>>>> case TC_ACT_PEDIT: { >>>>> - parse_flower_rewrite_to_netlink_action(buf, flower); >>>>> + parse_flower_rewrite_to_netlink_action(buf, action); >>>>> } >>>>> break; >>>>> case TC_ACT_ENCAP: { >>>>> @@ -1222,8 +1222,8 @@ parse_put_flow_set_masked_action(struct tc_flower >>>>> *flower, >>>>> uint64_t set_stub[1024 / 8]; >>>>> struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub); >>>>> char *set_data, *set_mask; >>>>> - char *key = (char *) &flower->rewrite.key; >>>>> - char *mask = (char *) &flower->rewrite.mask; >>>>> + char *key = (char *) &action->rewrite.key; >>>>> + char *mask = (char *) &action->rewrite.mask; >>>>> const struct nlattr *attr; >>>>> int i, j, type; >>>>> size_t size; >>>>> @@ -1265,14 +1265,6 @@ parse_put_flow_set_masked_action(struct tc_flower >>>>> *flower, >>>>> } >>>>> } >>>>> >>>>> - if (!is_all_zeros(&flower->rewrite, sizeof flower->rewrite)) { >>>>> - if (flower->rewrite.rewrite == false) { >>>>> - flower->rewrite.rewrite = true; >>>>> - action->type = TC_ACT_PEDIT; >>>>> - flower->action_count++; >>>>> - } >>>>> - } >>>>> - >>>>> if (hasmask && !is_all_zeros(set_mask, size)) { >>>>> VLOG_DBG_RL(&rl, "unsupported sub attribute of set action type >>>>> %d", >>>>> type); >>>>> @@ -1281,6 +1273,8 @@ parse_put_flow_set_masked_action(struct tc_flower >>>>> *flower, >>>>> } >>>>> >>>>> ofpbuf_uninit(&set_buf); >>>>> + action->type = TC_ACT_PEDIT; >>>>> + flower->action_count++; >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/lib/tc.c b/lib/tc.c >>>>> index adb2d3182ad4..b637f4c17ed9 100644 >>>>> --- a/lib/tc.c >>>>> +++ b/lib/tc.c >>>>> @@ -1006,14 +1006,14 @@ static const struct nl_policy pedit_policy[] = { >>>>> static int >>>>> nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower) >>>>> { >>>>> - struct tc_action *action; >>>>> + struct tc_action *action = &flower->actions[flower->action_count++]; >>>>> struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)]; >>>>> const struct tc_pedit *pe; >>>>> const struct tc_pedit_key *keys; >>>>> const struct nlattr *nla, *keys_ex, *ex_type; >>>>> const void *keys_attr; >>>>> - char *rewrite_key = (void *) &flower->rewrite.key; >>>>> - char *rewrite_mask = (void *) &flower->rewrite.mask; >>>>> + char *rewrite_key = (void *) &action->rewrite.key; >>>>> + char *rewrite_mask = (void *) &action->rewrite.mask; >>>>> size_t keys_ex_size, left; >>>>> int type, i = 0, err; >>>>> >>>>> @@ -1092,7 +1092,6 @@ nl_parse_act_pedit(struct nlattr *options, struct >>>>> tc_flower *flower) >>>>> i++; >>>>> } >>>>> >>>>> - action = &flower->actions[flower->action_count++]; >>>>> action->type = TC_ACT_PEDIT; >>>>> >>>>> return 0; >>>>> @@ -2399,14 +2398,14 @@ nl_msg_put_act_flags(struct ofpbuf *request) { >>>>> * first_word_mask/last_word_mask - the mask to use for the first/last >>>>> read >>>>> * (as we read entire words). */ >>>>> static void >>>>> -calc_offsets(struct tc_flower *flower, struct flower_key_to_pedit *m, >>>>> +calc_offsets(struct tc_action *action, struct flower_key_to_pedit *m, >>>>> int *cur_offset, int *cnt, ovs_be32 *last_word_mask, >>>>> ovs_be32 *first_word_mask, ovs_be32 **mask, ovs_be32 >>>>> **data) >>>>> { >>>>> int start_offset, max_offset, total_size; >>>>> int diff, right_zero_bits, left_zero_bits; >>>>> - char *rewrite_key = (void *) &flower->rewrite.key; >>>>> - char *rewrite_mask = (void *) &flower->rewrite.mask; >>>>> + char *rewrite_key = (void *) &action->rewrite.key; >>>>> + char *rewrite_mask = (void *) &action->rewrite.mask; >>>>> >>>>> max_offset = m->offset + m->size; >>>>> start_offset = ROUND_DOWN(m->offset, 4); >>>>> @@ -2473,7 +2472,8 @@ csum_update_flag(struct tc_flower *flower, >>>>> >>>>> static int >>>>> nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request, >>>>> - struct tc_flower *flower) >>>>> + struct tc_flower *flower, >>>>> + struct tc_action *action) >>>>> { >>>>> struct { >>>>> struct tc_pedit sel; >>>>> @@ -2497,7 +2497,7 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf >>>>> *request, >>>>> continue; >>>>> } >>>>> >>>>> - calc_offsets(flower, m, &cur_offset, &cnt, &last_word_mask, >>>>> + calc_offsets(action, m, &cur_offset, &cnt, &last_word_mask, >>>>> &first_word_mask, &mask, &data); >>>>> >>>>> for (j = 0; j < cnt; j++, mask++, data++, cur_offset += 4) { >>>>> @@ -2556,6 +2556,29 @@ nl_msg_put_flower_acts_release(struct ofpbuf >>>>> *request, uint16_t act_index) >>>>> nl_msg_end_nested(request, act_offset); >>>>> } >>>>> >>>>> +/* Aggregates all previous successive pedit actions csum_update_flags >>>>> + * to flower->csum_update_flags. Only append one csum action to the >>>>> + * last pedit action. */ >>>>> +static void >>>>> +nl_msg_put_csum_act(struct ofpbuf *request, struct tc_flower *flower, >>>>> + uint16_t *act_index) >>>>> +{ >>>>> + size_t act_offset; >>>>> + >>>>> + /* No pedit actions or processed already. */ >>>>> + if (!flower->csum_update_flags) { >>>>> + return; >>>>> + } >>>>> + >>>>> + act_offset = nl_msg_start_nested(request, (*act_index)++); >>>>> + nl_msg_put_act_csum(request, flower->csum_update_flags); >>>>> + nl_msg_put_act_flags(request); >>>>> + nl_msg_end_nested(request, act_offset); >>>>> + >>>>> + /* Clear it. So we can have another series of pedit actions. */ >>>>> + flower->csum_update_flags = 0; >>>>> +} >>>>> + >>>>> static int >>>>> nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower >>>>> *flower) >>>>> { >>>>> @@ -2572,21 +2595,18 @@ nl_msg_put_flower_acts(struct ofpbuf *request, >>>>> struct tc_flower *flower) >>>>> >>>>> action = flower->actions; >>>>> for (i = 0; i < flower->action_count; i++, action++) { >>>>> + if (action->type != TC_ACT_PEDIT) { >>>>> + nl_msg_put_csum_act(request, flower, &act_index); >>>>> + } >>>>> switch (action->type) { >>>>> case TC_ACT_PEDIT: { >>>>> act_offset = nl_msg_start_nested(request, act_index++); >>>>> - error = nl_msg_put_flower_rewrite_pedits(request, >>>>> flower); >>>>> + error = nl_msg_put_flower_rewrite_pedits(request, flower, >>>>> + action); >>>>> if (error) { >>>>> return error; >>>>> } >>>>> nl_msg_end_nested(request, act_offset); >>>>> - >>>>> - if (flower->csum_update_flags) { >>>>> - act_offset = nl_msg_start_nested(request, >>>>> act_index++); >>>>> - nl_msg_put_act_csum(request, >>>>> flower->csum_update_flags); >>>>> - nl_msg_put_act_flags(request); >>>>> - nl_msg_end_nested(request, act_offset); >>>>> - } >>>>> } >>>>> break; >>>>> case TC_ACT_ENCAP: { >>>>> diff --git a/lib/tc.h b/lib/tc.h >>>>> index a147ca461dc5..f08786b72528 100644 >>>>> --- a/lib/tc.h >>>>> +++ b/lib/tc.h >>>>> @@ -256,11 +256,23 @@ struct tc_action { >>>>> bool force; >>>>> bool commit; >>>>> } ct; >>>>> + >>>>> + struct { >>>>> + struct tc_flower_key key; >>>>> + struct tc_flower_key mask; >>>>> + } rewrite; >>>>> }; >>>>> >>>>> enum tc_action_type type; >>>>> }; >>>>> >>>>> +/* assert that if we overflow with a masked write of uint32_t to the >>>>> last byte >>>>> + * of action.rewrite we overflow inside struct tc_action. >>>>> + * shouldn't happen unless someone moves rewrite to the end of action */ >>>>> +BUILD_ASSERT_DECL(offsetof(struct tc_action, rewrite) >>>>> + + MEMBER_SIZEOF(struct tc_action, rewrite) >>>>> + + sizeof(uint32_t) - 2 < sizeof(struct tc_action)); >>>>> + >>>>> enum tc_offloaded_state { >>>>> TC_OFFLOADED_STATE_UNDEFINED, >>>>> TC_OFFLOADED_STATE_IN_HW, >>>>> @@ -333,12 +345,6 @@ struct tc_flower { >>>>> struct ovs_flow_stats stats; >>>>> uint64_t lastused; >>>>> >>>>> - struct { >>>>> - bool rewrite; >>>>> - struct tc_flower_key key; >>>>> - struct tc_flower_key mask; >>>>> - } rewrite; >>>>> - >>>>> uint32_t csum_update_flags; >>>>> >>>>> bool tunnel; >>>>> @@ -352,13 +358,6 @@ struct tc_flower { >>>>> enum tc_offload_policy tc_policy; >>>>> }; >>>>> >>>>> -/* assert that if we overflow with a masked write of uint32_t to the >>>>> last byte >>>>> - * of flower.rewrite we overflow inside struct flower. >>>>> - * shouldn't happen unless someone moves rewrite to the end of flower */ >>>>> -BUILD_ASSERT_DECL(offsetof(struct tc_flower, rewrite) >>>>> - + MEMBER_SIZEOF(struct tc_flower, rewrite) >>>>> - + sizeof(uint32_t) - 2 < sizeof(struct tc_flower)); >>>>> - >>>>> int tc_replace_flower(struct tcf_id *id, struct tc_flower *flower); >>>>> int tc_del_filter(struct tcf_id *id); >>>>> int tc_get_flower(struct tcf_id *id, struct tc_flower *flower); >>>>> -- >>>>> 2.8.0 >>>> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
