On 24 May 2022, at 4:53, Jianbo Liu wrote:
> On Mon, 2022-05-23 at 12:12 +0200, Eelco Chaudron wrote: >> >> >> On 17 May 2022, at 14:54, Jianbo Liu wrote: >> >>> On Fri, 2022-05-13 at 16:55 +0200, Eelco Chaudron wrote: >>>> >>>> >>>> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: >>>> >>>>> Add helpers to add, delete and get stats of police action with >>>>> the specified index. >>>> >>>> See inline comments… This is the last patch for this week, I’ll >>>> continue the review sometime next week! >>>> >>>> >>>>> Signed-off-by: Jianbo Liu <[email protected]> >>>>> --- >>>>> lib/netdev-linux.c | 133 >>>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>>> lib/netdev-linux.h | 6 ++ >>>>> lib/tc.c | 21 +++++++ >>>>> lib/tc.h | 6 ++ >>>>> 4 files changed, 166 insertions(+) >>>>> >>>>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >>>>> index eb05153c0..ef6c7312f 100644 >>>>> --- a/lib/netdev-linux.c >>>>> +++ b/lib/netdev-linux.c >>>>> @@ -5664,6 +5664,139 @@ tc_add_policer(struct netdev *netdev, >>>>> uint32_t kbits_rate, >>>>> return 0; >>>>> } >>>>> >>>>> +int >>>>> +tc_add_policer_action(uint32_t index, uint32_t kbits_rate, >>>>> + uint32_t kbits_burst, uint32_t >>>>> pkts_rate, >>>>> + uint32_t pkts_burst, bool update) >>>>> +{ >>>>> + struct tc_police tc_police; >>>>> + struct ofpbuf request; >>>>> + struct tcamsg *tcamsg; >>>>> + size_t offset; >>>>> + int flags; >>>>> + >>>>> + tc_policer_init(&tc_police, kbits_rate, kbits_burst); >>>>> + tc_police.index = index; >>>>> + >>>>> + flags = (update ? NLM_F_REPLACE : NLM_F_EXCL) | >>>>> NLM_F_CREATE; >>>>> + tcamsg = tc_make_action_request(RTM_NEWACTION, flags, >>>>> &request); >>>>> + if (!tcamsg) { >>>>> + return ENODEV; >>>>> + } >>>>> + >>>>> + offset = nl_msg_start_nested(&request, TCA_ACT_TAB); >>>>> + nl_msg_put_act_police(&request, &tc_police, pkts_rate, >>>>> pkts_burst); >>>>> + nl_msg_end_nested(&request, offset); >>>>> + >>>>> + return tc_transact(&request, NULL); >>>>> +} >>>>> + >>>>> +static int >>>>> +tc_update_policer_action_stats(struct ofpbuf *msg, >>>>> + struct ofputil_meter_stats >>>>> *stats) >>>>> +{ >>>>> + const struct nlattr *act = NULL; >>>>> + struct tc_flower flower; >>>>> + struct nlattr *prio; >>>>> + struct tcamsg *tca; >>>>> + int error; >>>>> + >>>> >>>> If would also do the if (!stats) return check here so none of the >>>> APIs will crash if messed up. >>>> >>>>> + if (NLMSG_HDRLEN + sizeof *tca > msg->size) { >>>>> + return EPROTO; >>>>> + } >>>>> + >>>>> + tca = ofpbuf_at_assert(msg, NLMSG_HDRLEN, sizeof *tca); >>>>> + >>>>> + act = nl_attr_find(msg, NLMSG_HDRLEN + sizeof *tca, >>>>> TCA_ACT_TAB); >>>>> + if (!act) { >>>>> + return EPROTO; >>>>> + } >>>>> + >>>>> + prio = (struct nlattr *) act + 1; >>>>> + memset(&flower, 0, sizeof(struct tc_flower)); >>>>> + error = tc_parse_single_action(prio, &flower, false); >>>> >>>> I do not like this approach, we zero out a complex data structure >>>> pass into a general function, and hope it gives us a counter we >>>> need. >>>> I think we should separate out the statistics handling from >>>> nl_parse_single_action() and make it available to use here. >>>> >>>>> + if (!error) { >>>>> + stats->packet_in_count += >>>>> + get_32aligned_u64(&flower.stats_sw.n_packets); >>>>> + stats->byte_in_count += >>>>> get_32aligned_u64(&flower.stats_sw.n_bytes); >>>>> + stats->packet_in_count += >>>>> + get_32aligned_u64(&flower.stats_hw.n_packets); >>>>> + stats->byte_in_count += >>>>> get_32aligned_u64(&flower.stats_hw.n_bytes); >>>> >>>> What about the band stats on dropped packets? We need this to be >>>> in >>>> line with the kernel dp. >>>> >>> >>> It looks something wrong with tc police stats for the dropped >>> packets, >>> and someone is working on the kernel. Maybe we have to ignore this >>> issue now? >> >> I think this is very important for debugging. Maybe we can add the >> code to support this, so it will be there when it’s fixed in the >> kernel? >> >>>>> + } >>>>> + >>>>> + return error; >>>>> +} >>>>> + >>>>> +int >>>>> +tc_get_policer_action(uint32_t index, struct >>>>> ofputil_meter_stats >>>>> *stats) >>>>> +{ >>>>> + struct ofpbuf *replyp = NULL; >>>>> + struct ofpbuf request; >>>>> + struct tcamsg *tcamsg; >>>>> + size_t root_offset; >>>>> + size_t prio_offset; >>>>> + int prio = 0; >>>>> + int error; >>>> >>>> If you do not add an “if !stats” check in >>>> tc_update_policer_action_stats(), I would add it here to avoid >>>> crashes with invalid API callback arguments. >>>> >>>>> + tcamsg = tc_make_action_request(RTM_GETACTION, 0, >>>>> &request); >>>>> + if (!tcamsg) { >>>>> + return ENODEV; >>>>> + } >>>>> + >>>>> + root_offset = nl_msg_start_nested(&request, TCA_ACT_TAB); >>>>> + prio_offset = nl_msg_start_nested(&request, ++prio); >>>> >>>> Why do we need the ++prio variable? Can we just not call it with >>>> 1? >>>> >>>>> + nl_msg_put_string(&request, TCA_ACT_KIND, "police"); >>>>> + nl_msg_put_u32(&request, TCA_ACT_INDEX, index); >>>>> + nl_msg_end_nested(&request, prio_offset); >>>>> + nl_msg_end_nested(&request, root_offset); >>>>> + >>>>> + error = tc_transact(&request, &replyp); >>>>> + if (error) { >>>>> + VLOG_ERR_RL(&rl, "failed to dump police action (index: >>>>> %u), err=%d", >>>> >>>> Capital F for Failed. >>>> >>>>> + index, error); >>>>> + return error; >>>>> + } >>>>> + >>>>> + error = tc_update_policer_action_stats(replyp, stats); >>>>> + if (error) { >>>>> + VLOG_ERR_RL(&rl, "failed to update police stats >>>>> (index: >>>>> %u), err=%d", >>>>> + index, error); >>>> >>>> Capital F for Failed. >>>> >>>> >>>> Any reason for having log messages here, but not for add and >>>> delete? >>>> >>>>> + } >>>>> + >>>>> + return error; >>>>> +} >>>>> + >>>>> +int >>>>> +tc_del_policer_action(uint32_t index, struct >>>>> ofputil_meter_stats >>>>> *stats) >>>>> +{ >>>>> + struct ofpbuf *replyp = NULL; >>>>> + struct ofpbuf request; >>>>> + struct tcamsg *tcamsg; >>>>> + size_t root_offset; >>>>> + size_t prio_offset; >>>>> + int prio = 0; >>>>> + int error; >>>>> + >>>>> + tcamsg = tc_make_action_request(RTM_DELACTION, NLM_F_ACK, >>>>> &request); >>>>> + if (!tcamsg) { >>>>> + return ENODEV; >>>>> + } >>>>> + >>>>> + root_offset = nl_msg_start_nested(&request, TCA_ACT_TAB); >>>>> + prio_offset = nl_msg_start_nested(&request, ++prio); >>>> >>>> See above on prio. >>>> >>>>> + nl_msg_put_string(&request, TCA_ACT_KIND, "police"); >>>>> + nl_msg_put_u32(&request, TCA_ACT_INDEX, index); >>>>> + nl_msg_end_nested(&request, prio_offset); >>>>> + nl_msg_end_nested(&request, root_offset); >>>>> + >>>>> + error = tc_transact(&request, &replyp); >>>>> + if (!error && stats) { >>>>> + error = tc_update_policer_action_stats(replyp, stats); >>>>> + } >>>> If the null check for stats get added to >>>> tc_update_policer_action_stats() the code can be changed to: >>>> >>>> if (error) { >>>> return error; >>>> } >>>> return tc_update_policer_action_stats(replyp, stats); >>>> >>>>> + >>>>> + return error; >>>>> +} >>>>> + >>>>> static void >>>>> read_psched(void) >>>>> { >>>>> diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h >>>>> index e1e30f806..9a416ce50 100644 >>>>> --- a/lib/netdev-linux.h >>>>> +++ b/lib/netdev-linux.h >>>>> @@ -19,6 +19,7 @@ >>>>> >>>>> #include <stdint.h> >>>>> #include <stdbool.h> >>>>> +#include "openvswitch/ofp-meter.h" >>>>> >>>>> /* These functions are Linux specific, so they should be used >>>>> directly only by >>>>> * Linux-specific code. */ >>>>> @@ -28,5 +29,10 @@ struct netdev; >>>>> int netdev_linux_ethtool_set_flag(struct netdev *netdev, >>>>> uint32_t >>>>> flag, >>>>> const char *flag_name, bool >>>>> enable); >>>>> int linux_get_ifindex(const char *netdev_name); >>>>> +int tc_add_policer_action(uint32_t index, uint32_t kbits_rate, >>>>> + uint32_t kbits_burst, uint32_t >>>>> pkts_rate, >>>>> + uint32_t pkts_burst, bool update); >>>>> +int tc_del_policer_action(uint32_t index, struct >>>>> ofputil_meter_stats *stats); >>>>> +int tc_get_policer_action(uint32_t index, struct >>>>> ofputil_meter_stats *stats); >>>>> >>>>> #endif /* netdev-linux.h */ >>>>> diff --git a/lib/tc.c b/lib/tc.c >>>>> index af7a7bc6d..ee16364ea 100644 >>>>> --- a/lib/tc.c >>>>> +++ b/lib/tc.c >>>>> @@ -199,6 +199,20 @@ tc_make_request(int ifindex, int type, >>>>> unsigned int flags, >>>>> return tcmsg; >>>>> } >>>>> >>>>> +struct tcamsg * >>>>> +tc_make_action_request(int type, unsigned int flags, >>>>> + struct ofpbuf *request) >>>>> +{ >>>>> + struct tcamsg *tcamsg; >>>>> + >>>>> + ofpbuf_init(request, 512); >>>>> + nl_msg_put_nlmsghdr(request, sizeof *tcamsg, type, >>>>> NLM_F_REQUEST | flags); >>>>> + tcamsg = ofpbuf_put_zeros(request, sizeof *tcamsg); >>>>> + tcamsg->tca_family = AF_UNSPEC; >>>>> + >>>>> + return tcamsg; >>>>> +} >>>>> + >>>>> static void request_from_tcf_id(struct tcf_id *id, uint16_t >>>>> eth_type, >>>>> int type, unsigned int flags, >>>>> struct ofpbuf *request) >>>>> @@ -1863,6 +1877,13 @@ nl_parse_single_action(struct nlattr >>>>> *action, struct tc_flower *flower, >>>>> return 0; >>>>> } >>>>> >>>>> +int >>>>> +tc_parse_single_action(struct nlattr *action, struct tc_flower >>>>> *flower, >>>>> + bool terse) >>>>> +{ >>>>> + return nl_parse_single_action(action, flower, terse); >>>>> +} >>>>> + >>>> >>>> Why don’t we move all the functions in lib/netdev-linux.c above >>>> to >>>> tc.c where they belong? I know it’s a bit more work, but it >>>> avoids >>>> exposing internals like this. >>>> >>> >>> It's because tc_add_policer_action() calls tc_policer_init() and >>> nl_msg_put_act_police(), which uses much of code in netdev-linux.c. >>> And there also is a similar func tc_action_policer() in this file. >> >> I know it’s a mess right now, maybe you can do this as a follow-up >> patch, i.e. moving all TC to tc.c and only call a single function >> from this file to install the TC rule. > > It doesn't look like a small task. Can we do the refactoring later? Sorry, I was not clear, that was what I suggested, we do this in a follow-up patch later. >> >>>>> #define TCA_ACT_MIN_PRIO 1 >>>>> >>>>> static int >>>>> diff --git a/lib/tc.h b/lib/tc.h >>>>> index 201345672..96b9e6ccc 100644 >>>>> --- a/lib/tc.h >>>>> +++ b/lib/tc.h >>>>> @@ -80,6 +80,8 @@ tc_get_minor(unsigned int handle) >>>>> >>>>> struct tcmsg *tc_make_request(int ifindex, int type, >>>>> unsigned int flags, struct >>>>> ofpbuf >>>>> *); >>>>> +struct tcamsg *tc_make_action_request(int type, unsigned int >>>>> flags, >>>>> + struct ofpbuf *request); >>>>> int tc_transact(struct ofpbuf *request, struct ofpbuf >>>>> **replyp); >>>>> int tc_add_del_qdisc(int ifindex, bool add, uint32_t block_id, >>>>> enum tc_qdisc_hook hook); >>>>> @@ -365,6 +367,8 @@ struct tc_flower { >>>>> enum tc_offload_policy tc_policy; >>>>> }; >>>>> >>>>> +struct nlattr; >>>> >>>> Rather than this, would it make sense to include “netlink.h” at >>>> the >>>> top? But this is not needed if we move the function to tc.c >>>> >>>>> + >>>>> 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); >>>>> @@ -376,5 +380,7 @@ int parse_netlink_to_tc_flower(struct >>>>> ofpbuf >>>>> *reply, >>>>> bool terse); >>>>> int parse_netlink_to_tc_chain(struct ofpbuf *reply, uint32_t >>>>> *chain); >>>>> void tc_set_policy(const char *policy); >>>>> +int tc_parse_single_action(struct nlattr *action, struct >>>>> tc_flower >>>>> *flower, >>>>> + bool terse); >>>>> >>>>> #endif /* tc.h */ >>>>> -- >>>>> 2.26.2 >>>>> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> [email protected] >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
