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 <[email protected]>
> > > > ---
> > > > lib/netdev-linux.c | 133
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > lib/netdev-linux.h | 6 ++
> > > > lib/tc.c | 21 +++++++
> > > > lib/tc.h | 6 ++
> > > > 4 files changed, 166 insertions(+)
> > > >
> > > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > > > index eb05153c0..ef6c7312f 100644
> > > > --- a/lib/netdev-linux.c
> > > > +++ b/lib/netdev-linux.c
> > > > @@ -5664,6 +5664,139 @@ tc_add_policer(struct netdev *netdev,
> > > > uint32_t kbits_rate,
> > > > return 0;
> > > > }
> > > >
> > > > +int
> > > > +tc_add_policer_action(uint32_t index, uint32_t kbits_rate,
> > > > + uint32_t kbits_burst, uint32_t
> > > > pkts_rate,
> > > > + uint32_t pkts_burst, bool update)
> > > > +{
> > > > + struct tc_police tc_police;
> > > > + struct ofpbuf request;
> > > > + struct tcamsg *tcamsg;
> > > > + size_t offset;
> > > > + int flags;
> > > > +
> > > > + tc_policer_init(&tc_police, kbits_rate, kbits_burst);
> > > > + tc_police.index = index;
> > > > +
> > > > + flags = (update ? NLM_F_REPLACE : NLM_F_EXCL) |
> > > > NLM_F_CREATE;
> > > > + tcamsg = tc_make_action_request(RTM_NEWACTION, flags,
> > > > &request);
> > > > + if (!tcamsg) {
> > > > + return ENODEV;
> > > > + }
> > > > +
> > > > + offset = nl_msg_start_nested(&request, TCA_ACT_TAB);
> > > > + nl_msg_put_act_police(&request, &tc_police, pkts_rate,
> > > > pkts_burst);
> > > > + nl_msg_end_nested(&request, offset);
> > > > +
> > > > + return tc_transact(&request, NULL);
> > > > +}
> > > > +
> > > > +static int
> > > > +tc_update_policer_action_stats(struct ofpbuf *msg,
> > > > + struct ofputil_meter_stats
> > > > *stats)
> > > > +{
> > > > + const struct nlattr *act = NULL;
> > > > + struct tc_flower flower;
> > > > + struct nlattr *prio;
> > > > + struct tcamsg *tca;
> > > > + int error;
> > > > +
> > >
> > > If would also do the if (!stats) return check here so none of the
> > > APIs will crash if messed up.
> > >
> > > > + if (NLMSG_HDRLEN + sizeof *tca > msg->size) {
> > > > + return EPROTO;
> > > > + }
> > > > +
> > > > + tca = ofpbuf_at_assert(msg, NLMSG_HDRLEN, sizeof *tca);
> > > > +
> > > > + act = nl_attr_find(msg, NLMSG_HDRLEN + sizeof *tca,
> > > > TCA_ACT_TAB);
> > > > + if (!act) {
> > > > + return EPROTO;
> > > > + }
> > > > +
> > > > + prio = (struct nlattr *) act + 1;
> > > > + memset(&flower, 0, sizeof(struct tc_flower));
> > > > + error = tc_parse_single_action(prio, &flower, false);
> > >
> > > I do not like this approach, we zero out a complex data structure
> > > pass into a general function, and hope it gives us a counter we
> > > need.
> > > I think we should separate out the statistics handling from
> > > nl_parse_single_action() and make it available to use here.
> > >
> > > > + if (!error) {
> > > > + stats->packet_in_count +=
> > > > + get_32aligned_u64(&flower.stats_sw.n_packets);
> > > > + stats->byte_in_count +=
> > > > get_32aligned_u64(&flower.stats_sw.n_bytes);
> > > > + stats->packet_in_count +=
> > > > + get_32aligned_u64(&flower.stats_hw.n_packets);
> > > > + stats->byte_in_count +=
> > > > get_32aligned_u64(&flower.stats_hw.n_bytes);
> > >
> > > What about the band stats on dropped packets? We need this to be
> > > in
> > > line with the kernel dp.
> > >
> >
> > It looks something wrong with tc police stats for the dropped
> > packets,
> > and someone is working on the kernel. Maybe we have to ignore this
> > issue now?
>
> I think this is very important for debugging. Maybe we can add the
> code to support this, so it will be there when it’s fixed in the
> kernel?
>
> > > > + }
> > > > +
> > > > + return error;
> > > > +}
> > > > +
> > > > +int
> > > > +tc_get_policer_action(uint32_t index, struct
> > > > ofputil_meter_stats
> > > > *stats)
> > > > +{
> > > > + struct ofpbuf *replyp = NULL;
> > > > + struct ofpbuf request;
> > > > + struct tcamsg *tcamsg;
> > > > + size_t root_offset;
> > > > + size_t prio_offset;
> > > > + int prio = 0;
> > > > + int error;
> > >
> > > If you do not add an “if !stats” check in
> > > tc_update_policer_action_stats(), I would add it here to avoid
> > > crashes with invalid API callback arguments.
> > >
> > > > + tcamsg = tc_make_action_request(RTM_GETACTION, 0,
> > > > &request);
> > > > + if (!tcamsg) {
> > > > + return ENODEV;
> > > > + }
> > > > +
> > > > + root_offset = nl_msg_start_nested(&request, TCA_ACT_TAB);
> > > > + prio_offset = nl_msg_start_nested(&request, ++prio);
> > >
> > > Why do we need the ++prio variable? Can we just not call it with
> > > 1?
> > >
> > > > + nl_msg_put_string(&request, TCA_ACT_KIND, "police");
> > > > + nl_msg_put_u32(&request, TCA_ACT_INDEX, index);
> > > > + nl_msg_end_nested(&request, prio_offset);
> > > > + nl_msg_end_nested(&request, root_offset);
> > > > +
> > > > + error = tc_transact(&request, &replyp);
> > > > + if (error) {
> > > > + VLOG_ERR_RL(&rl, "failed to dump police action (index:
> > > > %u), err=%d",
> > >
> > > Capital F for Failed.
> > >
> > > > + index, error);
> > > > + return error;
> > > > + }
> > > > +
> > > > + error = tc_update_policer_action_stats(replyp, stats);
> > > > + if (error) {
> > > > + VLOG_ERR_RL(&rl, "failed to update police stats
> > > > (index:
> > > > %u), err=%d",
> > > > + index, error);
> > >
> > > Capital F for Failed.
> > >
> > >
> > > Any reason for having log messages here, but not for add and
> > > delete?
> > >
> > > > + }
> > > > +
> > > > + return error;
> > > > +}
> > > > +
> > > > +int
> > > > +tc_del_policer_action(uint32_t index, struct
> > > > ofputil_meter_stats
> > > > *stats)
> > > > +{
> > > > + struct ofpbuf *replyp = NULL;
> > > > + struct ofpbuf request;
> > > > + struct tcamsg *tcamsg;
> > > > + size_t root_offset;
> > > > + size_t prio_offset;
> > > > + int prio = 0;
> > > > + int error;
> > > > +
> > > > + tcamsg = tc_make_action_request(RTM_DELACTION, NLM_F_ACK,
> > > > &request);
> > > > + if (!tcamsg) {
> > > > + return ENODEV;
> > > > + }
> > > > +
> > > > + root_offset = nl_msg_start_nested(&request, TCA_ACT_TAB);
> > > > + prio_offset = nl_msg_start_nested(&request, ++prio);
> > >
> > > See above on prio.
> > >
> > > > + nl_msg_put_string(&request, TCA_ACT_KIND, "police");
> > > > + nl_msg_put_u32(&request, TCA_ACT_INDEX, index);
> > > > + nl_msg_end_nested(&request, prio_offset);
> > > > + nl_msg_end_nested(&request, root_offset);
> > > > +
> > > > + error = tc_transact(&request, &replyp);
> > > > + if (!error && stats) {
> > > > + error = tc_update_policer_action_stats(replyp, stats);
> > > > + }
> > > If the null check for stats get added to
> > > tc_update_policer_action_stats() the code can be changed to:
> > >
> > > if (error) {
> > > return error;
> > > }
> > > return tc_update_policer_action_stats(replyp, stats);
> > >
> > > > +
> > > > + return error;
> > > > +}
> > > > +
> > > > static void
> > > > read_psched(void)
> > > > {
> > > > diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h
> > > > index e1e30f806..9a416ce50 100644
> > > > --- a/lib/netdev-linux.h
> > > > +++ b/lib/netdev-linux.h
> > > > @@ -19,6 +19,7 @@
> > > >
> > > > #include <stdint.h>
> > > > #include <stdbool.h>
> > > > +#include "openvswitch/ofp-meter.h"
> > > >
> > > > /* These functions are Linux specific, so they should be used
> > > > directly only by
> > > > * Linux-specific code. */
> > > > @@ -28,5 +29,10 @@ struct netdev;
> > > > int netdev_linux_ethtool_set_flag(struct netdev *netdev,
> > > > uint32_t
> > > > flag,
> > > > const char *flag_name, bool
> > > > enable);
> > > > int linux_get_ifindex(const char *netdev_name);
> > > > +int tc_add_policer_action(uint32_t index, uint32_t kbits_rate,
> > > > + uint32_t kbits_burst, uint32_t
> > > > pkts_rate,
> > > > + uint32_t pkts_burst, bool update);
> > > > +int tc_del_policer_action(uint32_t index, struct
> > > > ofputil_meter_stats *stats);
> > > > +int tc_get_policer_action(uint32_t index, struct
> > > > ofputil_meter_stats *stats);
> > > >
> > > > #endif /* netdev-linux.h */
> > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > index af7a7bc6d..ee16364ea 100644
> > > > --- a/lib/tc.c
> > > > +++ b/lib/tc.c
> > > > @@ -199,6 +199,20 @@ tc_make_request(int ifindex, int type,
> > > > unsigned int flags,
> > > > return tcmsg;
> > > > }
> > > >
> > > > +struct tcamsg *
> > > > +tc_make_action_request(int type, unsigned int flags,
> > > > + struct ofpbuf *request)
> > > > +{
> > > > + struct tcamsg *tcamsg;
> > > > +
> > > > + ofpbuf_init(request, 512);
> > > > + nl_msg_put_nlmsghdr(request, sizeof *tcamsg, type,
> > > > NLM_F_REQUEST | flags);
> > > > + tcamsg = ofpbuf_put_zeros(request, sizeof *tcamsg);
> > > > + tcamsg->tca_family = AF_UNSPEC;
> > > > +
> > > > + return tcamsg;
> > > > +}
> > > > +
> > > > static void request_from_tcf_id(struct tcf_id *id, uint16_t
> > > > eth_type,
> > > > int type, unsigned int flags,
> > > > struct ofpbuf *request)
> > > > @@ -1863,6 +1877,13 @@ nl_parse_single_action(struct nlattr
> > > > *action, struct tc_flower *flower,
> > > > return 0;
> > > > }
> > > >
> > > > +int
> > > > +tc_parse_single_action(struct nlattr *action, struct tc_flower
> > > > *flower,
> > > > + bool terse)
> > > > +{
> > > > + return nl_parse_single_action(action, flower, terse);
> > > > +}
> > > > +
> > >
> > > Why don’t we move all the functions in lib/netdev-linux.c above
> > > to
> > > tc.c where they belong? I know it’s a bit more work, but it
> > > avoids
> > > exposing internals like this.
> > >
> >
> > It's because tc_add_policer_action() calls tc_policer_init() and
> > nl_msg_put_act_police(), which uses much of code in netdev-linux.c.
> > And there also is a similar func tc_action_policer() in this file.
>
> I know it’s a mess right now, maybe you can do this as a follow-up
> patch, i.e. moving all TC to tc.c and only call a single function
> from this file to install the TC rule.
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
> > > > [email protected]
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
