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