Add switch-level attribute to control PTP for the entire switch #2148
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 2148 in repo opencomputeproject/SAI |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@rdsugarmannv - as discussed please fix the meta checker errors WARNING: first line doxygen comment should be with '/': saiswitch.h: / |
|
@rajkumar38, @rck-innovium, @JaiOCP - please help review |
Hopefully fixed now |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@rdsugarmannv - still failing you may need to squash your commits and force push |
done |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
rck-innovium
left a comment
rck-innovium
left a comment
There was a problem hiding this comment.
- The existing SAI_PORT_ATTR_PTP_MODE=SAI_PORT_PTP_MODE_NONE meaning was to disable special PTP processing on the port. Now we are changing this to mean that SAI_SWITCH_ATTR_PTP_MODE should take effect. However, the comments do not bring this out clearly.
- What does the below config mean?
SAI_SWITCH_ATTR_PTP_MODE = SAI_SWITCH_PTP_MODE_SINGLE_STEP_TIMESTAMP, and port P1's SAI_PORT_ATTR_PTP_MODE=SAI_PORT_PTP_MODE_NONE.
If we want to enable PTP single step timestamp on all ports, and disable PTP processing on port P1, what should be the config?
I think you're asking that I change the PORT comments to reference the global ones so this behavior is clear? I'll do that.
It means global/SWITCH-level config takes effect. The port would use SINGLE_STEP mode. I'll resolve this by updating the comments for the PORT attributes (same action as above).
Today, before my PR, you'd do this by using the per-port API. You'd set all ports to SINGLE_STEP, and then set the ports you wanted to disable to NONE. With my PR, this doesn't change. You wouldn't set the SWITCH attribute at all, and it would default to NONE, and you'd get the same bahvior. In future, if you wanted to use the SWITCH API to enable PTP for all ports, and then switch it off on a per-port basis, you could add a new PORT-level attribute called "OFF" (or similar). But I don't want to add that as part of this change, since there's no requirement for it yet, and I don't want to add attributes that no-one plans to use. Sound OK? |
|
I've re-used the PORT enum at both levels, as requested. |
| * @brief PTP mode | ||
| * These modes can be used at the port and switch level. | ||
| * All ports use the value set at the switch level unless explicitly configured | ||
| * at the port level to a value other than SAI_PORT_PTP_MODE_NONE. |
There was a problem hiding this comment.
@kcudnik We want SAI_SWITCH_ATTR_PTP_MODE (new) and SAI_PORT_ATTR_PTP_MODE (existing) to use the same enums. Today, SAI_PORT_ATTR_PTP_MODE is using sai_port_ptp_mode_t.
Is there a way to deprecate sai_port_ptp_mode_t and instead use a new sai_ptp_mode_t without breaking backward compatibility? The enum values will be the same.
There was a problem hiding this comment.
You can change enum in switch attributes in new one, and add in comments that this is deprecated, it will introduce issues in serialization and deserializstion in sonic sairedis where we would need to add manual conversion for both of those enums, we can discuss this on sai community meeting today
Is there a reason that ptp mode was introduced in switch if it applies to ports anyway ? Or you just want to have anum without port I name? If values will be the same then it seems like one of those enums is redundant and should not be added on the first place
There was a problem hiding this comment.
And it seems like switch attributes should be named SAI SWITCH ATTR PORT PTP MODE maybe
I think description of this attributes explains it, but also it seems like this attributes is redundant, if this only applies to ports, then this attribute will n3ver have effect since each time port ptp mode will be different, port always overrides this, not sure why this was added at the first place, let's discuss on sai meeting g
There was a problem hiding this comment.
@kcudnik We want SAI_SWITCH_ATTR_PTP_MODE (new) and SAI_PORT_ATTR_PTP_MODE (existing) to use the same enums. Today, SAI_PORT_ATTR_PTP_MODE is using sai_port_ptp_mode_t.
Is there a way to deprecate sai_port_ptp_mode_t and instead use a new sai_ptp_mode_t without breaking backward compatibility? The enum values will be the same.
I don't see an obvious way to deprecate the old names that doesn't introduce confusion. There are other examples that share enums the way I've done here. For example, see how sai_tunnel_encap_ecn_mode_t is used by both SAI_SWITCH_TUNNEL_ATTR_TUNNEL_ENCAP_ECN_MODE and SAI_TUNNEL_ATTR_ENCAP_ECN_MODE.
There was a problem hiding this comment.
And it seems like switch attributes should be named SAI SWITCH ATTR PORT PTP MODE maybe
I think description of this attributes explains it, but also it seems like this attributes is redundant, if this only applies to ports, then this attribute will n3ver have effect since each time port ptp mode will be different, port always overrides this, not sure why this was added at the first place, let's discuss on sai meeting g
I'll be at the SAI meeting today.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| ### New Switch PTP Configuration Type | ||
| ```c | ||
| typedef struct _sai_switch_ptp_mode_t | ||
| { |
There was a problem hiding this comment.
All the comments that are addressed in the C header files needs to be reflect in this MD file as well.
|
please fix errors: |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
inc/saiswitch.h
Outdated
| * @flags CREATE_AND_SET | ||
| * @default SAI_PORT_PTP_MODE_NONE | ||
| */ | ||
| SAI_SWITCH_ATTR_PTP_MODE, |
There was a problem hiding this comment.
The sai meta checkers would expect this attribute to be named as SAI_SWITCH_ATTR_PORT_PTP_MODE or the enum to be named as sai_ptp_mode_t.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@rdsugarmannv - please squash your commits |
Signed-off-by: Rich Sugarman <[email protected]>
done |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
7 participants
The SAI API currently only allows PTP to be configured on a individual port basis. Some switches only support global configuration (on/off) for PTP. This change adds a switch level SAI attribute to enable PTP for the whole switch.