Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Td2: Reclaim buffer from unused ports #1830

Merged
merged 6 commits into from
Jul 26, 2021

Conversation

neethajohn
Copy link
Contributor

By default, lossless profiles are attached to PGs 3, 4 of all ports which results in a lot of buffer wastage when most of those ports are unused (not associated with a DEVICE_NEIGHBOR). In TD2, ingress pool size comprises of the reserved space as well. Hence making use of a special cable length of '0m' to identify unused ports and skip reserving space for those ports

What I did

  • In buffermgr, if port with cable len '0m' is identified, return immediately without creating pg lossless profile or attaching profile to the lossless pgs of that port
  • Listen to 'admin_status' update as well from the PORT table and update port-speed mapping. This is to handle add-rack scenario where a port is added later
    - The port starts of with cable length 0m
    - Configlet to add a port is applied. The order of operations specifc to the PORT/CABLE_LENGTH table are - port is initially set to admin down, cable length is updated for that port, port table attributes are defined and port is set to admin up
    - speed update might not be seen when the port is set to admin up. Hence port-speed mapping will capture the speed update whenever its seen and once the cable length is updated while the port is brought back up, profiles can be attached to the lossless pgs of the port

How I verified it

  • Manual tests done. Verified that no space is reserved for unused ports
  • Verified that when a port is added using 'add-rack' scenario, profile is attached to pgs of that port
  • New VS test added

Signed-off-by: Neetha John <nejo@microsoft.com>
…updates from PORT table

Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
cfgmgr/buffermgr.cpp Outdated Show resolved Hide resolved
Signed-off-by: Neetha John <nejo@microsoft.com>
@prsunny
Copy link
Collaborator

prsunny commented Jul 25, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sonic-net sonic-net deleted a comment from neethajohn Jul 25, 2021
@sonic-net sonic-net deleted a comment from azure-pipelines bot Jul 25, 2021
@neethajohn neethajohn merged commit 7f80f06 into sonic-net:master Jul 26, 2021
@neethajohn neethajohn deleted the td2_buf_reclaim branch July 26, 2021 17:42
qiluo-msft pushed a commit that referenced this pull request Jul 27, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

By default, lossless profiles are attached to PGs 3, 4 of all ports which results in a lot of buffer wastage when most of those ports are unused (not associated with a DEVICE_NEIGHBOR). In TD2, ingress pool size comprises of the reserved space as well. Hence making use of a special cable length of '0m' to identify unused ports and skip reserving space for those ports

What I did
* In buffermgr, if port with cable len '0m' is identified, return immediately without creating pg lossless profile or attaching profile to the lossless pgs of that port
* Listen to 'admin_status' update as well from the PORT table and update port-speed mapping. This is to handle add-rack scenario where a port is added later
   - The port starts of with cable length 0m
   - Configlet to add a port is applied. The order of operations specifc to the PORT/CABLE_LENGTH table are - port is initially set to admin down, cable length is updated for that port, port table attributes are defined and port is set to admin up
   - speed update might not be seen when the port is set to admin up. Hence port-speed mapping will capture the speed update whenever its seen and once the cable length is updated while the port is brought back up, profiles can be attached to the lossless pgs of the port

How I verified it
- Manual tests done. Verified that no space is reserved for unused ports
- Verified that when a port is added using 'add-rack' scenario, profile is attached to pgs of that port
- New VS test added
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 30, 2021
Update submodule for swss

f54b7d0b [Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net/sonic-swss#1820)
ac7f5cff Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830)
04105a4b [debugcounterorch] check if counter type is supported before querying (sonic-net/sonic-swss#1789)
a67d8af6 [202012][portsorch] fix errors when moving port from one lag to another. (sonic-net/sonic-swss#1819)
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 31, 2021
This PR includes the following commits

a67d8af [202012][portsorch] fix errors when moving port from one lag to another (sonic-net/sonic-swss#1819)
04105a4 [debugcounterorch] check if counter type is supported before querying (sonic-net/sonic-swss#1789)
ac7f5cff Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830)
f54b7d0 [Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for active ports without speed configured  (sonic-net/sonic-swss#1820)

Signed-off-by: Neetha John <nejo@microsoft.com>
neethajohn added a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 2, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 3, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
vaibhavhd added a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 3, 2021
Update sonic-swss submodule head to include below fixes:

Ignore ALREADY_EXIST error in FDB creation (sonic-net/sonic-swss#1815)
Update MACsec SA PN counter to support SAI API 1.8 (sonic-net/sonic-swss#1818)
[swss]: Allow portsyncd to run on system without ports (sonic-net/sonic-swss#1808)
[debugcounterorch] check if counter type is supported before querying… (sonic-net/sonic-swss#1789)
[configure.ac] Add the option of passing libnl path to configure script (sonic-net/sonic-swss#1824)
refactor(fdbsyncd): Convert files with dos2unix (sonic-net/sonic-swss#1828)
[VS] Fix for VS test failures (sonic-net/sonic-swss#1836)
Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830)
[gearbox] Set context for phys based on configs (sonic-net/sonic-swss#1826)
[Dynamic Buffer Calc] Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net/sonic-swss#1822)
Bridge mac setting, fix statedb time format (sonic-net/sonic-swss#1844)
[cfgmgr]: Introduce common libs. (sonic-net/sonic-swss#1842)
qiluo-msft added a commit to qiluo-msft/sonic-swss that referenced this pull request Aug 4, 2021
neethajohn added a commit to neethajohn/sonic-buildimage that referenced this pull request Aug 5, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
neethajohn added a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 6, 2021
* Update default cable len to 0m for TD2 (#8298)
* Update sonic-cfggen tests with the correct cable len

Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
- With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
- Cfggen tests passed with the cable len update
judyjoseph pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 7, 2021
* Update default cable len to 0m for TD2 (#8298)
* Update sonic-cfggen tests with the correct cable len

Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
- With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
- Cfggen tests passed with the cable len update
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 7, 2021
* Update default cable len to 0m for TD2 (#8298)
* Update sonic-cfggen tests with the correct cable len

Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
- With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
- Cfggen tests passed with the cable len update
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…t#8313)

Update sonic-swss submodule head to include below fixes:

Ignore ALREADY_EXIST error in FDB creation (sonic-net/sonic-swss#1815)
Update MACsec SA PN counter to support SAI API 1.8 (sonic-net/sonic-swss#1818)
[swss]: Allow portsyncd to run on system without ports (sonic-net/sonic-swss#1808)
[debugcounterorch] check if counter type is supported before querying… (sonic-net/sonic-swss#1789)
[configure.ac] Add the option of passing libnl path to configure script (sonic-net/sonic-swss#1824)
refactor(fdbsyncd): Convert files with dos2unix (sonic-net/sonic-swss#1828)
[VS] Fix for VS test failures (sonic-net/sonic-swss#1836)
Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830)
[gearbox] Set context for phys based on configs (sonic-net/sonic-swss#1826)
[Dynamic Buffer Calc] Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net/sonic-swss#1822)
Bridge mac setting, fix statedb time format (sonic-net/sonic-swss#1844)
[cfgmgr]: Introduce common libs. (sonic-net/sonic-swss#1842)
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
)

* Update default cable len to 0m for TD2 (sonic-net#8298)
* Update sonic-cfggen tests with the correct cable len

Signed-off-by: Neetha John <nejo@microsoft.com>

As part of the buffer reclamation efforts for TD2, setting the default cable len to 0m which means unused ports will have a cable len of 0m.

Why I did it
To align with the changes in sonic-net/sonic-swss#1830

How to verify it
- With the default cable len set to 0m and the associated changes in swss, CABLE_LENGTH table had '0m' set for unused ports and accordingly more space was reserved for the shared pool
- Cfggen tests passed with the cable len update
judyjoseph pushed a commit that referenced this pull request Aug 18, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

By default, lossless profiles are attached to PGs 3, 4 of all ports which results in a lot of buffer wastage when most of those ports are unused (not associated with a DEVICE_NEIGHBOR). In TD2, ingress pool size comprises of the reserved space as well. Hence making use of a special cable length of '0m' to identify unused ports and skip reserving space for those ports

What I did
* In buffermgr, if port with cable len '0m' is identified, return immediately without creating pg lossless profile or attaching profile to the lossless pgs of that port
* Listen to 'admin_status' update as well from the PORT table and update port-speed mapping. This is to handle add-rack scenario where a port is added later
   - The port starts of with cable length 0m
   - Configlet to add a port is applied. The order of operations specifc to the PORT/CABLE_LENGTH table are - port is initially set to admin down, cable length is updated for that port, port table attributes are defined and port is set to admin up
   - speed update might not be seen when the port is set to admin up. Hence port-speed mapping will capture the speed update whenever its seen and once the cable length is updated while the port is brought back up, profiles can be attached to the lossless pgs of the port

How I verified it
- Manual tests done. Verified that no space is reserved for unused ports
- Verified that when a port is added using 'add-rack' scenario, profile is attached to pgs of that port
- New VS test added
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
Signed-off-by: Neetha John <nejo@microsoft.com>

By default, lossless profiles are attached to PGs 3, 4 of all ports which results in a lot of buffer wastage when most of those ports are unused (not associated with a DEVICE_NEIGHBOR). In TD2, ingress pool size comprises of the reserved space as well. Hence making use of a special cable length of '0m' to identify unused ports and skip reserving space for those ports

What I did
* In buffermgr, if port with cable len '0m' is identified, return immediately without creating pg lossless profile or attaching profile to the lossless pgs of that port
* Listen to 'admin_status' update as well from the PORT table and update port-speed mapping. This is to handle add-rack scenario where a port is added later
   - The port starts of with cable length 0m
   - Configlet to add a port is applied. The order of operations specifc to the PORT/CABLE_LENGTH table are - port is initially set to admin down, cable length is updated for that port, port table attributes are defined and port is set to admin up
   - speed update might not be seen when the port is set to admin up. Hence port-speed mapping will capture the speed update whenever its seen and once the cable length is updated while the port is brought back up, profiles can be attached to the lossless pgs of the port

How I verified it
- Manual tests done. Verified that no space is reserved for unused ports
- Verified that when a port is added using 'add-rack' scenario, profile is attached to pgs of that port
- New VS test added
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
sonic-net#1831)

#### What I did
Fixing issue sonic-net#1830

#### How I did it
Problem we consume the function `loadData` from sonic-yang-mgmt pkg that always crops tables without YANG models from configdb json object. It does it as a side-effect and is not an expected outcome of the function.

The fix here is to crop the current/target tables before doing any sorting, this way gurantee we avoid this bug. I think the better soln is to fix sonic-yang-mgmt pkg. Will look into this more next week.

#### How to verify it
Run the command in the issue, it will result in the expected outcome

#### Previous command output (if the output of a command-line utility has changed)

#### New command output (if the output of a command-line utility has changed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants