Adding PG7 check to DscpToPgMapping by ansrajpu-git · Pull Request #9859 · sonic-net/sonic-mgmt

ansrajpu-git · 2023-09-06T14:56:54Z

Description of PR

With the new SAI image & the fix as part of PR# sonic-net/sonic-buildimage#16254, the DscpToPgMapping qos test needs a check on the PG7 as well because, the control packets from CPU will now come to PG7 and may increase the packet count.

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

201911
202012
202205

Approach

Adding pg7 to verification along with pg0 & pg4 as, the control packets from CPU will now come to pg7 and may increase the packet count.

What is the motivation for this PR?

DscpToPgMapping failed in test run.

How did you do it?

How did you verify/test it?

Executed the qos test suite.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

neethajohn · 2023-09-06T19:34:50Z

tests/saitests/py3/sai_qos_tests.py

all control traffic is always mapped to q7. on which platform do you see lacp and bgp packets mapped to q0 and q4 respectively?

all control traffic is always mapped to q7. on which platform do you see lacp and bgp packets mapped to q0 and q4 respectively?

The LLDP and lacp packets received from peer will go to Q0 since it is untagged non ip packet. The BGP packets from peer are going to Q4 with the TOS value 0xC0.

Checked with @neethajohn. We need to check why the packets aren't coming on PG=7. all control packets should go to PG=7.

@neethajohn if you are refering to this PR #8097 and related fixes -- isn't not for the TX packets only ? i.e all the control packets being tx'ed should use queue 7.

Here @saksarav-nokia is taking about the queue used for packets RX, coming into the front panel ports. Is that also always queue 7 irrespective of the DSCP or other fields in the packet?

ansrajpu-git · 2023-09-21T15:46:51Z

@vmittal-msft ,Could you please approve this PR?

judyjoseph · 2023-09-26T18:39:27Z

@ansrajpu-git @saksarav-nokia Could you confirm the behavior for all control packets BGP/LLDP/LACP/Macsec etc ? thanks

saksarav-nokia · 2023-09-26T20:14:46Z

@judyjoseph , Are you talking about the Queues assigned for these control packets received on front panel port? OR CPU?. Could you elaborate more?

judyjoseph · 2023-09-26T23:24:48Z

@judyjoseph , Are you talking about the Queues assigned for these control packets received on front panel port? OR CPU?. Could you elaborate more?

I was referring to the earlier comment from Vineet -- that all the control packets should come in queue 7 irrespective of whether it is send out of CPU port or from front panel port. We can find out the behavior we observe and talk to broadcom if that is different from what was expected.

saksarav-nokia · 2023-09-28T14:45:07Z

@judyjoseph , As i mentioned in the chat, the control packets received on front panel ports are queued based on the .1p or dscp values based on the packet type. SInce BGP packets are ip packets, it goes to Queue 0 based on the DSCP value. Since LLDP and LACP packets are untagged L2 packets, they are going to Queue 0. If this behavior is different from XGS, we need to check with BCM.

judyjoseph · 2023-10-02T20:49:04Z

Following up with brcm via CS00012316603

rlhui · 2023-11-14T20:25:12Z

In this PR, you're modifying it for PG7, and not queue7 right? Please update the title if indeed PG7 is being checked here.

vmittal-msft · 2023-12-21T06:26:18Z

@ansrajpu-git @saksarav-nokia we may need to update this PR to accommodate extra packets coming in on PG3/4 as we are not planning to change copp_cfg file to force all control packets to go to PG7.

vmittal-msft · 2024-01-24T19:46:04Z

@ansrajpu-git, Since we don't plan to add any changes in this. lets close this.

saksarav-nokia · 2024-01-26T19:56:26Z

@ansrajpu-git, Since we don't plan to add any changes in this. lets close this.

@vmittal-msft , The control packets sent from CPU are assigned to PG 7 of the port to which the control packet is sent out. The reason for this is that when CPU sends the packet out, it is sent via KNET which adds the PTCH header for DNX asic to understand the source port and the source system port is the port to which the control packet is sent out. since we send the control packets to TC=7, the tx control packets will be counted against PG 7 of the front panel port. I am checking with BCM to see why it is designed this way. If they can't fix it, then we need this PR.

saksarav-nokia · 2024-01-30T14:14:07Z

@vmittal-msft , BCM confirmed in csp CS00012333616 that the control packets sent from CPU to the dest port will be mapped to the PG 7( PG mapped to sai_default_cpu_tx_tc=7) of the dest port since the packets sent from CPU are KNET using PTCH header. So we need to merge this PR.

judyjoseph · 2024-02-01T17:31:46Z

@saksarav-nokia @vmittal-msft we are seeing failures in the testrun without this fix -- so there are packets coming in PG7, planning to go ahead and merge this one. Let me know any thought

saksarav-nokia · 2024-02-01T17:34:16Z

@saksarav-nokia @vmittal-msft we are seeing failures in the testrun without this fix -- so there are packets coming in PG7, planning to go ahead and merge this one. Let me know any thought

