SAI Proposal for Counter enhancement. #1941

rajkumar38 · 2024-01-07T07:50:41Z

Within the current framework of SAI, objects such as vlan, router-interface, tunnel, queue, port, and others come with built-in statistical enumeration. The expectation is that these statistics will be counted by default upon the creation of objects.
With this proposal,

User has flexibility to decide which object and what stat-ids should support counting.
Helps to optimally use the hardware counting resources.

kcudnik · 2024-01-16T11:53:25Z

/azp run

azure-pipelines · 2024-01-16T11:53:34Z

Azure Pipelines successfully started running 1 pipeline(s).

rlhui · 2024-01-25T16:39:02Z

@JaiOCP , @srikrishnagopu will review this.

rck-innovium · 2024-02-08T10:15:12Z

@JaiOCP , @srikrishnagopu will review this.

@JaiOCP @srikrishnagopu Can you please review as well as let us know about your preference among the two approaches. We can then do the required SAI .h file additions to the PR.

srikrishnagopu

Added few comments/questions.

Overall, Option 1 looks to be a better approach than Option 2. I will have @JaiOCP also weigh in.

srikrishnagopu · 2024-02-22T07:15:37Z

doc/SAI-Counter-enhancement-hld.md

+     /** Counting is disabled */
+    SAI_STATS_COUNT_MODE_NONE
+
+} sai_stats_count_mode_t;


Should we leverage sai_counter_stat_t and add these new enums to sai_counter_stat_t ? This enum is similar to sai_counter_stat_t.

We should not mix count mode with sai_counter_stat_t.

srikrishnagopu · 2024-02-22T07:18:17Z

doc/SAI-Counter-enhancement-hld.md

+     * @flags CREATE_AND_SET
+     * @default SAI_STATS_COUNT_MODE_PACKET_AND_BYTE
+     */
+    SAI_PORT_ATTR_STAT_ENABLED


Can we rename it to SAI_PORT_ATTR_STAT_MODE or SAI_PORT_ATTR_COUNTER_MODE ?

Stat mode SAI_STATS_COUNT_MODE_NONE should indicate that counter has been disabled.

Updated the doc with "SAI_PORT_ATTR_STAT_MODE " . Since Option 2 is suggested, this change will not be applicable.

srikrishnagopu · 2024-02-22T07:23:49Z

doc/SAI-Counter-enhancement-hld.md

+     * @default SAI_NULL_OBJECT_ID
+     */
+
+    SAI_ROUTER_INTERFACE_ATTR_IN_COUNTER,


Will this mode be any better if we define something

* @type sai_object_list_t * @flags CREATE_AND_SET * @objects SAI_OBJECT_TYPE_COUNTER * @allownull true * @default SAI_NULL_OBJECT_ID */

SAI_ROUTER_INTERFACE_ATTR_COUNTER_IDS

and enforce the list size as SAI_ROUTER_INTERFACE_STAT_MAX and indexing based on the stat ID ?

Below are the CONs of using list indexed with stat ID,

Need to add new stats enum (SAI_XXX_STAT_END)to identify the max number ( saivlan, sairouterinterface, saitunnel).

Enforce stats id enum to be contiguous.

How to handle existing disjoint sets in the stat-id enum. Adjusting existing enums to determine MAX_STAT index may not be backward compatible.
For example, in saiport.h, "SAI_PORT_STAT_END" is last enum and debug counters are part of stats enum list. How to arrive the max id in such scenario ? Can we move "SAI_PORT_STAT_END" before "SAI_PORT_STAT_IN_DROP_REASON_RANGE_BASE = 0x00001000". This will break backward compatibility.

Irrespective of supported enum set, application and SAI must manage the state for SAI_ROUTER_INTERFACE_STAT_MAX stat-ids.

JaiOCP · 2024-02-27T19:36:27Z

