Skip to content

[cmis] add read only cache decorator #562

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.

Sign up for GitHub

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

Merged
keboliu merged 15 commits into from
Jun 18, 2025
Merged

[cmis] add read only cache decorator #562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
dba1898
Add read only cache decorator
jianyuewu May 8, 2025
288d96b
Refactor code and update UT
jianyuewu May 8, 2025
3aa39dc
Remove unnecessary decorator
jianyuewu May 8, 2025
f89ac17
Add clear cache
jianyuewu May 8, 2025
0957256
Move cache api to utils dir
jianyuewu May 9, 2025
27a14d9
Refactor: relocate cache clearing to test helper
jianyuewu May 9, 2025
5d8a40d
Add utils sub package
jianyuewu May 9, 2025
4314b07
Support cache all non-empty collections
jianyuewu May 9, 2025
d13815d
Add clear cache when SFP module removal event happen
jianyuewu May 12, 2025
d24c082
[cmis] add cache enable flag
jianyuewu May 13, 2025
830f591
Add transceiver remove api
jianyuewu May 15, 2025
530c551
Move MEDIA_TYPE_FIELD lookup out of the loop
jianyuewu May 22, 2025
f64fa68
Set caching as default enabled
jianyuewu May 23, 2025
80eadea
Merge branch 'sonic-net:master' into cmis_cache
jianyuewu Jun 13, 2025
865d120
Add line back
jianyuewu Jun 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
'sonic_platform_base.sonic_xcvr.api.public',
'sonic_platform_base.sonic_xcvr.codes',
'sonic_platform_base.sonic_xcvr.codes.public',
'sonic_platform_base.sonic_xcvr.utils',
'sonic_platform_base.sonic_xcvr.api.credo',
'sonic_platform_base.sonic_xcvr.mem_maps.credo',
'sonic_platform_base.sonic_xcvr.codes.credo',
Expand Down
6 changes: 6 additions & 0 deletions sonic_platform_base/sfp_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,3 +479,9 @@ def get_xcvr_api(self):
if self._xcvr_api is None:
self.refresh_xcvr_api()
return self._xcvr_api

def remove_xcvr_api(self):
Copy link
Collaborator

@prgeor prgeor May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jianyuewu need to make sure the API object held by a thread still using it does not experience any issue

Copy link
Contributor Author

@jianyuewu jianyuewu May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junchao had tested it, it is safe, code is:

import random
import threading
import time
 
class SFP:
    def __init__(self, api):
        self.api = api
 
    def get_api(self):
        return self.api
 
    def reset(self):
        self.api = None
 
 
class Api:
    def __init__(self, value):
        self.value = value
 
 
g_api = Api(1000000)
sfp = SFP(g_api)
 
 
def thread_func():
    global sfp
    time.sleep(1)
    i = 0
    while True:
        api = sfp.get_api()
        if api is None:
            continue
        if i % 2 == 0:
            value = api.value
            sfp.reset() # set api object to None
            sfp.api = Api(value - 1) # re-create api object
            if sfp.api.value <= 0:
                break
        else:
            print(f'{threading.get_ident()}: {api.value}: {id(api)}')
        i += 1
 
 
def thread_func1():
    global sfp
    time.sleep(1)
    i = 0
    while True:
        api = sfp.get_api()
        if api is None:
            continue
        if i % 2 == 0:
            value = api.value
            sfp.reset() # set api object to None
            time.sleep(10)
            sfp.api = Api(value - 1) # re-create api object
            if sfp.api.value <= 0:
                break
        else:
            print(f'{threading.get_ident()}: {api.value}: {id(api)}')
        i += 1
 
def thread_func2():
    global sfp
    api = sfp.get_api()
    time.sleep(10)
    sfp.reset()
 
def thread_func3():
    global sfp
    api = sfp.get_api()
    time.sleep(10)
    for i in range(60):
        print(api.value)
 
 
t1 = threading.Thread(target=thread_func2)
t2 = threading.Thread(target=thread_func3)
 
t1.start()
t2.start()
t1.join()
t2.join()

Copy link
Collaborator

@prgeor prgeor May 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Junchao-Mellanox @jianyuewu please try this program. In your program you are not introducing race condition. Here is an example

 cat race.py
