Add packet trimming counter #2177

running-h · 2025-06-18T04:44:57Z

This PR adds the following:

SAI_PORT_STAT_DROPPED_TRIM_PACKETS
SAI_PORT_STAT_TX_TRIM_PACKETS
SAI_QUEUE_STAT_DROPPED_TRIM_PACKETS
SAI_QUEUE_STAT_TX_TRIM_PACKETS
SAI_SWITCH_STAT_DROPPED_TRIM_PACKETS
SAI_SWITCH_STAT_TX_TRIM_PACKETS

inc/saiswitch.h

kcudnik · 2025-06-20T10:15:56Z

inc/saiswitch.h

+    SAI_SWITCH_STAT_START,
+
+    /** Packets trimmed but dropped due to failed shared buffer admission on a new queue */
+    SAI_SWITCH_STAT_TRIM_DROPPED_PACKETS = SAI_SWITCH_STAT_START,


please follow concept on other ranges if that the range name

SAI_SWITCH_STAT_TRIM_DROPPED_PACKETS_RANGE_BASE = SAI_SWITCH_STAT_START ... SAI_SWITCH_STAT_TRIM_DROPPED_PACKETS_RANGE_END = 0x00000fff

or introduce other range, are you sure your stat type dont fit any existing range ?

the existing switch stats belong to clear group - in drop reasons (Debug counters), out drop reasons (debug counters) or fabric
While the trim stats are "regular" group
I don't see we keep regular group in other places like port or queue, nor do we group every stat to a group
in the future we may add other regular stats

r12f · 2025-06-24T18:09:38Z

hi @running-h , do you mind to rebase this PR and submit it again? The CI is somehow stuck.

r12f · 2025-06-24T18:13:43Z

inc/saiswitch.h

+    /** Switch stat range start */
+    SAI_SWITCH_STAT_START,
+
+    /** Packets trimmed but dropped due to failed shared buffer admission on a new queue */


Please update the comment to show this stat is a global-wise dropped trimmed packets for all original trim-enabled queues.

Please update the comment to show this stat is a global-wise dropped trimmed packets for all original trim-enabled queues.

we will change to "Global counter of packets trimmed but dropped due to failed shared buffer admission on a new queue for trimmed packets". OK ?

bandaru-viswanath · 2025-06-24T18:13:59Z

inc/saiport.h

@@ -3368,6 +3368,12 @@ typedef enum _sai_port_stat_t
    /** Packets trimmed due to failed shared buffer admission [uint64_t] */
    SAI_PORT_STAT_TRIM_PACKETS,

+    /** Packets trimmed but dropped due to failed shared buffer admission on a new queue for trimmed packets */


Is this "new queue" sam as the "trim-queue" ?

Is this "new queue" sam as the "trim-queue" ?

Distinguish between trimming-eligible queue (where trimming instead of tail-drop is done) and queue where trimmed packets from all trimming-eligible queues are sent (new queue)

bandaru-viswanath · 2025-06-24T18:16:51Z

inc/saiswitch.h

+    SAI_SWITCH_STAT_START,
+
+    /** Packets trimmed but dropped due to failed shared buffer admission on a new queue */
+    SAI_SWITCH_STAT_TRIM_DROPPED_PACKETS = SAI_SWITCH_STAT_START,


Can the new enums be inserted at the end, and have an appropriate (new-range) base, instead of adding at the top ?

i have already answered to kamil
The in/out drop reasons are in group to be able to support a bigger range of debug counters. There are 8 pre-defined in/out counters, but application can create more than that, so we need a range
whomever decided to add fabric, did it with a range
but i don't see a reason to do that, nor do we do it in all other objects we use counters, we are not grouping every counter subject into a range, and all regular counters are always at the beginning

bandaru-viswanath · 2025-06-24T18:18:30Z

inc/saiswitch.h

@@ -3599,6 +3605,9 @@ typedef enum _sai_switch_stat_t
    /** Switch stat fabric drop reasons range end */
    SAI_SWITCH_STAT_FABRIC_DROP_REASON_RANGE_END = 0x00003fff,

+    /** Switch stat range end */
+    SAI_SWITCH_STAT_END,


Along side PORT and QUEUE, why not introduce TRIM_SENT_PACKETS for SWITCH also ? Will be consistent across.

added. Thanks for suggestion

bandaru-viswanath · 2025-06-25T16:25:14Z

trim-queue meant the queue where all trimmed packets are sent. It is confusing for me with the "new queue" terminology. Thanks

…

On Wed, Jun 25, 2025 at 7:51 PM eddyk-nvidia ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In inc/saiport.h <#2177 (comment)> : > @@ -3368,6 +3368,12 @@ typedef enum _sai_port_stat_t /** Packets trimmed due to failed shared buffer admission [uint64_t] */ SAI_PORT_STAT_TRIM_PACKETS, + /** Packets trimmed but dropped due to failed shared buffer admission on a new queue for trimmed packets */ Is this "new queue" sam as the "trim-queue" ? Distinguish between trimming-eligible queue (where trimming instead of tail-drop is done) and queue where trimmed packets from all trimming-eligible queues are sent (new queue) — Reply to this email directly, view it on GitHub <#2177 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE7FUDTGZF656BHH5VLSMUD3FKV7PAVCNFSM6AAAAAB7R6HJLOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSNJYGM4TMNZQGI> . You are receiving this because you commented.Message ID: ***@***.***>

-- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

JaiOCP · 2025-06-26T15:04:59Z

inc/saiport.h

@@ -3368,6 +3368,12 @@ typedef enum _sai_port_stat_t
    /** Packets trimmed due to failed shared buffer admission [uint64_t] */
    SAI_PORT_STAT_TRIM_PACKETS,

+    /** Packets trimmed but dropped due to failed shared buffer admission on a new queue (for trimmed packets) */
+    SAI_PORT_STAT_TRIM_DROPPED_PACKETS,


We are following a fixed nomenclature like TRIM QUEUES, TRIM PACKET and so on.
Keeping consistent with it, please change the attr to
SAI_PORT_STAT_DROPPED_TRIM_PACKETS

JaiOCP · 2025-06-26T15:06:05Z

inc/saiport.h

+
+    /** Packets trimmed and successfully transmitted on port */
+    SAI_PORT_STAT_TRIM_SENT_PACKETS,
+


Same as above. Please change it to
SAI_PORT_STAT_TX_TRIM_PACKETS

@JaiOCP - done

JaiOCP · 2025-06-26T15:06:48Z

inc/saiqueue.h

@@ -433,6 +433,12 @@ typedef enum _sai_queue_stat_t
    /** Get watermark queue shared occupancy in cells [uint64_t] */
    SAI_QUEUE_STAT_SHARED_WATERMARK_CELLS = 0x0000002c,

+    /** Packets trimmed but failed to be admitted on a new queue (for trimmed packets) due to congestion. Counted on the original trimming-eligible queue [uint64_t] */
+    SAI_QUEUE_STAT_TRIM_DROPPED_PACKETS = 0x0000002d,


Please change it to
SAI_QUEUE_STAT_DROPPED_PACKETS

JaiOCP · 2025-06-26T15:07:16Z

inc/saiqueue.h

+    SAI_QUEUE_STAT_TRIM_DROPPED_PACKETS = 0x0000002d,
+
+    /** Packets trimmed and successfully transmitted on a new queue (for trimmed packets). Counted on the original trimming-eligible queue [uint64_t] */
+    SAI_QUEUE_STAT_TRIM_SENT_PACKETS = 0x0000002e,


Please change it to
SAI_QUEUE_STAT_TX_TRIM_PACKETS

JaiOCP · 2025-06-26T15:07:55Z

inc/saiswitch.h

+    SAI_SWITCH_STAT_START,
+
+    /** Global (switch-wise) counter of packets trimmed but dropped due to failed shared buffer admission on a new queue (for trimmed packets) */
+    SAI_SWITCH_STAT_TRIM_DROPPED_PACKETS = SAI_SWITCH_STAT_START,


Please change it to
SAI_SWITCH_STAT_DROPPED_TRIM_PACKETS

JaiOCP · 2025-06-26T15:08:14Z

inc/saiswitch.h

+    SAI_SWITCH_STAT_TRIM_DROPPED_PACKETS = SAI_SWITCH_STAT_START,
+
+    /** Global (switch-wise) counter of packets trimmed and successfully sent on a new queue (for trimmed packets) */
+    SAI_SWITCH_STAT_TRIM_SENT_PACKETS,


Please change it to
SAI_SWITCH_STAT_TX_TRIM_PACKETS

@JaiOCP - done. Jay, could you please approve the PR if everything is OK ? Thanks in advance

tjchadaga · 2025-06-26T15:22:42Z

/azp run

azure-pipelines · 2025-06-26T15:22:53Z

Azure Pipelines successfully started running 1 pipeline(s).

itaibaz · 2025-07-01T20:42:43Z

@kcudnik @r12f @bandaru-viswanath @JaiOCP
Can you please review the current version and approve or suggest if any further changes are needed ?

developfast

as comments

developfast · 2025-07-02T07:47:30Z

inc/saiqueue.h

+    SAI_QUEUE_STAT_DROPPED_TRIM_PACKETS = 0x0000002d,
+
+    /** Packets trimmed and successfully transmitted on a new queue (for trimmed packets). Counted on the original trimming-eligible queue [uint64_t] */
+    SAI_QUEUE_STAT_TX_TRIM_PACKETS = 0x0000002e,


The functional relationship between DROPPED_TRIM_PACKETS and TX_TRIM_PACKETS isn't clear. If TX_TRIM_PACKETS represents successfully transmitted trimmed packets, consider clarifying whether these are mutually exclusive counts or if there's overlap.

no overlap.
DROPPED_TRIM_PACKETS == trimmed & not sent
TX_TRIM_PACKETS == trimmed & sent

developfast · 2025-07-02T07:48:12Z

inc/saiswitch.h

+    SAI_SWITCH_STAT_DROPPED_TRIM_PACKETS = SAI_SWITCH_STAT_START,
+
+    /** Global (switch-wise) counter of packets trimmed and successfully sent on a new queue (for trimmed packets) */
+    SAI_SWITCH_STAT_TX_TRIM_PACKETS,


