[nvidia] Skip SAI discovery on ports on fast-boot #1416

stepanblyschak · 2024-08-21T15:57:46Z

Given that modern systems have lots of ports, performing SAI discovery takes very long time, e.g. (8 sec) for 256 port system. This has a big impact of fast-boot downtime and the discovery itself is not required for Nvidia platform fast-boot.

Same applies to Nvidia fastfast-boot (aka warm-boot), yet needs to be tested separately.

Given that modern systems have lots of ports, performing SAI discovery takes very long time, e.g. 8 sec for 256 port system. This has a big impact of fast-boot downtime and the discovery itself is not required for Nvidia platform fast-boot. Same applies to Nvidia fastfast-boot (aka warm-boot), yet needs to be tested separately. Signed-off-by: Stepan Blyschak <[email protected]>

kcudnik

this will lead to inconsistency ASIC_DB vs what's on device, which will later on lead to crash

kcudnik · 2024-10-02T12:25:25Z

syncd/SaiSwitchInterface.h

@@ -89,7 +89,8 @@ namespace syncd

            virtual void onPostPortCreate(
                    _In_ sai_object_id_t port_rid,
-                    _In_ sai_object_id_t port_vid) = 0;
+                    _In_ sai_object_id_t port_vid,
+                    _In_ bool discoverPortObjects = true) = 0;


this is very strict to ports, if we decide later on to do something similar on other objects then this is not optimal solution

The function is meant to be used on ports. Considering current approach, I assume there will be onPostXCreate() functions for other object types. Then, if needed, they can accept a boolean flag in the same way. This is simple and gives required granularity.

kcudnik · 2024-10-02T12:26:13Z

syncd/Syncd.cpp

+#ifdef SKIP_SAI_PORT_DISCOVERY_ON_FAST_BOOT
+    const bool discoverPortObjectsInFastBoot = false;
+#else
+    const bool discoverPortObjectsInFastBoot = true;
+#endif


fast boot cak be initiated after code was compiled which then this check will be hardcoded

also there are no tests for testing this code

fast boot cak be initiated after code was compiled which then this check will be hardcoded

This was the intention. For Nvidia - skip discover on ports in fast boot. The runtime check for fast boot is done in the condition below.

this should be runtime check

@kcudnik What is the benefit of runtime check here? Syncd is compiled per platform and on Nvidia we do not want to run discovery. We know this at compile time.

stepanblyschak · 2024-11-26T14:23:04Z

this will lead to inconsistency ASIC_DB vs what's on device, which will later on lead to crash

@kcudnik Yes, current design leads to performance problems on devices with lots of ports (could be 512, 1024 and more - tens of thousands keys to insert to ASIC_DB on init).
Could you suggest a test to cause a crash?

kcudnik · 2024-11-27T10:17:42Z

this will lead to inconsistency ASIC_DB vs what's on device, which will later on lead to crash

@kcudnik Yes, current design leads to performance problems on devices with lots of ports (could be 512, 1024 and more - tens of thousands keys to insert to ASIC_DB on init). Could you suggest a test to cause a crash?

Discovery Is done only once at switch create, how long it takes in your case ? We need to have a full view of device since later on warm boo will try to delete objects which were not discovered at current stage it's hard to predict when cras will happen, but it will be 99% on warm boot

stepanblyschak · 2024-11-27T13:30:47Z

@kcudnik I got your point, please note that Nvidia does not use standard warm-reboot flow in syncd, instead uses fast-fast-boot mode which is quite similar to fast-boot. No comparison logic is involved on Nvidia, therefore in my testing I did not observe crashes even after doing consecutive fast-reboot and warm-reboot. Therefore, the change is limited to be Nvidia only.

I attach the log of discovery during fast-reboot. I measured 4.8 sec on 202405 dedicated to discovery which is 16% of time budget for fast-reboot. It is only a 256 port system and we expect the time to increase linearly with the number of ports which is growing with new HWSKUs.

logs.txt

kcudnik · 2024-11-27T14:17:29Z

Logs are very consuming time please check without logging, also maybe there is also room on your reiver to optimize speed to return objects on ports ?

kcudnik · 2024-11-27T15:55:28Z

fropm logs you pasted, it seems like you are creating those posts explicitly? why thsoe ports are not existing already when switch is created ?

stepanblyschak · 2024-11-29T16:31:43Z

Logs are very consuming time please check without logging, also maybe there is also room on your reiver to optimize speed to return objects on ports ?

Logs are set to CRITICAL level during discovery and there are no many logs from syncd itself. We optimized various SAI calls. However, it does not address the main issue which is that on Nvidia platform there's no reason to do discovery.

fropm logs you pasted, it seems like you are creating those posts explicitly? why thsoe ports are not existing already when switch is created ?

It's a matter of user configuration. SAI provides default ports or none, user can override/breakout ports in CONFIG_DB. In later case ports are added using port bulk API. Either way, we would like to not have to discover objects on fast/warm boot as the number of objects and attributes grows with new platforms.

kcudnik · 2024-11-29T17:29:25Z

Then w need deeper discussion here, @lguohan can you jump in? If there will be no warm boot performed on this particular platform, then we can add feature to exclude this specific platform via switch from comma done option

lguohan · 2024-12-04T21:36:16Z

