From 8c56a710bd6e36f7b60258237b13a0c9e734e5b2 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sun, 25 Apr 2021 04:49:07 +0000 Subject: [PATCH 1/3] Optimize SFP modules initialization Originally, SFP modules were always accessed from platform daemons, and arbitrary SFP module can be accessed in the daemon. All SFP modules were initialized in one-shot once one of the following chassis APIs called - get_all_sfps - get_sfp_numbers - get_sfp Recently, we noticed that SFP modules can also be accessed from CLI, eg. the latest refactor of sfputil. In this case, only one SFP module is accessed in the chassis object's life cycle. To initialize all SFP modules in one-shot is waste of time. The new optimized flow is: - get_sfp called, only initialize the SFP module being accessed all other elements in _sfp_list are None - get_all_sfps and get_sfp_numbers called, initialize all SFP modules as usual Signed-off-by: Stephen Sun --- .../sonic_platform/chassis.py | 54 +++++++++++++------ 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py b/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py index 2ae93e14d4c..09a991e2db2 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py @@ -69,7 +69,8 @@ def __init__(self): # move the initialization of each components to their dedicated initializer # which will be called from platform - self.sfp_module_initialized = False + self.sfp_module_full_initialized = False + self.sfp_module_partial_initialized = False self.sfp_event_initialized = False self.reboot_cause_initialized = False self.sdk_handle = None @@ -115,7 +116,17 @@ def initialize_fan(self): self._fan_list.append(fan) - def initialize_sfp(self): + def initialize_single_sfp(self, index): + if not self._sfp_list[index]: + if index >= self.QSFP_PORT_START and index < self.PORTS_IN_BLOCK: + sfp_module = self.sfp_module(index, 'QSFP', self.get_sdk_handle, self.platform_name) + else: + sfp_module = self.sfp_module(index, 'SFP', self.get_sdk_handle, self.platform_name) + + self._sfp_list[index] = sfp_module + + + def initialize_sfp(self, index=None): from sonic_platform.sfp import SFP self.sfp_module = SFP @@ -127,15 +138,22 @@ def initialize_sfp(self): self.PORT_END = port_position_tuple[2] self.PORTS_IN_BLOCK = port_position_tuple[3] - for index in range(self.PORT_START, self.PORT_END + 1): - if index in range(self.QSFP_PORT_START, self.PORTS_IN_BLOCK + 1): - sfp_module = SFP(index, 'QSFP', self.get_sdk_handle, self.platform_name) - else: - sfp_module = SFP(index, 'SFP', self.get_sdk_handle, self.platform_name) - - self._sfp_list.append(sfp_module) + if index is not None: + if not self.sfp_module_partial_initialized: + if index >= self.PORT_START and index < self.PORT_END: + self._sfp_list = list([None]*(self.PORT_END + 1)) + else: + raise IndexError("{} is not a valid index of SPF modules. Valid index range:[{}, {}]".format( + index, self.PORT_START + 1, self.PORT_END + 1)) + self.sfp_module_partial_initialized = True + else: + if not self.sfp_module_partial_initialized: + self._sfp_list = list([None]*(self.PORT_END + 1)) + self.sfp_module_partial_initialized = True + for index in range(self.PORT_START, self.PORT_END + 1): + self.initialize_single_sfp(index) - self.sfp_module_initialized = True + self.sfp_module_full_initialized = True def get_sdk_handle(self): @@ -206,7 +224,7 @@ def get_num_sfps(self): Returns: An integer, the number of sfps available on this chassis """ - if not self.sfp_module_initialized: + if not self.sfp_module_full_initialized: self.initialize_sfp() return len(self._sfp_list) @@ -219,7 +237,7 @@ def get_all_sfps(self): A list of objects derived from SfpBase representing all sfps available on this chassis """ - if not self.sfp_module_initialized: + if not self.sfp_module_full_initialized: self.initialize_sfp() return self._sfp_list @@ -237,13 +255,17 @@ def get_sfp(self, index): Returns: An object dervied from SfpBase representing the specified sfp """ - if not self.sfp_module_initialized: - self.initialize_sfp() - sfp = None index -= 1 + try: + if not self.sfp_module_partial_initialized: + self.initialize_sfp(index) + sfp = self._sfp_list[index] + if not sfp: + self.initialize_single_sfp(index) + sfp = self._sfp_list[index] except IndexError: sys.stderr.write("SFP index {} out of range (0-{})\n".format( index, len(self._sfp_list)-1)) @@ -488,7 +510,7 @@ def reinit_sfps(self, port_dict): :return: """ # SFP not initialize yet, do nothing - if not self.sfp_module_initialized: + if not self.sfp_module_full_initialized: return from . import sfp From 98392088a9e80e366cbf579c235319b3f4b6f5d3 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 27 Apr 2021 10:03:56 +0000 Subject: [PATCH 2/3] Add comments Signed-off-by: Stephen Sun --- .../sonic_platform/chassis.py | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py b/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py index 09a991e2db2..b18d780faba 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py @@ -66,11 +66,38 @@ def __init__(self): # Initialize Platform name self.platform_name = device_info.get_platform() - + # move the initialization of each components to their dedicated initializer # which will be called from platform - self.sfp_module_full_initialized = False + # + # Multiple scenarios need to be taken into consideration regarding the SFP modules initialization. + # - Platform daemons + # - Can access multiple or all SFP modules + # - sfputil + # - Sometimes can access only one SFP module + # - Call get_sfp to get one SFP module object + # + # We should initialize all SFP modules only if it is necessary because initializing SFP module is time-consuming. + # This means, + # - If get_sfp is called, + # - If the _sfp_list isn't initialized, initialize it first. + # - Only the SFP module being required should be initialized. + # - If get_all_sfps is called, + # - If the _sfp_list isn't initialized, initialize it first. + # - All SFP modules need to be initialized. + # But the SFP modules that have already been initialized should not be initialized for the second time. + # This can caused by get_sfp being called before. + # + # Due to the complexity of SFP modules initialization, we have to introduce two initialized flags for SFP modules + # - sfp_module_partial_initialized: + # - False: The _sfp_list is [] (SFP stuff has never been accessed) + # - True: The _sfp_list is a list whose length is number of SFP modules supported by the platform + # - sfp_module_full_initialized: + # - False: All SFP modules have not been created + # - True: All SFP modules have been created + # self.sfp_module_partial_initialized = False + self.sfp_module_full_initialized = False self.sfp_event_initialized = False self.reboot_cause_initialized = False self.sdk_handle = None From f7c256560684e075c57bdc592f0ff8cb3534f092 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 28 Apr 2021 06:38:18 +0000 Subject: [PATCH 3/3] Add unit test cases Signed-off-by: Stephen Sun --- .../mlnx-platform-api/tests/test_sfp.py | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 platform/mellanox/mlnx-platform-api/tests/test_sfp.py diff --git a/platform/mellanox/mlnx-platform-api/tests/test_sfp.py b/platform/mellanox/mlnx-platform-api/tests/test_sfp.py new file mode 100644 index 00000000000..0c24eb83354 --- /dev/null +++ b/platform/mellanox/mlnx-platform-api/tests/test_sfp.py @@ -0,0 +1,84 @@ +import os +import sys +import pytest +from mock import MagicMock +from .mock_platform import MockFan + +test_path = os.path.dirname(os.path.abspath(__file__)) +modules_path = os.path.dirname(test_path) +sys.path.insert(0, modules_path) + +from sonic_py_common import device_info +from sonic_platform.sfp import SFP +from sonic_platform.chassis import Chassis + + +def mock_get_platform(): + return 'x86_64-mlnx_msn2410-r0' + + +def mock_read_eeprom_specific_bytes(self, offset, num_bytes): + return None + + +def mock_get_sdk_handle(self): + if not self.sdk_handle: + self.sdk_handle = 1 + return self.sdk_handle + +device_info.get_platform = mock_get_platform +SFP._read_eeprom_specific_bytes = mock_read_eeprom_specific_bytes +Chassis.get_sdk_handle = mock_get_sdk_handle + + +def test_sfp_partial_and_then_full_initialize(): + """ + Verify SFP initialization flow (partial and then full): + 1. get_sfp to tirgger a partial initialization + 2. get_sfp for another SPF module and verify the partial initialization isn't executed again + 3. get_all_sfps to trigger a full initialization + """ + chassis = Chassis() + + # Fetch a sfp + # This should trigger SFP modules be partial initialized + sfp1 = chassis.get_sfp(1) + # Verify the SFP list has been created + assert len(chassis._sfp_list) == chassis.PORT_END + 1 + assert chassis.sfp_module_partial_initialized == True + assert chassis.sfp_module_full_initialized == False + + # Fetch another SFP module + sfp2 = chassis.get_sfp(2) + # Verify the previous SFP module isn't changed + assert sfp1 == chassis.get_sfp(1) + + # Fetch all SFP modules + allsfp = chassis.get_all_sfps() + # Verify sfp1 and sfp2 aren't changed + assert sfp1 == chassis.get_sfp(1) + assert sfp2 == chassis.get_sfp(2) + # Verify the SFP has been fully initialized + assert chassis.sfp_module_partial_initialized == True + assert chassis.sfp_module_full_initialized == True + + +def test_sfp_full_initialize_without_partial(): + """ + Verify SFP initialization flow (full): + 1. get_all_sfps to trigger a full initialization + 2. get_sfp for a certain SFP module and verify the partial initialization isn't executed again + """ + chassis = Chassis() + + # Fetch all SFP modules + allsfp = chassis.get_all_sfps() + # Verify the SFP has been fully initialized + assert chassis.sfp_module_partial_initialized == True + assert chassis.sfp_module_full_initialized == True + for sfp in allsfp: + assert sfp is not None + + # Verify when get_sfp is called, the SFP modules won't be initialized again + sfp1 = allsfp[0] + assert sfp1 == chassis.get_sfp(1)