@judyjoseph , We need this PR. Please see my previous comments and also the BCM csp i mentioned.

judyjoseph · 2024-02-06T01:39:14Z

tests/saitests/py3/sai_qos_tests.py

I see this code as a common sai qos test library - do we need to add a check for broadcom DNX ASIC ? else this will get applicable to all asic platforms

@judyjoseph , added the broadcom DNX ASIC check. Kindly review.

thanks. My original comment was to protect just this new change i.e i=7 alone with the check for asic == dnx. The other checks for i=0, i=4 was existing earlier, will leave it as such

judyjoseph · 2024-02-22T06:54:33Z

tests/saitests/py3/sai_qos_tests.py

The behavior where we see packets in PG 7 of the source port, due to TX packets from CPU sent out with TC =7 --- is Broadcom specific

Shouldn't the check be something like this -- i.e expect PG7 only if it is brcm-dnx ?
if i == 0 or i == 4 or (platform_asic and platform_asic == "broadcom-dnx" and i == 7):

vmittal-msft · 2024-02-23T20:08:38Z

tests/saitests/py3/sai_qos_tests.py

As discussed, lets try this logic -

i => 0 -> 7
pg => 0,3,4
dscps => 62,1,1

DNX/Chassis:
pg = 0 => Some extra packets with unmarked TC
pg = 4 => Extra packets for LACP/BGP packets
pg = 7 => packets from cpu to front panel ports

if non chassis:
if i == pg:
assert(pg_cntrs[pg] == pg_cntrs_base[pg] + len(dscps))
else:
assert(pg_cntrs[i] == pg_cntrs_base[i])
else
i = {3}
assert(pg_cntrs[pg] == pg_cntrs_base[pg] + len(dscps))
i = {0,4,7}
assert(pg_cntrs[pg] >= pg_cntrs_base[pg] + len(dscps))
else {1,2,5,6}
assert(pg_cntrs[i] == pg_cntrs_base[i])

vmittal-msft

Lets try commented logic to see if it works for chassis.

ansrajpu-git · 2024-02-27T15:28:25Z

Lets try commented logic to see if it works for chassis.

@vmittal-msft, as per the discussion and comments added a change for dnx chassis. Please review & comment.

vmittal-msft · 2024-02-28T20:12:21Z

tests/saitests/py3/sai_qos_tests.py

@ansrajpu-git Can you please keep non DNX check same ?

@vmittal-msft , As per the discussion and notes taken, I have implemented the changes.
the non-chassis behavior is reverted back to how it was originally implemented PR #7079

@vmittal-msft vmittal-msft 2 weeks ago
As discussed, lets try this logic -

i => 0 -> 7
pg => 0,3,4
dscps => 62,1,1

DNX/Chassis:
pg = 0 => Some extra packets with unmarked
pg = 4 => Extra packets for LACP/BGP packets
pg = 7 => packets from cpu to front panel ports

if non chassis:
if i == pg:
assert(pg_cntrs[pg] == pg_cntrs_base[pg] + len(dscps))
else:
assert(pg_cntrs[i] == pg_cntrs_base[i])
else
i = {3}
assert(pg_cntrs[pg] == pg_cntrs_base[pg] + len(dscps))
i = {0,4,7}
assert(pg_cntrs[pg] >= pg_cntrs_base[pg] + len(dscps))
else {1,2,5,6}
assert(pg_cntrs[i] == pg_cntrs_base[i])

Please confirm if more changes are required..

@ansrajpu-git i still see some difference from the logic we discussed. Can you please check ?

@vmittal-msft, updated the logic. Please check.

…oPgMapping" This reverts commit 16025cd8f8dab5281cdca33c00b37df963bfdb80.

vmittal-msft · 2024-03-20T00:21:26Z

@yxieca @wangxin please help merge.

ansrajpu-git · 2024-05-03T14:44:36Z

@vmittal-msft, please help this merge to 202205 branch.

* Qos newSAI change_adding queue7 to DscpToPGMapping * QoS_DscpToPgMapping_platform_asic check added

The issue is introduced by the PR:sonic-net#9859. This PR should be dedicated for broadcom-dnx, it should not change the logic for the remaining platform, so restore the original logic for non-broadcom-dnx.

The issue is introduced by the PR:#9859. This PR should be dedicated for broadcom-dnx, it should not change the logic for the remaining platform, so restore the original logic for non-broadcom-dnx.

The issue is introduced by the PR:sonic-net#9859. This PR should be dedicated for broadcom-dnx, it should not change the logic for the remaining platform, so restore the original logic for non-broadcom-dnx.

The issue is introduced by the PR:#9859. This PR should be dedicated for broadcom-dnx, it should not change the logic for the remaining platform, so restore the original logic for non-broadcom-dnx.

The issue is introduced by the PR:sonic-net#9859. This PR should be dedicated for broadcom-dnx, it should not change the logic for the remaining platform, so restore the original logic for non-broadcom-dnx.

