-
Notifications
You must be signed in to change notification settings - Fork 338
Add PORT_PHY_ATTR flex counter support #1674
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
base: master
Are you sure you want to change the base?
Add PORT_PHY_ATTR flex counter support #1674
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dhanasekar-arista please fix the build errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanasekar-arista can you please confirm if SAI_PORT_ATTR_FEC_ALIGNMENT_LOCK and SAI_PORT_RX_SIGNAL_DETECT are using sai_serialize_port_lane_latch_status_list for serialization ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@dhanasekar-arista PR is in DRAFT ? Not ready? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This commit introduces support for polling PORT PHY attributes using the flex counter infrastructure.
Following changes are added in sairedis to support this feature
- Added an extra parameter(AttrDataType) to the template of class AttrContext,
this is required to handle attributes with complex data strucutres.
- Introduce PortAttrContext extending AttrContext with
specialized data handling for lane-based attribute types
- Refactor serialization to use compact JSON dictionary format instead of
verbose array-of-objects (e.g., {"0":"T*","1":"F"})
- Support complex attribute value types (PORT_LANE_LATCH_STATUS_LIST,
PORT_SNR_LIST) with proper memory management
- Write collected metrics to PORT_PHY_ATTR_TABLE with attribute aliases:
* phy_rx_signal_detect (latch status: T/F with * for changed)
* pcs_fec_lane_alignment_lock (per-lane FEC lock status)
* rx_snr (per-lane SNR values)
- Add validation helpers and unit tests covering serialize/deserialize and
full collection workflow with mocked SAI interface
0c1a013 to
bf52df5
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: dhanasekar-arista <dhanasekar@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| arr.push_back(item); | ||
| if (status_list.list[i].value.changed) | ||
| { | ||
| value += "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanasekar-arista we need an entry in DB to know the time when the value changed because the next polling may overwrite this * value and SW may miss to there was a signal change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try to understand the usecase with an example,
T1 - application reads T* from DB
T2 - application reads T* from DB
but there is a chance that value might have transitioned from T* -> F* -> T* in between reads at T1, T2.
So you want to add timestamp to detect if this got modified since the last read or not.
Is my understanding correct?
IOW,
'*' - indicates the change of value at asic level, i.e if signal changed between 2 successive poll from syncd
'timestamp (nanosec since epoch in int64)' - helps the application detect if the value in counters DB flapped between 2 calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please confirm the final format in which we want the output.
Shall we use this format?
{"0":["T*",1734960123456],"1":["F",1734960123456]}
I think millisec granularity should be more than sufficient and nanosec would be an overkill.
|
|
||
| #include "FlexCounter.h" | ||
| #include "VidManager.h" | ||
| #include "SaiSwitch.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanasekar-arista why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this line, i had hardcoded MAX_LANES_PER_PORT as the number of lanes initially which required this.
|
|
||
| #include "swss/redisapi.h" | ||
| #include "swss/tokenize.h" | ||
| #include "swss/schema.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanasekar-arista why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required will remove it.
| } | ||
|
|
||
| template <> | ||
| void deserializeAttr( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanasekar-arista should be renamed to deserializePortAttr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we cannot rename this. This is a template specialization of the generic deserializeAttr template function that's used by the AttrContext class template.
| }; | ||
|
|
||
| // Specialized context for PORT_PHY_ATTR that writes to dedicated table with port name as key | ||
| class PortAttrContext : public AttrContext<sai_port_attr_t, PortAttributeData> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanasekar-arista nit, PortAttrContext -> PortPhyAttrContext rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Implement PORT_PHY_ATTR flex counter support for monitoring the port attributes (RX_SIGNAL_DETECT, FEC_ALIGNMENT_LOCK, RX_SNR).
Changes
meta/: Add serialization support for PORT_SNR_LIST and PORT_LANE_LATCH_STATUS_LIST
syncd/: Introduce PortAttrContext extending AttrContext with specialized data handling for lane-based attribute types
unittest/: Add unit tests for serialization and collectData APIs.
Implementation
Queries SAI for per-port lane count using BUFFER_OVERFLOW pattern
Maintains lane count cache for efficient memory allocation
Writes collected data to dedicated PORT_PHY_ATTR table in COUNTERS_DB