import random
import threading
import time

class SFP:
    def __init__(self, api):
        self.api = api

    def get_api(self):
        return self.api

    def reset(self):
        self.api = None


class Api:
    def __init__(self, value):
        self.value = value


g_api = Api(1000000)
sfp = SFP(g_api)


def thread_func1():
    global sfp
    i = 0
    while True:
        api = sfp.get_api()
        if api is None:
            break

        time.sleep(5)
        sfp.reset() # set api object to None

def thread_func2():
    global sfp
    i = 1

    while True:
        api = sfp.get_api()
        if api is None:
            break

        time.sleep(1)
        sfp.api.value = sfp.api.value + 1

t1 = threading.Thread(target=thread_func1)
t2 = threading.Thread(target=thread_func2)

t1.start()
t2.start()
t1.join()
t2.join()

Copy link
Collaborator

@prgeor prgeor May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its OK the thread to stop but not crash like below:-

prgeor@prgeor-dev-vm:~/scripts$ python3 race.py
Exception in thread Thread-2:
Traceback (most recent call last):
  File "/usr/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.8/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "race.py", line 46, in thread_func2
    sfp.api.value = sfp.api.value + 1
AttributeError: 'NoneType' object has no attribute 'value'
prgeor@prgeor-dev-vm:~/scripts$

Copy link
Contributor Author

@jianyuewu jianyuewu Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prgeor OK, there is some race condition, will consider that.

Copy link
Contributor Author

@jianyuewu jianyuewu Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Junchao-Mellanox @jianyuewu please try this program. In your program you are not introducing race condition. Here is an example

 cat race.py
import random
import threading
import time

class SFP:
    def __init__(self, api):
        self.api = api

    def get_api(self):
        return self.api

    def reset(self):
        self.api = None


class Api:
    def __init__(self, value):
        self.value = value


g_api = Api(1000000)
sfp = SFP(g_api)


def thread_func1():
    global sfp
    i = 0
    while True:
        api = sfp.get_api()
        if api is None:
            break

        time.sleep(5)
        sfp.reset() # set api object to None

def thread_func2():
    global sfp
    i = 1

    while True:
        api = sfp.get_api()
        if api is None:
            break

        time.sleep(1)
        sfp.api.value = sfp.api.value + 1

t1 = threading.Thread(target=thread_func1)
t2 = threading.Thread(target=thread_func2)

t1.start()
t2.start()
t1.join()
t2.join()

Hi @prgeor Looks sfp.api.value should be api.value, after test, there is no issue. Because we are using api in local, instead of global in official code.

diff race_test.py race.py
46c46
<         api.value = api.value + 1
---
>         sfp.api.value = sfp.api.value + 1
54c54

I also tried if use global sfp.api.value, then issue will be:

# python3 race_test.py
Exception in thread Thread-2 (thread_func2):
Traceback (most recent call last):
  File "/usr/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.11/threading.py", line 975, in run
    self._target(*self._args, **self._kwargs)
  File "//race_test.py", line 46, in thread_func2
    sfp.api.value = sfp.api.value + 1
                    ^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'value'

"""
Removes the cached XcvrApi so that the next get_xcvr_api() call will refresh it.
"""
self._xcvr_api = None
37 changes: 35 additions & 2 deletions sonic_platform_base/sonic_xcvr/api/public/cmis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import time
import copy
from collections import defaultdict
from ...utils.cache import read_only_cached_api_return

logger = logging.getLogger(__name__)
logger.addHandler(logging.NullHandler())
Expand Down Expand Up @@ -114,6 +115,16 @@ class CmisApi(XcvrApi):
LowPwrRequestSW = 4
LowPwrAllowRequestHW = 6

# Default caching enabled; control via classmethod
cache_enabled = True

@classmethod
def set_cache_enabled(cls, enabled: bool):
"""
Set the cache_enabled flag to control read_only_cached_api_return behavior.
"""
cls.cache_enabled = bool(enabled)

def __init__(self, xcvr_eeprom):
super(CmisApi, self).__init__(xcvr_eeprom)
self.vdm = CmisVdmApi(xcvr_eeprom) if not self.is_flat_memory() else None
Expand Down Expand Up @@ -184,12 +195,14 @@ def get_vdm_unfreeze_status(self):
'''
return self.xcvr_eeprom.read(consts.VDM_UNFREEZE_DONE)

@read_only_cached_api_return
def get_manufacturer(self):
'''
This function returns the manufacturer of the module
'''
return self.xcvr_eeprom.read(consts.VENDOR_NAME_FIELD)

@read_only_cached_api_return
def get_model(self):
'''
This function returns the part number of the module
Expand All @@ -202,42 +215,49 @@ def get_cable_length_type(self):
'''
return "Length Cable Assembly(m)"

@read_only_cached_api_return
def get_cable_length(self):
'''
This function returns the cable length of the module
'''
return self.xcvr_eeprom.read(consts.LENGTH_ASSEMBLY_FIELD)

@read_only_cached_api_return
def get_vendor_rev(self):
'''
This function returns the revision level for part number provided by vendor
'''
return self.xcvr_eeprom.read(consts.VENDOR_REV_FIELD)

@read_only_cached_api_return
def get_serial(self):
'''
This function returns the serial number of the module
'''
return self.xcvr_eeprom.read(consts.VENDOR_SERIAL_NO_FIELD)

@read_only_cached_api_return
def get_module_type(self):
'''
This function returns the SFF8024Identifier (module type / form-factor). Table 4-1 in SFF-8024 Rev4.6
'''
return self.xcvr_eeprom.read(consts.ID_FIELD)

@read_only_cached_api_return
def get_module_type_abbreviation(self):
'''
This function returns the SFF8024Identifier (module type / form-factor). Table 4-1 in SFF-8024 Rev4.6
'''
return self.xcvr_eeprom.read(consts.ID_ABBRV_FIELD)

@read_only_cached_api_return
def get_connector_type(self):
'''
This function returns module connector. Table 4-3 in SFF-8024 Rev4.6
'''
return self.xcvr_eeprom.read(consts.CONNECTOR_FIELD)

@read_only_cached_api_return
def get_module_hardware_revision(self):
'''
This function returns the module hardware revision
Expand All @@ -249,6 +269,7 @@ def get_module_hardware_revision(self):
hw_rev = [str(num) for num in [hw_major_rev, hw_minor_rev]]
return '.'.join(hw_rev)

@read_only_cached_api_return
def get_cmis_rev(self):
'''
This function returns the CMIS version the module complies to
Expand Down Expand Up @@ -571,6 +592,7 @@ def is_copper(self):
media_intf = self.get_module_media_type()
return media_intf == "passive_copper_media_interface" if media_intf else None

@read_only_cached_api_return
def is_flat_memory(self):
return self.xcvr_eeprom.read(consts.FLAT_MEM_FIELD) is not False

Expand Down Expand Up @@ -810,6 +832,7 @@ def get_tx_los(self):
tx_los_final.append(bool(tx_los[key]))
return tx_los_final

@read_only_cached_api_return
def get_tx_disable_support(self):
return not self.is_flat_memory() and self.xcvr_eeprom.read(consts.TX_DISABLE_SUPPORT_FIELD)

Expand Down Expand Up @@ -923,6 +946,7 @@ def set_power_override(self, power_override, power_set):
def get_transceiver_thresholds_support(self):
return not self.is_flat_memory()

@read_only_cached_api_return
def get_lpmode_support(self):
power_class = self.xcvr_eeprom.read(consts.POWER_CLASS_FIELD)
if power_class is None:
Expand Down Expand Up @@ -964,13 +988,15 @@ def get_module_media_interface(self):
else:
return 'Unknown media interface'

@read_only_cached_api_return
def is_coherent_module(self):
'''
Returns True if the module follow C-CMIS spec, False otherwise
'''
mintf = self.get_module_media_interface()
return False if 'ZR' not in mintf else True

@read_only_cached_api_return
def get_datapath_init_duration(self):
'''
This function returns the duration of datapath init
Expand All @@ -983,6 +1009,7 @@ def get_datapath_init_duration(self):
value = float(duration)
return value * DATAPATH_INIT_DURATION_MULTIPLIER if value <= DATAPATH_INIT_DURATION_OVERRIDE_THRESHOLD else value

