On Mon, 2022-06-20 at 12:14 +0200, Eelco Chaudron wrote:
> On 27 May 2022, at 11:00, Jianbo Liu wrote:
>
> > Add helpers to add, delete and get stats of police action with
> > the specified index.
> >
> > Signed-off-by: Jianbo Liu <[email protected]>
> > ---
> > lib/netdev-linux.c | 142
> > +++++++++++++++++++++++++++++++++++++++++++++
> > lib/netdev-linux.h | 6 ++
> > lib/tc.c | 114 +++++++++++++++++++++++++-----------
> > lib/tc.h | 7 +++
> > 4 files changed, 235 insertions(+), 34 deletions(-)
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 4956e2fd1..94422cbc8 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -5667,6 +5667,148 @@ 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;
> > + int error;
> > +
> > + 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);
> > +
> > + error = tc_transact(&request, NULL);
> > + if (error) {
> > + VLOG_ERR_RL(&rl, "Failed to %s police action, err=%d",
> > + update ? "update" : "add", error);
> > + }
> > +
> > + return error;
> > +}
> > +
> > +static int
> > +tc_update_policer_action_stats(struct ofpbuf *msg,
> > + struct ofputil_meter_stats *stats)
> > +{
> > + struct ovs_flow_stats stats_dropped = {};
> > + struct ovs_flow_stats stats_hw = {};
> > + struct ovs_flow_stats stats_sw = {};
> > + const struct nlattr *act = NULL;
> > + struct nlattr *prio;
> > + struct tcamsg *tca;
> > + int error;
> > +
> > + if (!stats) {
> > + return 0;
> > + }
> > +
> > + if (NLMSG_HDRLEN + sizeof *tca > msg->size) {
> > + VLOG_ERR_RL(&rl, "Failed to get action stats, size
> > error");
> > + 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) {
> > + VLOG_ERR_RL(&rl, "Failed to get action stats, can't find
> > attr");
> > + return EPROTO;
> > + }
> > +
> > + prio = (struct nlattr *) act + 1;
> > + error = tc_parse_action_stats(prio, &stats_sw, &stats_hw,
> > &stats_dropped);
> > + if (!error) {
> > + stats->packet_in_count +=
> > + get_32aligned_u64(&stats_sw.n_packets);
> > + stats->byte_in_count +=
> > get_32aligned_u64(&stats_sw.n_bytes);
> > + stats->packet_in_count +=
> > + get_32aligned_u64(&stats_hw.n_packets);
> > + stats->byte_in_count +=
> > get_32aligned_u64(&stats_hw.n_bytes);
>
> We need to check here if band 0 is available.
> So:
> if (stats->n_bands >= 1) {
> stats->bands[0].packet_count +=
> get_32aligned_u64(&stats_dropped.n_packets);
> }
OK, I will add the checking.
>
> > + stats->bands[0].packet_count +=
> > + get_32aligned_u64(&stats_dropped.n_packets);
> > + }
> > +
> > + 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 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, 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",
> > + index, error);
> > + return error;
> > + }
> > +
> > + return tc_update_policer_action_stats(replyp, stats);
> > +}
> > +
> > +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 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, 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 delete police action (index:
> > %u), err=%d",
> > + index, error);
> > + return error;
> > + }
> > +
> > + return tc_update_policer_action_stats(replyp, stats);
> > +}
> > +
> > 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 5d96567e0..77d0c9de7 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)
> > @@ -1759,23 +1773,67 @@ static const struct nl_policy
> > stats_policy[] = {
> > [TCA_STATS_BASIC_HW] = { .type = NL_A_UNSPEC,
> > .min_len = sizeof(struct
> > gnet_stats_basic),
> > .optional = true, },
> > + [TCA_STATS_QUEUE] = { .type = NL_A_UNSPEC,
> > + .min_len = sizeof(struct
> > gnet_stats_queue),
> > + .optional = true, },
> > };
> >
> > +static int
> > +nl_parse_action_stats(struct nlattr *act_stats,
> > + struct ovs_flow_stats *stats_sw,
> > + struct ovs_flow_stats *stats_hw,
> > + struct ovs_flow_stats *stats_dropped)
> > +{
> > + struct gnet_stats_basic bs_sw = { .packets = 0, .bytes = 0, };
> > + struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
> > + const struct gnet_stats_basic *bs_all = NULL;
> > + const struct gnet_stats_basic *bs_hw = NULL;
> > + const struct gnet_stats_queue *qs;
>
> We need a null check for stats here.
>
> if (!stats) {
> return EINVAL;
> }
Are you talking about act_stats?
It should not be NULL, because [TCA_ACT_STATS] is not optional in
act_policy.
>
> > + if (!nl_parse_nested(act_stats, stats_policy, stats_attrs,
> > + ARRAY_SIZE(stats_policy))) {
> > + VLOG_ERR_RL(&error_rl, "failed to parse action stats
> > policy");
>
> Capital F for Failed here! I know you just moved the message but I
> think this is a good time to fix this.
>
> > + return EPROTO;
> > + }
> > +
> > + bs_all = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC],
> > sizeof *bs_all);
> > + if (stats_attrs[TCA_STATS_BASIC_HW]) {
> > + bs_hw =
> > nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW],
> > + sizeof *bs_hw);
> > +
> > + bs_sw.packets = bs_all->packets - bs_hw->packets;
> > + bs_sw.bytes = bs_all->bytes - bs_hw->bytes;
> > + } else {
> > + bs_sw.packets = bs_all->packets;
> > + bs_sw.bytes = bs_all->bytes;
> > + }
> > +
> > + if (bs_sw.packets > get_32aligned_u64(&stats_sw->n_packets)) {
> > + put_32aligned_u64(&stats_sw->n_packets, bs_sw.packets);
> > + put_32aligned_u64(&stats_sw->n_bytes, bs_sw.bytes);
> > + }
> > +
> > + if (bs_hw && bs_hw->packets > get_32aligned_u64(&stats_hw-
> > >n_packets)) {
> > + put_32aligned_u64(&stats_hw->n_packets, bs_hw->packets);
> > + put_32aligned_u64(&stats_hw->n_bytes, bs_hw->bytes);
> > + }
> > +
> > + if (stats_dropped && stats_attrs[TCA_STATS_QUEUE]) {
> > + qs = nl_attr_get_unspec(stats_attrs[TCA_STATS_QUEUE],
> > sizeof *qs);
> > + put_32aligned_u64(&stats_dropped->n_packets, qs->drops);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > nl_parse_single_action(struct nlattr *action, struct tc_flower
> > *flower,
> > bool terse)
> > {
> > struct nlattr *act_options;
> > - struct nlattr *act_stats;
> > struct nlattr *act_cookie;
> > const char *act_kind;
> > struct nlattr *action_attrs[ARRAY_SIZE(act_policy)];
> > - struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
> > - struct ovs_flow_stats *stats_sw = &flower->stats_sw;
> > - struct ovs_flow_stats *stats_hw = &flower->stats_hw;
> > - const struct gnet_stats_basic *bs_all = NULL;
> > - const struct gnet_stats_basic *bs_hw = NULL;
> > - struct gnet_stats_basic bs_sw = { .packets = 0, .bytes = 0, };
> > int err = 0;
> >
> > if (!nl_parse_nested(action, act_policy, action_attrs,
> > @@ -1825,37 +1883,25 @@ nl_parse_single_action(struct nlattr
> > *action, struct tc_flower *flower,
> > flower->act_cookie.len = nl_attr_get_size(act_cookie);
> > }
> >
> > - act_stats = action_attrs[TCA_ACT_STATS];
> > -
> > - if (!nl_parse_nested(act_stats, stats_policy, stats_attrs,
> > - ARRAY_SIZE(stats_policy))) {
> > - VLOG_ERR_RL(&error_rl, "failed to parse action stats
> > policy");
> > - return EPROTO;
> > - }
> > -
> > - bs_all = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC],
> > sizeof *bs_all);
> > - if (stats_attrs[TCA_STATS_BASIC_HW]) {
> > - bs_hw =
> > nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW],
> > - sizeof *bs_hw);
> > -
> > - bs_sw.packets = bs_all->packets - bs_hw->packets;
> > - bs_sw.bytes = bs_all->bytes - bs_hw->bytes;
> > - } else {
> > - bs_sw.packets = bs_all->packets;
> > - bs_sw.bytes = bs_all->bytes;
> > - }
> > + return nl_parse_action_stats(action_attrs[TCA_ACT_STATS],
> > + &flower->stats_sw, &flower-
> > >stats_hw, NULL);
> > +}
> >
> > - if (bs_sw.packets > get_32aligned_u64(&stats_sw->n_packets)) {
> > - put_32aligned_u64(&stats_sw->n_packets, bs_sw.packets);
> > - put_32aligned_u64(&stats_sw->n_bytes, bs_sw.bytes);
> > - }
> > +int
> > +tc_parse_action_stats(struct nlattr *action, struct ovs_flow_stats
> > *stats_sw,
> > + struct ovs_flow_stats *stats_hw,
> > + struct ovs_flow_stats *stats_dropped)
> > +{
> > + struct nlattr *action_attrs[ARRAY_SIZE(act_policy)];
> >
> > - if (bs_hw && bs_hw->packets > get_32aligned_u64(&stats_hw-
> > >n_packets)) {
> > - put_32aligned_u64(&stats_hw->n_packets, bs_hw->packets);
> > - put_32aligned_u64(&stats_hw->n_bytes, bs_hw->bytes);
> > + if (!nl_parse_nested(action, act_policy, action_attrs,
> > + ARRAY_SIZE(act_policy))) {
> > + VLOG_ERR_RL(&error_rl, "failed to parse single action
> > options");
>
> Capital F for Failed.
>
> > + return EPROTO;
> > }
> >
> > - return 0;
> > + return nl_parse_action_stats(action_attrs[TCA_ACT_STATS],
> > stats_sw,
> > + stats_hw, stats_dropped);
> > }
> >
> > #define TCA_ACT_MIN_PRIO 1
> > diff --git a/lib/tc.h b/lib/tc.h
> > index 751bec891..30cd2f7ff 100644
> > --- a/lib/tc.h
> > +++ b/lib/tc.h
> > @@ -23,6 +23,7 @@
> > #include <linux/pkt_cls.h>
> > #include <linux/pkt_sched.h>
> >
> > +#include "netlink.h"
> > #include "netlink-socket.h"
> > #include "odp-netlink.h"
> > #include "openvswitch/ofpbuf.h"
> > @@ -80,6 +81,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);
> > @@ -375,5 +378,9 @@ 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_action_stats(struct nlattr *action,
> > + struct ovs_flow_stats *stats_sw,
> > + struct ovs_flow_stats *stats_hw,
> > + struct ovs_flow_stats *stats_dropped);
> >
> > #endif /* tc.h */
> > --
> > 2.26.2
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
