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