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