Quick comment regarding choice of Option 1 and Option 2.
Option 1:
Doesn't provide granularity of enabling and disabling at granular level. This approach also means that all the counters will be oneof [packet, packet/byte, byte, none]. This does not tend to usecase well as the graluarity of packet or byte count changes for a given counter type.
Last problem and probably the deal breaker for this approach is that it doesn't tend well to modern approach of flex counters, where counter as a generic pool can be allocated on a per type basis.

My input will be to go with Option 2. This is more work but is forward looking and works well with flex counter pool HW design.

rajkumar38 · 2024-03-17T08:00:14Z

Quick comment regarding choice of Option 1 and Option 2. Option 1: Doesn't provide granularity of enabling and disabling at granular level. This approach also means that all the counters will be oneof [packet, packet/byte, byte, none]. This does not tend to usecase well as the graluarity of packet or byte count changes for a given counter type. Last problem and probably the deal breaker for this approach is that it doesn't tend well to modern approach of flex counters, where counter as a generic pool can be allocated on a per type basis.

My input will be to go with Option 2. This is more work but is forward looking and works well with flex counter pool HW design.

Thanks @JaiOCP for the inputs. One of the challenge with Option 2 to minimize the number of attributes. To address this, We came up with option 3. Below is the highlight of option 3 and doc is updated with same. Pls review and let know for any comments.

Option 3 Introduce a new attribute value type holding list of stat_id to counter object.

saitypes.h

typedef struct 
{
      /** Object type of the stat enum*/
      sai_object_type_t  object_type;

      /** Stat enum value */
      sai_stat_id_t      stat_enum;

      /** Counter ObjectId associated with stat enum */
      sai_object_id_t    counter_id;

} sai_counter_id_t;

typedef struct _sai_counter_list_t
{
    /** Number of stats */
    uint32_t count;

    /** List of stat-id to counter object */
    sai_counter_id_t *list;

} sai_counter_list_t;


typedef union _sai_attribute_value_t
{
 

    /** @validonly meta->attrvaluetype == SAI_ATTR_VALUE_TYPE_STATID_COUNTER_LIST */
    sai_counter_list_t statidcounterlist;

} sai_attribute_value_t;

saimetadatypes.h

typedef enum _sai_attr_value_type_t
{

    /**
     * @brief Attribute value is STAT enum to COUNTER object list.
     */
    SAI_ATTR_VALUE_TYPE_STATID_COUNTER_LIST
  
} sai_attr_value_type_t;

Introduce an attribute for stat-id counter object list in each of the object file.

sairouterinterface.h Query and update the list only for supported stat enum

 * @brief Router interface counter objects
 * Counter Object list for supported router interface stats enum 
 * @type sai_counter_list_t
 * @flags CREATE_AND_SET
 * @allownull true
 * @default SAI_NULL_OBJECT_ID
 */
 SAI_ROUTER_INTERFACE_ATTR_COUNTER_IDS

kcudnik · 2024-03-17T09:32:31Z

there will be problem withh sai_counter_id_t, since it contains sai_object_id_t, which cant't be tracked for object dependency if used in list as attribute, this will not pass validation and sanity checks

kcudnik · 2024-03-18T15:30:08Z

doc/SAI-Counter-enhancement-hld.md

+      sai_stat_id_t      stat_enum;
+
+      /** Counter ObjectId associated with stat enum */
+      sai_object_id_t    counter_id;


this will be a problem when making it a struct there is no easy way to track dependencies on that, it would need to be an attribute

we can discuss this on SAI community meeting

maybe converting this entire structure to new object type and each field to separate attributee taht would solve the issue

Thanks for the inputs, @kcudnik . I have replaced option 3 and defined new attributes as part of saicounter.h to realize the functionality. Pls review and let know for any comments.

kcudnik · 2024-04-24T16:36:27Z

please fix build errors

inc/saicounter.h

inc/saiport.h

tjchadaga · 2024-05-29T05:48:23Z

@JaiOCP, @srikrishnagopu - could you please help complete the review on this?

JaiOCP

Final review going on with internal team.

JaiOCP · 2024-07-09T18:35:27Z

