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

[xcvrd] Handle port config change on fly in xcvrd #839

Merged
merged 5 commits into from Oct 12, 2021

Conversation

Junchao-Mellanox
Copy link
Contributor

@Junchao-Mellanox Junchao-Mellanox commented Aug 13, 2021

Currently, xcvrd assumes that port mapping information is never changed, so it always read static port mapping information from platform.json/port_config.ini and save it to a global data structure. However, things changed since dynamic port breakout feature introduced. Port can be added/created on the fly, xcvrd cannot update transceiver information, DOM information and transceiver status information without knowing the ports change. This change introduces a way to handle port configuration change on fly in xcvrd.

Related PRs:

PR title state context
[xcvrd] Subscribe port configuration change in xcvrd GitHub issue/pull request detail GitHub pull request check contexts

@liat-grozovik
Copy link
Collaborator

@Junchao-Mellanox please add a tracking PRs to the PR description.

@liat-grozovik
Copy link
Collaborator

@zhangyanzhao could you please suggest a reviewer?

keboliu
keboliu previously approved these changes Sep 14, 2021
Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

As per our discussion, I see following issues due to usage of event queue (which is not required IMO)

  1. For each port change event, main task reads the port change from config DB (1st read), writes it into the event queue of both the observers (2 writes). Each of the observer then reads the event from their respective event queue(2 reads). So total 3 reads and 2 writes.
  2. Since, the same event queue is accessed for write by (main task) and read by observer threads, a lock is a must. For each port change event there will be a total of 4 lock and unlock operations (because of 2 reads and 2 writes in the event queue of observers)

Consider a worst case where there are 36 ports in x4 breakout so a total of 144 logical ports entries in the event queue needs to be added or deleted which means a total of 144 * 4 = 576 lock/unlock operations.

Lets consider an alternative approach, without the event queue. Since, DB allows to share information across multiple process, we can just let the observers listen to CONFIG_DB changes instead of only main thread. This way each observer can fetch the port mapping independently by reading the PORT_TABLE in APP_DB. No queue required and ZERO locks required. Please note, three select() is lightweight operation compared to lock/unlocks. In this approach there will be only 3 reads required compared to 3 reads and 2 writes and additional overhead due to mutually exclusive access to the event queue.

doc/xrcvd/transceiver-monitor-hld.md Outdated Show resolved Hide resolved
@prgeor prgeor merged commit 6bebaac into sonic-net:master Oct 12, 2021
@zhangyanzhao zhangyanzhao added this to In progress in 202111 Release HLD via automation Oct 12, 2021
@zhangyanzhao zhangyanzhao moved this from In progress to Done in 202111 Release HLD Oct 12, 2021
@Junchao-Mellanox Junchao-Mellanox deleted the update-xcvrd branch October 14, 2021 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants