SAI_MACSEC_SA_STAT_OCTETS_ENCRYPTED should be a SC counter as specified in 802.1ae statistics

[SarathBug]

https://github.com/opencomputeproject/SAI/blob/1eb35afdb2146baf40e6c2b8f2f8bfe99075eaee/inc/saimacsec.h#L844

The description says as below

/** * @brief The sum of this count over all Secure Associations of a Secure * Channel gives 802.1ae statistics outOctetsEncrypted for egress and * inOctetsDecrypted for ingress */

If any PHY or switch supports this counter at SC(compliant to 802.1ae statistics outOctetsEncrypted) this adds undue burden. SAI need to get the SC object for SA and then read the counters. Even if the PHY or switch does not support it, SAI could hide those details and read the counters from associated SAs. This also simplifies the SoNiC code.

[lguohan]

@dipankar-ba , comments?

[dipankar-ba]

Here are the reasons why this SAI counter deviates from the IEEE spec:

1. Per SA count is more granular than per SC count and can be useful for debugging byte loss at boundary of SA switchover.
2. Rolling the per SA count into per SC count would cause information loss for NOS.
3. There are currently 2 MACsec chips that support SAI and both support per SA count.

[SarathBug]

@dipankar-ba

Thanks for clarifying. Perhaps NOS can display per SA count as additional counter. per SC count attribute is needed for the chips which don't have support for per SA count.

per SC count attribute: Chips that have support for it will read it from HW counter per SA count attribute: Chips that don't have support will return 0

[dipankar-ba]

1. For existing chips with only per-SA counter, if the API exposes both per-SC and per-SA counter then the driver has to construct counter states to support both per-SA and per-SC reads from the NOS.
2. If a future chip that does not implement per-SA count, the driver can return the per-SC count in response to per-SA read from NOS on any of the active SA of the SC.

IMO, option-2 is less complex. Also it avoids churning current driver code.

[SarathBug]

There is an issue with reading SC counters when SA counters are asked for. If there is a SAK refresh between two SA counter reads, it will mess up the SC counter.

As per the standards SoNiC should read the SC counter and it should be up to SAI how these counters are implemented. Either read from SA or read from SC. It can lead to performance issues if SoNiC has to read SA counters and sum them up to present them as SC counter.

We are looking at two chips which have support per-SC count and do not have per-SA count support for OCTETS_ENCRYPTED.

[dipankar-ba]

Just to be clear, as per existing API, the NOS should sum up the sai_macsec_sa_stat_octets_encrypted for all SAs of an SC. For a chip that implements per SC counter for octets_encrypted, I am proposing the driver return that counter value in response to NOS reading sai_macsec_sa_stat_octets_encrypted for a single active SA for an SC. The thesis is since the driver is returning the per SC count for only for one SA, when the NOS sums over all SAs of an SC, it should get the correct value.

Given this implementation, I do not understand the issues described viz:

1. "There is an issue with reading SC counters when SA counters are asked for." Can you please explain the issue in the context described above?
2. "If there is a SAK refresh between two SA counter reads, it will mess up the SC counter." Again can you please explain in more detail how this happens with the proposed method?

Thanks for your patience - I really need to understand the issues fully.