Re: [RFC net-next 1/1] seg6: add counters support for SRv6 Behaviors

[Andrea Mayer]

On Wed, 7 Apr 2021 16:55:41 -0600
David Ahern <dsahern@xxxxxxxxx> 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