rlhui added the P0 label Sep 6, 2023

rlhui assigned ansrajpu-git Sep 6, 2023

rlhui requested review from neethajohn and vmittal-msft September 6, 2023 16:14

neethajohn reviewed Sep 6, 2023

View reviewed changes

vmittal-msft added the Request for 202205 branch label Sep 13, 2023

ansrajpu-git changed the title ~~Adding queue7 check to DscpToPgMapping~~ Adding PG7 check to DscpToPgMapping Nov 15, 2023

rlhui removed the P0 label Dec 13, 2023

ansrajpu-git force-pushed the DscpToPG branch from 5c428e2 to f9ab164 Compare January 9, 2024 17:36

judyjoseph previously approved these changes Feb 1, 2024

View reviewed changes

judyjoseph reviewed Feb 6, 2024

View reviewed changes

ansrajpu-git dismissed judyjoseph’s stale review via f6d4314 February 14, 2024 22:35

judyjoseph reviewed Feb 22, 2024

View reviewed changes

vmittal-msft reviewed Feb 23, 2024

View reviewed changes

vmittal-msft reviewed Feb 28, 2024

View reviewed changes

judyjoseph previously approved these changes Mar 7, 2024

View reviewed changes

ansrajpu-git dismissed judyjoseph’s stale review via dc2f9d7 March 9, 2024 04:51

ansrajpu-git added 7 commits March 9, 2024 23:28

Qos newSAI change_adding queue7 to DscpToPGMapping

92522e5

Updating the condition to correct queue value_testQosSaiDscpToPgMapping

ad0b42c

Revert "Updating the condition to correct queue value_testQosSaiDscpT…

19db34b

…oPgMapping" This reverts commit 16025cd8f8dab5281cdca33c00b37df963bfdb80.

Updating DscpToPGmapping comments

3391187

QoS_DscpToPgMapping_platform_asic check added

5ba9082

Qos_DscpToPGMapping change for dnx platform

6f462b4

updating sai_qos for DscpToPG

08acfa0

ansrajpu-git force-pushed the DscpToPG branch from dc2f9d7 to 08acfa0 Compare March 10, 2024 04:43

vmittal-msft approved these changes Mar 20, 2024

View reviewed changes

judyjoseph approved these changes Mar 20, 2024

View reviewed changes

judyjoseph merged commit 7c59439 into sonic-net:master Mar 20, 2024

judyjoseph added Request for 202305 branch Request for 202311 branch labels Apr 25, 2024

ansrajpu-git added a commit to ansrajpu-git/sonic-mgmt that referenced this pull request May 9, 2024

Adding PG7 check to DscpToPgMapping (sonic-net#9859)

6c976e0

* Qos newSAI change_adding queue7 to DscpToPGMapping * QoS_DscpToPgMapping_platform_asic check added

ansrajpu-git mentioned this pull request May 9, 2024

[Cherry-pick] Adding PG7 check to DscpToPgMapping (#9859) #12786

Merged

8 tasks

yxieca pushed a commit that referenced this pull request May 15, 2024

Adding PG7 check to DscpToPgMapping (#9859) (#12786)

9f89f84

* Qos newSAI change_adding queue7 to DscpToPGMapping * QoS_DscpToPgMapping_platform_asic check added

JibinBao mentioned this pull request Nov 6, 2024

Fix testQosSaiDscpToPgMapping issue #15378

Merged

8 tasks

mssonicbld mentioned this pull request Nov 6, 2024

[action] [PR:15378] Fix testQosSaiDscpToPgMapping issue #15406

Merged

8 tasks

Conversation

ansrajpu-git commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

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!

ansrajpu-git commented Sep 21, 2023

Uh oh!

judyjoseph commented Sep 26, 2023

Uh oh!

saksarav-nokia commented Sep 26, 2023

Uh oh!

judyjoseph commented Sep 26, 2023

Uh oh!

saksarav-nokia commented Sep 28, 2023

Uh oh!

judyjoseph commented Oct 2, 2023

Uh oh!

rlhui commented Nov 14, 2023

Uh oh!

vmittal-msft commented Dec 21, 2023

Uh oh!

vmittal-msft commented Jan 24, 2024

Uh oh!

saksarav-nokia commented Jan 26, 2024

Uh oh!

saksarav-nokia commented Jan 30, 2024

Uh oh!

judyjoseph commented Feb 1, 2024

Uh oh!

saksarav-nokia commented Feb 1, 2024

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!

vmittal-msft left a comment

Choose a reason for hiding this comment

Uh oh!

ansrajpu-git commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmittal-msft Mar 7, 2024 • 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!

vmittal-msft commented Mar 20, 2024

Uh oh!

ansrajpu-git commented May 3, 2024

Uh oh!

Reviewers

Assignees

Labels

ansrajpu-git commented Sep 6, 2023 •

edited

Loading

vmittal-msft Mar 7, 2024 •

edited

Loading