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?
> > + }
> > +
> > + 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
