-
Notifications
You must be signed in to change notification settings - Fork 275
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
ASIC temperature sensors support #383
Conversation
@@ -138,6 +138,9 @@ std::string sai_serialize_ipmc_entry_type( | |||
std::string sai_serialize_qos_map_item( | |||
_In_ const sai_qos_map_t& qosmap); | |||
|
|||
std::string sai_serialize_switch_attr( |
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 remove we already have generic function to serialize attributes for all objects
@@ -287,4 +290,8 @@ void sai_deserialize_queue_attr( | |||
_In_ const std::string& s, | |||
_Out_ sai_queue_attr_t& attr); | |||
|
|||
void sai_deserialize_switch_attr( |
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 remove we already have generic function to deserialize attributes for all objects
@@ -1636,6 +1636,14 @@ std::string sai_serialize_object_meta_key( | |||
return key; | |||
} | |||
|
|||
std::string sai_serialize_switch_attr( |
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 remove we already have generic function to serialize attributes for all objects
@@ -2976,3 +2984,12 @@ void sai_deserialize_queue_attr( | |||
|
|||
sai_deserialize_enum(s, &sai_metadata_enum_sai_queue_attr_t, (int32_t&)attr); | |||
} | |||
|
|||
void sai_deserialize_switch_attr( |
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 remove we already have generic function to deserialize attributes for all objects
@@ -8,6 +8,7 @@ static std::map<std::string, std::shared_ptr<FlexCounter>> g_flex_counters_map; | |||
static std::set<sai_port_stat_t> supportedPortCounters; | |||
static std::set<sai_queue_stat_t> supportedQueueCounters; | |||
static std::set<sai_ingress_priority_group_stat_t> supportedPriorityGroupCounters; | |||
static std::set<sai_switch_attr_t> supportedSwitchSensors; |
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.
why temperature sensors are in counters ? i think we dont need this implementation here at all, seems like sensors are just another attribute and we can definitely query them
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.
ASIC sensor readings are not counters per-se - yes.. This is one of the few environmental variables that need to be retrieved thru' SAI : the list may grow but may not bloat. We wanted to leverage the existing flex counter infra (instead of creating a private poller).
Since thermal algorithms may typically need this every few seconds, having a script based daemon may be possible.
Can you point me to how a script may query attributes directly (rather than query the DB) ?
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.
as script i dont know, i had in mind orchagent which would query those sensors using sai apis
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.
since the load is low, ~8 values in maybe one minute. Moving to orchagent is better. agree with @kcudnik
@@ -2907,6 +2907,17 @@ void processFlexCounterEvent( | |||
|
|||
FlexCounter::setPriorityGroupAttrList(vid, rid, groupName, pgAttrIds); | |||
} | |||
else if (objectType == SAI_OBJECT_TYPE_SWITCH && field == SWITCH_SENSOR_ID_LIST) |
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.
why we need this here as special case ?
also this solution for those cases here is getting bigger and bigger we should think of some generic approach
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 wasn't sure if there could be other (non thermal) attributes that may be of interest in the future.. Agree that there should be a generic approach to handle flex group events (a single table) instead of having to extend event/attribute handling multiple locations...
Hi Padamanarayan, |
Hi Amit, |
Thanks Padamanarayana, Will the poller for ASIC internal sensors be able to handle other ASIC controls like LED ? As the higher speed ports can support various speeds(400G,100G,25G, 40G,10G) and multi-color LED reflects the color based on the port speed configured which needs to be updated whenever the port is configured for a specific speed. Can the platform API be provided to control the ASIC LED as well. Could you please confirm about the tentative timelines for adding poller support? |
Please do not merge yet.
Recently (opencomputeproject/SAI#880), new switch attributes were added to retrieve the temperature readings from the ASIC's internal sensors.
This is a preliminary commit (pending SAI support from vendors) for temperature monitoring. The max, average and the entire list of temperatures are now added to the flex counters DB so that platform sensors scripts may query and use these values in thermal control algorithms.