-
Notifications
You must be signed in to change notification settings - Fork 649
[buffermgrd]: Update PG profile on cable length change #4004
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
base: master
Are you sure you want to change the base?
[buffermgrd]: Update PG profile on cable length change #4004
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| m_cableLenLookup[port] = cable_length; | ||
| SWSS_LOG_INFO("Cable length set to %s for port %s", m_cableLenLookup[port].c_str(), port.c_str()); | ||
|
|
||
| if (cable_length != "None" && m_cableLenLookup[port] != cable_length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could change None --> "0m"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jithenderkondam could you update the PR with the comments addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kperumalbfn
During dynamic port breakout, the CABLE_LENGTH parameter is passed as None, which is why a check is implemented to handle this case.
For a cable length of "0m", the handling is done within the "doSpeedUpdateTask" function.
root@sonic:/home/admin# redis-cli -n 4 MONITOR | grep "CABLE_LENGTH"
1764201567.816362 [4 127.0.0.1:34584] "HGETALL" "CABLE_LENGTH|AZURE"
1764201567.897019 [4 127.0.0.1:34604] **"HSET" "CABLE_LENGTH|AZURE" "Ethernet0" "None"**
1764201567.898136 [4 unix:/var/run/redis/redis.sock] "HGETALL" "CABLE_LENGTH|AZURE"
1764201567.901065 [4 127.0.0.1:34604] "HGETALL" "CABLE_LENGTH|AZURE"
1764201567.901277 [4 127.0.0.1:34604] "HGETALL" "CABLE_LENGTH|AZURE"
1764201567.901579 [4 127.0.0.1:34604] "HSET" "CABLE_LENGTH|AZURE" "Ethernet8" "40m"
1764201567.901883 [4 unix:/var/run/redis/redis.sock] "HGETALL" "CABLE_LENGTH|AZURE"
1764201567.901939 [4 127.0.0.1:34604] "HDEL" "CABLE_LENGTH|AZURE" "Ethernet0"
1764201567.902200 [4 unix:/var/run/redis/redis.sock] "HGETALL" "CABLE_LENGTH|AZURE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jithenderkondam which module sets the cable-length as None during breakout?
| fvs_buffer_pg = self.config_db.get_entry("BUFFER_PG", pg_field_key) | ||
|
|
||
| # Check if fvs_buffer_pg profile is not equal to expected dynamic profile | ||
| expected_profile = "pg_lossless_{}_{}_profile".format(test_speed, test_cable_len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to buffer_profile name, could you also check xon, threshold and xon_offset values of the profile to confirm the profile is generated with correct values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kperumalbfn These values are dynamic and are determined based on either the "static headroom lookup" or "the dynamic headroom calculation" approach.
Given that, could you clarify what the comparison should involve—specifically, when reading the associated buffer_profile, what should these values be compared against?
| # Check if fvs_buffer_pg profile is not equal to expected dynamic profile | ||
| expected_profile = "pg_lossless_{}_{}_profile".format(test_speed, test_cable_len) | ||
| if fvs_buffer_pg.get("profile") != expected_profile: | ||
| assert False, "BUFFER_PG profile {} is not {}".format(fvs_buffer_pg.get("profile"), expected_profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls add one more test to revert the cable_length to "0m" to remove the pg_profile from the port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kperumalbfn , Could you confirm the expected behavior when the cable length is configured as "0m"?
From reviewing the code, it appears that the "doSpeedUpdateTask" function exits without performing any operations in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For cable-length 0m, there shouldn't be any PG profile attached.
b706286 to
2e9aabf
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| # Save original port speed and admin status | ||
| orig_fvs_port = self.config_db.get_entry("PORT", self.INTF) | ||
| orig_port_speed = orig_fvs_port.get("speed") if orig_fvs_port else None |
Check notice
Code scanning / CodeQL
Unused local variable
| # Save original port speed and admin status | ||
| orig_fvs_port = self.config_db.get_entry("PORT", self.INTF) | ||
| orig_port_speed = orig_fvs_port.get("speed") if orig_fvs_port else None | ||
| orig_port_status = orig_fvs_port.get("admin_status") if orig_fvs_port else None |
Check notice
Code scanning / CodeQL
Unused local variable
|
@kperumalbfn , would you review/approve? |
2e9aabf to
1fcdce1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1fcdce1 to
e6616ce
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Why I did it: When the cable length of a port changes, the Buffer PG profile isn't automatically updated. This causes a mismatch between the actual cable length and the buffer configuration, potentially leading to inefficient or incorrect buffer allocation. How I did it: The fix ensures that whenever a valid cable length is set, the buffer profile is recalculated and updated accordingly (by calling doSpeedUpdateTask). This maintains alignment between the hardware setup and buffer management. How to verify it: Can be verified by running below test test_buffer_traditional.py::TestBuffer::test_update_buffer_pg_for_cable_len_change Which release branch to backport (provide reason below if selected): 202505 Tested branch (Please provide the tested image version): 202505 Signed-off-by: Jithender Kondam <[email protected]>
e6616ce to
43d7e9b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Why I did it:
When the cable length of a port changes, the Buffer PG profile isn't automatically updated. This causes a mismatch between the actual cable length and the buffer configuration, potentially leading to inefficient or incorrect buffer allocation.
How I did it:
The fix ensures that whenever a valid cable length is set, the buffer profile is recalculated and updated accordingly (by calling doSpeedUpdateTask). This maintains alignment between the hardware setup and buffer management.
How to verify it:
Can be verified by running below test
test_buffer_traditional.py::TestBuffer::test_update_buffer_pg_for_cable_len_change
Which release branch to backport (provide reason below if selected):
Tested branch (Please provide the tested image version):