| Message ID | 20220503030836.28783-5-jianbol@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for ovs metering with tc offload | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <jianbol@nvidia.com> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <jianbol@nvidia.com> > > --- > > 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? > > + } > > + > > + 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. > > #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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <jianbol@nvidia.com> >>> --- >>> 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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
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 <jianbol@nvidia.com> > > > > --- > > > > 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? > > > > > #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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
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 <jianbol@nvidia.com> >>>>> --- >>>>> 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 >>>>> dev@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>
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 (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); + 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); + } + + 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; + + 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); + 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", + 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); + } + + 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); + 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); + } + + 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); +} + #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; + 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 */
Add helpers to add, delete and get stats of police action with the specified index. Signed-off-by: Jianbo Liu <jianbol@nvidia.com> --- lib/netdev-linux.c | 133 +++++++++++++++++++++++++++++++++++++++++++++ lib/netdev-linux.h | 6 ++ lib/tc.c | 21 +++++++ lib/tc.h | 6 ++ 4 files changed, 166 insertions(+)