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. >>> 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