i am a little bit confuse, the PR title is about fast-boot. @kcudnik , while you are talking about warm reboot. what does this code impact?

kcudnik · 2024-12-05T10:30:06Z

it does not matter whether is warm boot or fast boot this wiil cause inconsestincy in db and later on crash

kcudnik · 2024-12-05T15:54:32Z

syncd/SaiSwitch.cpp

@@ -948,46 +948,50 @@ void SaiSwitch::redisUpdatePortLaneMap(

 void SaiSwitch::onPostPortCreate(
        _In_ sai_object_id_t port_rid,
-        _In_ sai_object_id_t port_vid)
+        _In_ sai_object_id_t port_vid,
+        _In_ bool discoverPortObjects)


i feel like this eintrie change in this function is overcomplicated, it sholud be something like this:

if (object_type(oid) == SAI_OBJECT_TYPE_PORT && shouldSkipPorts) continue;

@kcudnik
Regarding object_type(oid) == SAI_OBJECT_TYPE_PORT, the function is called onPostPortCreate so unless someone is calling it on object other than port I don't think this check is needed.

Do you mean early return? Like:

redisUpdatePortLaneMap(port_rid); if (!discoverPortObjects) { return; } ...

ah, i thoung you also modify discover process, since it will also discover all objects on all ports, so i guess on cold boot you only need onpostportcreate, but this could still crash on next fast-boot

please do couple of fst-boot to fast-boot reboots with your patch to see if this will wrok

looking at taht code, you only need to modify SaiDiscovery process with flag to ignore port discovery, no else code is needed to be changed

and ig you look on master, in SaiDiscovery.cpp file at line 34, you can actually pass new flag - to not discover port objects over VendorSaiOptions class to not forward all bool arguments to discover ports, if you want

yes, since we don want to skip port ddetection on other platforms than yours

and you can disable those ports in init script for your platform only

@kcudnik Platform is known at compile time, syncd is compiled differently for different platforms. My change as is right now should not affect other platforms. This change purpose is to improve startup time, however with platform detection done in script I will add some additional CPU cycles for that. Even though it is very small, on some lower systems the init scripts execution time is worse, that's why I am leaning towards moving all to compile time if possible.

if it's know at compile time, put this:

#ifdef nvidia if (object_type(oid) == SAI_OBJECT_TYPE_PORT) continue; #endif

in si discovery

also add log warn message, that discovery port was disabled on nvidia platform

stepanblyschak force-pushed the skip-discovery-on-fast-boot branch from aa63de0 to 995d79b Compare August 22, 2024 22:04

liat-grozovik requested review from kcudnik and vaibhavhd September 3, 2024 08:36

kcudnik requested changes Oct 2, 2024

View reviewed changes

kcudnik reviewed Oct 2, 2024

View reviewed changes

Merge branch 'master' into skip-discovery-on-fast-boot

9ce1a02

Merge branch 'master' into skip-discovery-on-fast-boot

7811b60

Merge branch 'master' into skip-discovery-on-fast-boot

56c4012

kcudnik reviewed Dec 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nvidia] Skip SAI discovery on ports on fast-boot #1416

[nvidia] Skip SAI discovery on ports on fast-boot #1416

stepanblyschak commented Aug 21, 2024 •

edited

Loading

kcudnik left a comment

kcudnik Oct 2, 2024

stepanblyschak Nov 26, 2024

kcudnik Oct 2, 2024

kcudnik Oct 2, 2024

stepanblyschak Nov 26, 2024 •

edited

Loading

kcudnik Dec 5, 2024

stepanblyschak Dec 5, 2024

stepanblyschak commented Nov 26, 2024 •

edited

Loading

kcudnik commented Nov 27, 2024

stepanblyschak commented Nov 27, 2024 •

edited

Loading

kcudnik commented Nov 27, 2024

kcudnik commented Nov 27, 2024

stepanblyschak commented Nov 29, 2024

kcudnik commented Nov 29, 2024

lguohan commented Dec 4, 2024

kcudnik commented Dec 5, 2024

kcudnik Dec 5, 2024

stepanblyschak Dec 5, 2024

kcudnik Dec 5, 2024

kcudnik Dec 5, 2024

kcudnik Dec 5, 2024

kcudnik Dec 6, 2024

kcudnik Dec 6, 2024

stepanblyschak Dec 6, 2024

kcudnik Dec 6, 2024

kcudnik Dec 6, 2024

[nvidia] Skip SAI discovery on ports on fast-boot #1416

Are you sure you want to change the base?

[nvidia] Skip SAI discovery on ports on fast-boot #1416

Conversation

stepanblyschak commented Aug 21, 2024 • edited Loading

kcudnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepanblyschak Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepanblyschak commented Nov 26, 2024 • edited Loading

kcudnik commented Nov 27, 2024

stepanblyschak commented Nov 27, 2024 • edited Loading

kcudnik commented Nov 27, 2024

kcudnik commented Nov 27, 2024

stepanblyschak commented Nov 29, 2024

kcudnik commented Nov 29, 2024

lguohan commented Dec 4, 2024

kcudnik commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepanblyschak commented Aug 21, 2024 •

edited

Loading

stepanblyschak Nov 26, 2024 •

edited

Loading

stepanblyschak commented Nov 26, 2024 •

edited

Loading

stepanblyschak commented Nov 27, 2024 •

edited

Loading