inc/saiport.h

+     * @default empty
+     */
+    SAI_PORT_ATTR_SELECTIVE_COUNTER_LIST,
+


We need to provide port object level control for enabling/disabling packet or bye count.
With the current proposal 1 port can have multiple stats object (of type selective) where each stats object can be enabled independently for only packet, only byte or both. This though provides a fine grain control but do not allow coarse control where all the stats object for a given port can be consistently enabled for either packet or byte or both

Thanks @JaiOCP for the review comments. As suggested, I have added new attribute to control counting of supported PKT/BYTE/Both stats at object level.
Also I have updated the HLD with work flow. Pls review option 3.
PR is updated with other SAI modules which can use this functionality.

JaiOCP · 2024-07-09T18:35:59Z

inc/saiqueue.h

+     * @default empty
+     */
+    SAI_QUEUE_ATTR_SELECTIVE_COUNTER_LIST,
+


Same comment as for port. Need a queue level control for all the stats object

JaiOCP · 2024-07-09T18:36:21Z

inc/sairouterinterface.h

+     * @default empty
+     */
+    SAI_ROUTER_INTERFACE_ATTR_SELECTIVE_COUNTER_LIST,
+


Same comment as for port/queue. Need a RIF level control

JaiOCP · 2024-07-09T18:36:33Z

inc/saitunnel.h

+     * @default empty
+     */
+    SAI_TUNNEL_ATTR_SELECTIVE_COUNTER_LIST,
+


Need a tunnel a level control

JaiOCP · 2024-07-09T18:36:44Z

inc/saivlan.h

+     * @default empty
+     */
+    SAI_VLAN_ATTR_SELECTIVE_COUNTER_LIST,
+


Need a vlan level control

kcudnik · 2024-07-19T07:32:37Z

Instead od adding same attributes for all objects would it be better to add global API for this ?

rajkumar38 · 2024-07-23T04:09:31Z

Instead od adding same attributes for all objects would it be better to add global API for this ?

Hi @kcudnik, If we have to manage this by global API, we may have to define attribute in saicounter.h to store the list of OIDs using a specific counter object. IMO this doesn't look correct. May be we can discuss this in next weekly meeting.

tjchadaga · 2024-08-20T19:34:42Z

@kcudnik - could you please help clarify the proposed change or help sign off on the current one?

kcudnik · 2024-08-23T18:30:27Z

Instead of adding 2 attributes to all objects add global API that will query those values, no need for definig which objects support that, if some will not support API will retire not supported

rajkumar38 · 2024-08-26T11:09:56Z

Instead of adding 2 attributes to all objects add global API that will query those values, no need for definig which objects support that, if some will not support API will retire not supported

This proposal is not about about query. Here the primary requirement is to let user select on which of the objects ( say router interfaces) or which stat-id's per object , they need statistics to be enabled/disabled.
Note that it is not at object type level but at per object instance (OID). NOS can enable/disable the counting and accordingly set these two attributes to pass the configuration to vendor SAI.
Without attributes defined, we are not able to visualize how to achieve this using global API.
If needed we discuss and go over the proposal again in the community meeting.

JaiOCP

LGTM

tjchadaga · 2024-09-06T00:09:34Z

Instead of adding 2 attributes to all objects add global API that will query those values, no need for definig which objects support that, if some will not support API will retire not supported

This proposal is not about about query. Here the primary requirement is to let user select on which of the objects ( say router interfaces) or which stat-id's per object , they need statistics to be enabled/disabled. Note that it is not at object type level but at per object instance (OID). NOS can enable/disable the counting and accordingly set these two attributes to pass the configuration to vendor SAI. Without attributes defined, we are not able to visualize how to achieve this using global API. If needed we discuss and go over the proposal again in the community meeting.

@kcudnik - Could you please help clarify further? This comment has been discussed a few times in the community meeting, but there does not seem to be a clear way of achieving this with global API.

kcudnik · 2024-10-02T10:04:26Z

