On Wed, 7 Apr 2021 16:55:41 -0600
David Ahern <dsah...@gmail.com> wrote:
> On 4/7/21 12:03 PM, Andrea Mayer wrote:
> > diff --git a/include/uapi/linux/seg6_local.h
> > b/include/uapi/linux/seg6_local.h
> > index 3b39ef1dbb46..ae5e3fd12b73 100644
> > --- a/include/uapi/linux/seg6_local.h
> > +++ b/include/uapi/linux/seg6_local.h
> > @@ -27,6 +27,7 @@ enum {
> > SEG6_LOCAL_OIF,
> > SEG6_LOCAL_BPF,
> > SEG6_LOCAL_VRFTABLE,
> > + SEG6_LOCAL_COUNTERS,
> > __SEG6_LOCAL_MAX,
> > };
> > #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1)
> > @@ -78,4 +79,11 @@ enum {
> >
> > #define SEG6_LOCAL_BPF_PROG_MAX (__SEG6_LOCAL_BPF_PROG_MAX - 1)
> >
> > +/* SRv6 Behavior counters */
> > +struct seg6_local_counters {
> > + __u64 rx_packets;
> > + __u64 rx_bytes;
> > + __u64 rx_errors;
> > +};
> > +
> > #endif
>
> It's highly likely that more stats would get added over time. It would
> be good to document that here for interested parties and then make sure
> iproute2 can handle different sized stats structs. e.g., commit support
> to your repo, then add a new one (e.g, rx_drops) and verify the
> combinations handle it. e.g., old kernel - new iproute2, new kernel -
> old iproute, old - old and new-new.
>
Hi David,
thanks for your review.
I totally agree with you: we may want to add other counters in the future, even
if they are not considered in RFC8986.
With that in mind, the shared struct seg6_local_counters is not the best way to
go if we want to add other counters (because it will be difficult to manage
different sized structures when considering different kernel/iproute2 versions).
To make it easier adding new counters, instead of sharing the struct
seg6_local_counters, I would use netlink nested attributes to exchange counters
individually. In this way, only recognized (nested) attributes can be processed
by both the kernel and iproute2.
For example:
enum {
SEG6_LOCAL_CNT_UNSPEC,
SEG6_LOCAL_CNT_PAD, /* padding for 64 bits values */
SEG6_LOCAL_CNT_RX_PACKETS,
SEG6_LOCAL_CNT_RX_BYTES,
SEG6_LOCAL_CNT_RX_ERRORS,
__SEG6_LOCAL_CNT_MAX,
};
#define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1)
updating the policy for SEG6_LOCAL_COUNTERS to NLA_NESTED.
Then, I create a new policy for counters which handles each supported
counter separately.
static const struct
nla_policy seg6_local_counters_policy[SEG6_LOCAL_CNT_MAX + 1] = {
[SEG6_LOCAL_CNT_RX_PACKETS] = { .type = NLA_U64 },
[SEG6_LOCAL_CNT_RX_BYTES] = { .type = NLA_U64 },
[SEG6_LOCAL_CNT_RX_ERRORS] = { .type = NLA_U64 },
};
At the end, I update the parse_nla_counters(), put_nla_counters(), etc
according to the changes, i.e:
- nla_parse_nested() in parse_nla_counters();
- nla_nest_{start/end}() and for each supported counter nla_put_u64_64bit()
in put_nla_counters().
On the iproute2 side, we have to update the code to reflect the changes
discussed above.
I plan to issue an RFC v2 in a few days.
Andrea