@read_only_cached_api_return
def get_datapath_deinit_duration(self):
'''
This function returns the duration of datapath deinit
Expand All @@ -992,6 +1019,7 @@ def get_datapath_deinit_duration(self):
duration = self.xcvr_eeprom.read(consts.DP_PATH_DEINIT_DURATION)
return float(duration) if duration is not None else 0

@read_only_cached_api_return
def get_datapath_tx_turnon_duration(self):
'''
This function returns the duration of datapath tx turnon
Expand All @@ -1001,6 +1029,7 @@ def get_datapath_tx_turnon_duration(self):
duration = self.xcvr_eeprom.read(consts.DP_TX_TURNON_DURATION)
return float(duration) if duration is not None else 0

@read_only_cached_api_return
def get_datapath_tx_turnoff_duration(self):
'''
This function returns the duration of datapath tx turnoff
Expand All @@ -1010,6 +1039,7 @@ def get_datapath_tx_turnoff_duration(self):
duration = self.xcvr_eeprom.read(consts.DP_TX_TURNOFF_DURATION)
return float(duration) if duration is not None else 0

@read_only_cached_api_return
def get_module_pwr_up_duration(self):
'''
This function returns the duration of module power up
Expand All @@ -1019,6 +1049,7 @@ def get_module_pwr_up_duration(self):
duration = self.xcvr_eeprom.read(consts.MODULE_PWRUP_DURATION)
return float(duration) if duration is not None else 0

@read_only_cached_api_return
def get_module_pwr_down_duration(self):
'''
This function returns the duration of module power down
Expand Down Expand Up @@ -1647,7 +1678,6 @@ def get_module_level_flag(self):
'aux2_low_alarm_flag': aux2_low_alarm_flag,
'aux2_high_warn_flag': aux2_high_warn_flag,
'aux2_low_warn_flag': aux2_low_warn_flag}

aux1_high_alarm_flag = bool((module_flag_byte2 >> 0) & 0x1)
aux1_low_alarm_flag = bool((module_flag_byte2 >> 1) & 0x1)
aux1_high_warn_flag = bool((module_flag_byte2 >> 2) & 0x1)
Expand Down Expand Up @@ -2528,6 +2558,7 @@ def get_datapath_deinit(self):
return None
return [bool(datapath_deinit & (1 << lane)) for lane in range(self.NUM_CHANNELS)]

@read_only_cached_api_return
def get_application_advertisement(self):
"""
Get the application advertisement of the CMIS transceiver
Expand Down Expand Up @@ -2557,6 +2588,8 @@ def get_application_advertisement(self):
logger.error('Failed to read APPLS_ADVT_FIELD_PAGE01: ' + str(e))
return ret

media_type = self.xcvr_eeprom.read(consts.MEDIA_TYPE_FIELD)
prefix = map.get(media_type)
for app in range(1, 16):
buf = {}

Expand All @@ -2566,7 +2599,6 @@ def get_application_advertisement(self):
continue
buf['host_electrical_interface_id'] = val

prefix = map.get(self.xcvr_eeprom.read(consts.MEDIA_TYPE_FIELD))
if prefix is None:
continue
key = "{}_{}".format(prefix, app)
Expand Down Expand Up @@ -3033,3 +3065,4 @@ def get_error_description(self):
return 'OK'

# TODO: other XcvrApi methods

Empty file modified sonic_platform_base/sonic_xcvr/mem_maps/public/cmis.py
100644 → 100755
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions sonic_platform_base/sonic_xcvr/utils/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# utils package for transceiver common utilities
19 changes: 19 additions & 0 deletions sonic_platform_base/sonic_xcvr/utils/cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from collections import abc
import os

def read_only_cached_api_return(func):
"""Cache until func() returns a non-None, non-empty collections cache_value."""
cache_name = f'_{func.__name__}_cache'
def wrapper(self):
if not self.cache_enabled:
return func(self)
if not hasattr(self, cache_name):
cache_value = func(self)
setattr(self, cache_name, cache_value)
else:
cache_value = getattr(self, cache_name)
if cache_value is None or (isinstance(cache_value, abc.Iterable) and not cache_value):
cache_value = func(self)
setattr(self, cache_name, cache_value)
return cache_value
return wrapper
Loading
Loading