is this for all ports and therefore queues cumulatively? if so, please make this explicit in the comments

it is mentioned as "Global (switch-wise)"

developfast · 2025-07-02T07:50:20Z

inc/saiqueue.h

@@ -433,6 +433,12 @@ typedef enum _sai_queue_stat_t
    /** Get watermark queue shared occupancy in cells [uint64_t] */
    SAI_QUEUE_STAT_SHARED_WATERMARK_CELLS = 0x0000002c,

+    /** Packets trimmed but failed to be admitted on a new queue (for trimmed packets) due to congestion. Counted on the original trimming-eligible queue [uint64_t] */


The switch enum is the only one getting START/END markers. For consistency, please add similar markers to port and queue enums, if necessary. Moreover, what is the overflow behavior here?

rajkumar38 · 2025-07-02T15:57:59Z

@eddyk-nvidia , pls capture the changes in the document SAI-Proposal-Packet-Trimming.md

inc/saiport.h

kcudnik · 2025-07-03T11:47:31Z

/azp run

azure-pipelines · 2025-07-03T11:47:41Z

Azure Pipelines successfully started running 1 pipeline(s).

This PR adds the following: SAI_PORT_STAT_DROPPED_TRIM_PACKETS SAI_PORT_STAT_TX_TRIM_PACKETS SAI_QUEUE_STAT_DROPPED_TRIM_PACKETS SAI_QUEUE_STAT_TX_TRIM_PACKETS SAI_SWITCH_STAT_DROPPED_TRIM_PACKETS SAI_SWITCH_STAT_TX_TRIM_PACKETS Signed-off-by: Running Huang <[email protected]>

r12f · 2025-07-07T18:11:12Z

/azp run opencomputeproject.SAI

azure-pipelines · 2025-07-07T18:11:18Z

Commenter does not have sufficient privileges for PR 2177 in repo opencomputeproject/SAI

tjchadaga · 2025-07-07T18:18:04Z

/azp run

azure-pipelines · 2025-07-07T18:18:12Z

Azure Pipelines successfully started running 1 pipeline(s).

tjchadaga · 2025-07-07T18:19:54Z

@JaiOCP, @bandaru-viswanath - could you please help complete the review on this PR?

kcudnik requested changes Jun 18, 2025

View reviewed changes

inc/saiswitch.h Outdated Show resolved Hide resolved

running-h force-pushed the master branch 2 times, most recently from 68dd6ac to d5a3896 Compare June 19, 2025 07:00

kcudnik reviewed Jun 20, 2025

View reviewed changes

r12f reviewed Jun 24, 2025

View reviewed changes

bandaru-viswanath reviewed Jun 24, 2025

View reviewed changes

running-h force-pushed the master branch from 2f02846 to 9266ea9 Compare June 25, 2025 02:46

running-h force-pushed the master branch 2 times, most recently from 7e29369 to b8013b6 Compare June 26, 2025 07:51

JaiOCP reviewed Jun 26, 2025

View reviewed changes

tjchadaga requested a review from kcudnik June 26, 2025 15:30

tjchadaga added the reviewed PR is discussed in SAI Meeting label Jun 26, 2025

running-h force-pushed the master branch from b8013b6 to 61f22b1 Compare June 27, 2025 06:17

developfast reviewed Jul 2, 2025

View reviewed changes

kperumalbfn reviewed Jul 3, 2025

View reviewed changes

inc/saiport.h Outdated Show resolved Hide resolved

kcudnik approved these changes Jul 3, 2025

View reviewed changes

running-h force-pushed the master branch from 61f22b1 to 7e75864 Compare July 4, 2025 08:32

r12f approved these changes Jul 7, 2025

View reviewed changes

tjchadaga requested a review from JaiOCP July 7, 2025 18:19

kperumalbfn approved these changes Jul 7, 2025

View reviewed changes

JaiOCP approved these changes Jul 7, 2025

View reviewed changes

tjchadaga requested a review from bandaru-viswanath July 7, 2025 19:03

tjchadaga merged commit 0bed1a5 into opencomputeproject:master Jul 8, 2025
3 checks passed


		/** Packets trimmed and successfully transmitted on port */
		SAI_PORT_STAT_TRIM_SENT_PACKETS,

Add packet trimming counter #2177

Add packet trimming counter #2177

Conversation

running-h commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r12f commented Jun 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bandaru-viswanath Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bandaru-viswanath commented Jun 25, 2025 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjchadaga commented Jun 26, 2025

Uh oh!

azure-pipelines bot commented Jun 26, 2025

Uh oh!

itaibaz commented Jul 1, 2025

Uh oh!

developfast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajkumar38 commented Jul 2, 2025

Uh oh!

Uh oh!

kcudnik commented Jul 3, 2025

Uh oh!

azure-pipelines bot commented Jul 3, 2025

Uh oh!

r12f commented Jul 7, 2025

running-h commented Jun 18, 2025 •

edited

Loading

bandaru-viswanath Jun 24, 2025 •

edited

Loading