Skip to content

ASIC temperature sensors support#383

Closed
padmanarayana wants to merge 1 commit intosonic-net:masterfrom
padmanarayana:asic_sens_saired
Closed

ASIC temperature sensors support#383
padmanarayana wants to merge 1 commit intosonic-net:masterfrom
padmanarayana:asic_sens_saired

Conversation

@padmanarayana
Copy link

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.

std::string sai_serialize_qos_map_item(
_In_ const sai_qos_map_t& qosmap);

std::string sai_serialize_switch_attr(
Copy link
Collaborator

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

_In_ const std::string& s,
_Out_ sai_queue_attr_t& attr);

void sai_deserialize_switch_attr(
Copy link
Collaborator

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

return key;
}

std::string sai_serialize_switch_attr(
Copy link
Collaborator

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

sai_deserialize_enum(s, &sai_metadata_enum_sai_queue_attr_t, (int32_t&)attr);
}

void sai_deserialize_switch_attr(
Copy link
Collaborator

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

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;
Copy link
Collaborator

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

Copy link
Author

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) ?

Copy link
Collaborator

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

Copy link
Contributor

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


FlexCounter::setPriorityGroupAttrList(vid, rid, groupName, pgAttrIds);
}
else if (objectType == SAI_OBJECT_TYPE_SWITCH && field == SWITCH_SENSOR_ID_LIST)
Copy link
Collaborator

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

Copy link
Author

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...

@AmitKaushik7
Copy link

Hi Padamanarayan,
I see that these changes were not merged. Any reason for cancelling this. Are you coming up with some generic approach to add temperature sensors support?
Thanks
Amit

@padmanarayana
Copy link
Author

Hi Amit,
Yes, we're planning to add a poller for the ASIC internal sensors and provide platform APIs that thermal algorithms may use. Will raise HLD PR for review at the earliest.

@AmitKaushik7
Copy link

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?

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.

4 participants