Instead of adding 2 attributes to all objects add global API that will query those values, no need for definig which objects support that, if some will not support API will retire not supported

This proposal is not about about query. Here the primary requirement is to let user select on which of the objects ( say router interfaces) or which stat-id's per object , they need statistics to be enabled/disabled. Note that it is not at object type level but at per object instance (OID). NOS can enable/disable the counting and accordingly set these two attributes to pass the configuration to vendor SAI. Without attributes defined, we are not able to visualize how to achieve this using global API. If needed we discuss and go over the proposal again in the community meeting.

@kcudnik - Could you please help clarify further? This comment has been discussed a few times in the community meeting, but there does not seem to be a clear way of achieving this with global API.

please clarify which part you have trouble to understand, i think global API for this would be enough, also please resolve conflicts

Signed-off-by: Rajkumar P R <rpennadamram@marvell.com>

tjchadaga · 2024-10-04T22:23:04Z

@kcudnik - please help sign off on this, as discussed in the community meeting.

srikrishnagopu reviewed Feb 22, 2024

View reviewed changes

kcudnik requested changes Mar 18, 2024

View reviewed changes

rajkumar38 requested review from kcudnik and srikrishnagopu April 24, 2024 16:34

rajkumar38 force-pushed the master branch from 8cc5606 to 45087e9 Compare April 26, 2024 09:55

kcudnik reviewed Apr 26, 2024

View reviewed changes

inc/saicounter.h Show resolved Hide resolved

kcudnik reviewed Apr 26, 2024

View reviewed changes

inc/saiport.h Outdated Show resolved Hide resolved

kcudnik approved these changes May 4, 2024

View reviewed changes

JaiOCP reviewed Jul 3, 2024

View reviewed changes

JaiOCP reviewed Jul 9, 2024

View reviewed changes

rajkumar38 force-pushed the master branch from 1112414 to 1a12bc9 Compare July 18, 2024 08:31

rajkumar38 requested review from JaiOCP and kcudnik July 18, 2024 08:42

rajkumar38 force-pushed the master branch from 1a12bc9 to ebc7995 Compare July 18, 2024 09:22

rajkumar38 force-pushed the master branch from ebc7995 to ee74f89 Compare August 28, 2024 06:49

JaiOCP approved these changes Sep 5, 2024

View reviewed changes

rlhui added the reviewed PR is discussed in SAI Meeting label Sep 19, 2024

SAI Proposal for Counter enhancement.

d578cf0

Signed-off-by: Rajkumar P R <rpennadamram@marvell.com>

rajkumar38 force-pushed the master branch from ee74f89 to d578cf0 Compare October 3, 2024 09:35

kcudnik approved these changes Oct 5, 2024

View reviewed changes

tjchadaga merged commit f23185d into opencomputeproject:master Oct 7, 2024
3 checks passed

SAI Proposal for Counter enhancement. #1941

SAI Proposal for Counter enhancement. #1941

Conversation

rajkumar38 commented Jan 7, 2024 • edited Loading

kcudnik commented Jan 16, 2024

azure-pipelines bot commented Jan 16, 2024

rlhui commented Jan 25, 2024

rck-innovium commented Feb 8, 2024

srikrishnagopu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajkumar38 Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

JaiOCP commented Feb 27, 2024

rajkumar38 commented Mar 17, 2024

kcudnik commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcudnik commented Apr 24, 2024

tjchadaga commented May 29, 2024

JaiOCP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcudnik commented Jul 19, 2024

rajkumar38 commented Jul 23, 2024

tjchadaga commented Aug 20, 2024

kcudnik commented Aug 23, 2024

rajkumar38 commented Aug 26, 2024

JaiOCP left a comment

Choose a reason for hiding this comment

tjchadaga commented Sep 6, 2024

kcudnik commented Oct 2, 2024

tjchadaga commented Oct 4, 2024

rajkumar38 commented Jan 7, 2024 •

edited

Loading

rajkumar38 Mar 17, 2024 •

edited

Loading