Skip to content
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

Fix flow counter out-of-order issue by notifying counter operations using SelectableChannel #1362

Merged
merged 12 commits into from
Mar 26, 2024

Conversation

stephenxs
Copy link
Contributor

@stephenxs stephenxs commented Mar 13, 2024

What I did

Fix flow counter out-of-order issue by notifying counter operations using SelectableChannel

Signed-off-by: Stephen Sun stephens@nvidia.com

Why I did it

Currently, the operations of SAI objects and their counters (if any) are triggered by different channels, which introduces racing conditions:

  • the creation and destruction of the objects are notified using the SelectableChannel,
  • the operations of counters, including starting and stopping polling the counters, are notified by listening to the FLEX_COUNTER and FLEX_COUNTER_GROUP tables in the FLEX_COUNTER_DB
  • The orchagent always respects the order when starting/stopping counter-polling (which means to start counter-polling after creating the object and to stop counter-polling before destroying the object) but syncd can receive events in a wrong order, eg. it receives destroying an object first and then stopping counter polling on the object, it can poll counter for a non-exist object, which causes errors in vendor SAI.

The new solution is to extend SAI redis attributes on the SAI_SWITCH_OBJECT to notify counter polling. As a result, all the objects and their counters are notified using a unified channel, which is the SelectableChannel.

How I verified it

Unit test
Manual test
Regressions test

Details if related

There are two SAI Redis attributes introduced as below. There are some fields with const char * type for each attribute. Passing a field as nullptr means not to change it.

  • SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER_GROUP for counters represented by FLEX_COUNTER_GROUP table in the FLEX_COUNTER_DB, including the following fields
    • counter_group_name, which is the key of the table, representing the group name.
    • poll_interval, which is the field POLL_INTERVAL of an entry, representing the polling interval of the group.
    • operation, which is the field FLEX_COUNTER_STATUS of an entry, representing whether the counter polling is enabled for the group
    • stats_mode, which is the field STATS_MODE of an entry, either STATS_MODE_READ or STATS_MODE_READ_AND_CLEAR
    • plugins, which represents the Lua plugin related to the group
    • plugin_name, which is the name of the plugins field. It differs among different groups
  • SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER for counter groups represented by the FLEX_COUNTER table in the FLEX_COUNTER_DB, including the following fields
    • counter_key, which is the key of the table, with the name convention of <group-name>:oid:<oid-value>
    • counter_ids, which is a list of counter IDs to be polled for the object
    • counter_field_name, which is the name of the counter ID field. It differs among different groups
    • stats_mode, which is the field STATS_MODE of an entry, either STATS_MODE_READ or STATS_MODE_READ_AND_CLEAR

Both SAI attributes are terminated by the RedisRemoteSaiInterface object in the swss context, which serializes the SAI API call into the selectable channel.

  • REDIS_FLEX_COUNTER_COMMAND_SET_COUNTER_GROUP: represents the SET operation in the FLEX_COUNTER_GROUP table
  • REDIS_FLEX_COUNTER_COMMAND_DEL_COUNTER_GROUP: represents the DEL operation in the FLEX_COUNTER_GROUP table
  • REDIS_FLEX_COUNTER_COMMAND_START_POLL: represents the SET operation in the FLEX_COUNTER table
  • REDIS_FLEX_COUNTER_COMMAND_STOP_POLL: represents the DEL operation in the FLEX_COUNTER table

The Syncd will call flex counter functions to handle them on receiving the above-extended commands (representing both SAI extended attributes).

Gearbox flex counter database
Pass the Phy OID, an OID of a SAI switch object in syntax, when calling the SAI set API to set the extended attributes. By doing so, the SAI redis objects can choose in which context the SAI API call should be invoked and the corresponding gearbox syncd docker container will handle it.
(ps: THE ORIGINAL GEARBOX FLEX COUNTER IMPLEMENTATION IS BUGGY)

Context and critical section analysis
It does not change the critical section hierarchy

Performance analysis
The counter operations are handled in the same thread in both the new and old solutions.
In swss, the counter operation was asynchronous in the old solution and is synchronous now, which can introduce a bit more latency. However, as the number of counter operations is small, no performance degradation is observed.

Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/sairedis.h Outdated Show resolved Hide resolved
lib/sairedis.h Outdated Show resolved Hide resolved
syncd/Syncd.cpp Outdated Show resolved Hide resolved
syncd/Syncd.cpp Outdated Show resolved Hide resolved
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
@kcudnik
Copy link
Collaborator

kcudnik commented Mar 18, 2024

please satisfy code coverage test

Signed-off-by: Stephen Sun <stephens@nvidia.com>
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.cpp Outdated Show resolved Hide resolved
lib/RedisRemoteSaiInterface.h Outdated Show resolved Hide resolved
syncd/Syncd.cpp Show resolved Hide resolved
syncd/Syncd.cpp Outdated Show resolved Hide resolved
syncd/Syncd.cpp Outdated Show resolved Hide resolved
syncd/Syncd.cpp Outdated Show resolved Hide resolved
Signed-off-by: Stephen Sun <stephens@nvidia.com>
@stephenxs stephenxs requested a review from kcudnik March 25, 2024 08:52
@stephenxs
Copy link
Contributor Author

Hi @kcudnik
can you help to merge it?
thanks

@kcudnik kcudnik merged commit 594944d into sonic-net:master Mar 26, 2024
14 checks passed
@stephenxs stephenxs deleted the poc-flexcounter-new-infra branch March 26, 2024 15:12
byu343 added a commit to byu343/sonic-sairedis that referenced this pull request Sep 18, 2024
The counters for syncd (switch chip) were attempted to be added to
gbsyncd (gearbox phys), and vice versa.
This issue is introduced by
sonic-net#1362
When setting the redis attribute
SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER_GROUP and
SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER, the operation is applied to every
contexts (both syncd and gbsyncd). However, the counters to initialize
could only exist in one context.

The fix is to check that the target switch id exists in the context; if
not, skip the operation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants