Feature: PTP Peer-to-Peer (P2P) Path Delay Support - #2324
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
c9b91ab to
3ff98d8
Compare
| _Out_ char *buffer, | ||
| _In_ const sai_ptp_l2_header_t header) | ||
| { | ||
| return 0; |
There was a problem hiding this comment.
missing serialization method
| _In_ const char *buffer, | ||
| _Out_ sai_ptp_l2_header_t header) | ||
| { | ||
| return 0; |
There was a problem hiding this comment.
missing serialization method
| return (int)(buf - buffer); | ||
| } | ||
|
|
||
| int sai_serialize_ptp_clock_id( |
There was a problem hiding this comment.
if you are adding serialize and dserialize, then add also unittest for that
There was a problem hiding this comment.
custom serializer removed. applies to other comments as well.
| id[n] = (uint8_t)arr[n]; | ||
| } | ||
|
|
||
| return 8; |
There was a problem hiding this comment.
it should return "read" variable, number of consumed characters, take a look at sai_deserialize_mac and sai_deserialize_object_id
| CHECK_STRUCT_SIZE(sai_acl_field_data_data_t, 16); | ||
| CHECK_STRUCT_SIZE(sai_acl_field_data_mask_t, 16); | ||
| CHECK_STRUCT_SIZE(sai_attribute_value_t, 40); | ||
| CHECK_STRUCT_SIZE(sai_attribute_value_t, 64); |
There was a problem hiding this comment.
this is breaking (not backward compatible) change as attribute value size is changing
There was a problem hiding this comment.
fixed to use sai_u8_list_t instead
2c81938 to
06a3bb1
Compare
Adds the headers and design specification for the PTP Pdelay configuration object, expanding SAI capabilities to configure localized hardware measurement profiles for link propagation delays. This incorporates the v3 struct simplifications, utilizing standard sai_u8_list_t mappings to preserve SAI ABI compatibility. Signed-off-by: gurprem <gurprem@google.com>
06a3bb1 to
d7ba673
Compare
|
Thanks for review @kcudnik. Latest patch is updated so both attributes now use the standard sai_u8_list_t instead. This keeps the union at 40 bytes, and since it's a natively-supported type, it no longer needs any custom serialize/deserialize methods or their unit tests. |
| * @flags CREATE_AND_SET | ||
| * @default 0 | ||
| */ | ||
| SAI_PORT_ATTR_LINK_DELAY, |
There was a problem hiding this comment.
Please specify the unit in comments
| * | ||
| * @type sai_mac_t | ||
| * @flags CREATE_AND_SET | ||
| * @default vendor |
There was a problem hiding this comment.
Add comments explaining how default as vendor is useful
| * @flags CREATE_AND_SET | ||
| * @default 0 | ||
| */ | ||
| SAI_PTP_PDELAY_ATTR_L2_HEADER_LEN, |
| * @flags CREATE_AND_SET | ||
| * @default empty | ||
| */ | ||
| SAI_PTP_PDELAY_ATTR_PTP_L2_HEADER, |
There was a problem hiding this comment.
Add details that this header length is specified by the L2_HEADER_LEN attribute
| * @isvlan false | ||
| * @default 0 | ||
| */ | ||
| SAI_SWITCH_ATTR_PTP_PDELAY_MAX_PORTS, |
There was a problem hiding this comment.
Should there be a READ attribute for platform supporting max number of ports that can be enabled for peer delay?
| Setting this to `SAI_NULL_OBJECT_ID` detaches the profile and disables P2P | ||
| processing on the physical interface. | ||
|
|
||
| ### SAI_PORT_ATTR_LINK_DELAY |
There was a problem hiding this comment.
We already have SAI_PORT_ATTR_PTP_PEER_MEAN_PATH_DELAY. How would this and PDELAY_LINK_DELAY below differ from the existing one ?.
| * @flags CREATE_AND_SET | ||
| * @default 0 | ||
| */ | ||
| SAI_PORT_ATTR_PDELAY_LINK_DELAY, |
There was a problem hiding this comment.
how is it related to SAI_PORT_ATTR_LINK_DELAY below ?
| * @default 0 | ||
| */ | ||
| SAI_PORT_ATTR_LINK_DELAY, | ||
|
|
There was a problem hiding this comment.
how is it related to SAI_PORT_ATTR_PDELAY_LINK_DELAY ?
| * @type sai_uint8_t | ||
| * @flags CREATE_AND_SET | ||
| * @default 1 | ||
| */ |
| * @type sai_uint8_t | ||
| * @flags CREATE_AND_SET | ||
| * @default 0 | ||
| */ |
| * @flags CREATE_AND_SET | ||
| * @default empty | ||
| */ | ||
| SAI_SWITCH_ATTR_CLOCK_ID, |
There was a problem hiding this comment.
what should be set here ? How is it going to be used ? In PTP header ?
There was a problem hiding this comment.
https://github.com/gurprem-singh/SAI/blob/d7ba67347b04da77b741bd933730674184837c74/doc/PTP/SAI-Proposal-PTP-Pdelay.md?plain=1#L190
Ideally not the global clock instance, but specific to peer delay messages. Description can be updated accordingly. There is already SAI_SWITCH_ATTR_PORT_PTP_MODE towards global clock operation for PTP event messages.
https://github.com/gurprem-singh/SAI/blob/d7ba67347b04da77b741bd933730674184837c74/doc/PTP/SAI-Proposal-PTP-Pdelay.md?plain=1#L167
What is the difference between SAI_PORT_ATTR_PDELAY_LINK_DELAY and SAI_PORT_ATTR_PDELAY_NEIGHBOR_PROPAGATION_DELAY
https://github.com/gurprem-singh/SAI/blob/d7ba67347b04da77b741bd933730674184837c74/doc/PTP/SAI-Proposal-PTP-Pdelay.md?plain=1#L130
To specify how this inter operates with SAI_HOSTIF_TRAP_TYPE_PTP_PEER_DELAY. If Pdelay measurement is offloaded to hardware then the packet should be terminated and trap should be disabled for that port.
6 participants
Adds the headers and design specification for the PTP Pdelay configuration object, expanding SAI capabilities to configure localized hardware measurement profiles for link propagation delays.