cmisVDM: cache the static VDM descriptor pages - #729
Merged
prgeor merged 6 commits intoAug 4, 2026
Conversation
Signed-off-by: Aditya Bhiday <aditya@nexthop.ai>
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
The fake read_raw in the descriptor-cache tests ignored the requested
size and always returned a full 128-byte page. get_vdm_page reads the
value and threshold fields individually (2 bytes and 8 bytes), then
struct.unpack's them, so the oversized buffer raised
struct.error: unpack requires a buffer of 2 bytes
failing all four descriptor-cache tests. Return exactly the requested
width instead. The cache assertions are unchanged.
Signed-off-by: aditya-nexthop <aditya@nexthop.ai>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
aditya-nexthop
marked this pull request as ready for review
July 31, 2026 22:26
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
prgeor
reviewed
Aug 2, 2026
prgeor
previously approved these changes
Aug 2, 2026
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Address review feedback: cache the VDM descriptor pages in a self._vdm_descriptor dict initialized in the constructor, instead of extending the shared read_only_cached_api_return helper to support arguments. Signed-off-by: aditya-nexthop <aditya@nexthop.ai>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
prgeor
reviewed
Aug 4, 2026
Address review feedback: there is no use case for not caching the VDM descriptor pages, so remove the cache_enabled gate from CmisVdmApi rather than defaulting it on. Signed-off-by: aditya-nexthop <aditya@nexthop.ai>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
prgeor
approved these changes
Aug 4, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes SONiC CMIS VDM handling by caching static VDM descriptor pages (0x20–0x23) inside CmisVdmApi, reducing repeated 128B EEPROM reads during DOM cycles, and updates the unit tests to validate the caching behavior.
Changes:
- Add instance-level caching for raw VDM descriptor pages in
CmisVdmApi.get_vdm_page()via a new helper method. - Refactor
test_cmisVDM.pyto create a freshCmisVdmApiper test to avoid cross-test cache contamination. - Add unit tests to validate descriptor-page caching behavior across multiple cycles.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sonic_platform_base/sonic_xcvr/api/public/cmisVDM.py |
Adds descriptor-page caching helper and uses it from get_vdm_page() to avoid repeated reads. |
tests/sonic_xcvr/test_cmisVDM.py |
Refactors test setup and adds tests covering descriptor-page caching behavior. |
Comment on lines
+127
to
+143
| def test_vdm_descriptor_page_cached(self): | ||
| api, reads = self._make_vdm_api_with_counting_reader() | ||
| desc_off = 0x20 * PAGE_SIZE + PAGE_OFFSET | ||
| val_off = 0x24 * PAGE_SIZE + PAGE_OFFSET | ||
| for _ in range(3): | ||
| api.get_vdm_page(0x20, None) | ||
| # descriptor read once and reused; value page re-read every cycle | ||
| assert reads[desc_off] == 1 | ||
| assert reads[val_off] == 3 | ||
|
|
||
| def test_vdm_descriptor_cache_is_per_page(self): | ||
| api, reads = self._make_vdm_api_with_counting_reader() | ||
| for page in (0x20, 0x21, 0x20, 0x21): | ||
| api.get_vdm_page(page, None) | ||
| assert reads[0x20 * PAGE_SIZE + PAGE_OFFSET] == 1 | ||
| assert reads[0x21 * PAGE_SIZE + PAGE_OFFSET] == 1 | ||
|
|
Comment on lines
+56
to
+59
| if not self._vdm_descriptor.get(page): | ||
| offset = page * PAGE_SIZE + PAGE_OFFSET | ||
| self._vdm_descriptor[page] = self.xcvr_eeprom.read_raw(offset, PAGE_SIZE) | ||
| return self._vdm_descriptor[page] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cache the static VDM descriptor pages (0x20-0x23) on the
CmisVdmApiobject soeach page is read from EEPROM once instead of on every DOM cycle.
CmisVdmApi._read_vdm_descriptor_page(page), which returns a rawdescriptor page and memoizes it in a
self._vdm_descriptordict keyed by page.get_vdm_page()now obtains the descriptor through this helper instead ofcalling
xcvr_eeprom.read_raw()directly.next cycle rather than leaving VDM unavailable for the life of the api object.
pages a module actually advertises are read.
Motivation and Context
The VDM descriptors -- observable Type ID, threshold-set ID and monitored-lane
assignment for each of the 64 slots in a page -- are a static, read-only module
advertisement in CMIS. They describe how the VDM value and threshold pages are
laid out and do not change while the module is powered and plugged in, yet
get_vdm_page()re-read them on every DOM cycle.Each descriptor page is a 128 B read and
get_vdm_allpage()walks every page themodule advertises (up to four, 0x20-0x23), so this removes up to 4 x 128 B of
redundant EEPROM traffic per port per DOM cycle.
The cache lives on the api object, which xcvrd recreates when a module is
re-inserted, so it needs no explicit invalidation.
How Has This Been Tested?
Full
pytestsuite: 1388 passed, no failures.New unit tests in
tests/sonic_xcvr/test_cmisVDM.py:test_vdm_descriptor_page_cached-- the descriptor page is read once acrossthree
get_vdm_page()cycles, while the value page is re-read each cycle.test_vdm_descriptor_cache_is_per_page-- pages 0x20 and 0x21 are cachedindependently.
test_vdm_descriptor_empty_read_not_cached-- an empty descriptor read isretried rather than cached.
TestVDMbuilds its api insetup_methodso each test starts with an emptydescriptor cache.
Additional Information (Optional)
Descriptor caching is unconditional. It is not gated by
CmisApi.set_cache_enabled(), which continues to control only theread_only_cached_api_returnfields incmis.py.