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