Assign values to enum fields #1259
Conversation
Problem ======== Consider a scenario where an application (adapter host) using SAI spec X (say) warmboots to an application using new SAI spec Y (say). If an enum is extended between X and Y with fields inserted "in between" rather than "at the end" of the enum, the old fields can get "renumbered". However, the SAI implementation (adapter) continues to return old enums for previously returned objects thereby breaking the warmboot. Possible approaches =================== We discussed this during June 24, 2021 weekly SAI meeting and observed: - This can e avoided by mandating that any new inserts to an enum will be "at the end" (this could mean at the end of _END block). This will keep the enum field values the same. - However, that will come at the cost of readability: newly added fields may be related to existing fields and thus for readability, those fields belong togeter. Adding those to the end scatters the related fields across the enum. Solution ========= Thus, we agreed to go with the below approach: - Every enum field will have an explicit integer value assigned to it. - Insertions can be "in between" (maintains readability) but the next available unused integer value would be used for the new insertions. - Thus, once an integer is assigned to an enum field, it does not change with spec revision thereby not breaking warmboot. - Between SAI spec 1.6.5 to 1.8.1, saihash.h and saiacl.h had new enms inserted "in between". Those will be "fixed" by assigning integer values to enum fields. This fix will be applied to top of the sai.git tree. This fix will then be ported to 1.8.1 and 1.8.2 will be released. This patch =========== Towards that end, this diff fixes saiacl.h. Specifically, below patch was first introduced in v1.7.0 and inserted enum fields 'in betweeen': opencomputeproject@1aeb1c8 Thus, - In 1.6.5: SAI_ACL_ENTRY_ATTR_FIELD_DSCP is 4126, SAI_ACL_ENTRY_ATTR_ACTION_SET_DSCP is 8214 etc. - In 1.8.1: SAI_ACL_ENTRY_ATTR_FIELD_DSCP is 4134, SAI_ACL_ENTRY_ATTR_ACTION_SET_DSCP is 8216 etc. If the application created ACL entry with SAI_ACL_ENTRY_ATTR_FIELD_DSCP (4126) prior to warmboot, even after warmboot, a get query on that ACL entry returns 4126 which no longer means SAI_ACL_ENTRY_ATTR_FIELD_DSCP. This patch fixes it by assigning explicit integer values, and thus SAI_ACL_ENTRY_ATTR_FIELD_DSCP continues to be 4126 in newer SAI specs as well. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
Problem ======== Consider a scenario where an application (adapter host) using SAI spec X (say) warmboots to an application using new SAI spec Y (say). If an enum is extended between X and Y with fields inserted "in between" rather than "at the end" of the enum, the old fields can get "renumbered". However, the SAI implementation (adapter) continues to return old enums for previously returned objects thereby breaking the warmboot. Possible approaches =================== We discussed this during June 24, 2021 weekly SAI meeting and observed: - This can e avoided by mandating that any new inserts to an enum will be "at the end" (this could mean at the end of _END block). This will keep the enum field values the same. - However, that will come at the cost of readability: newly added fields may be related to existing fields and thus for readability, those fields belong togeter. Adding those to the end scatters the related fields across the enum. Solution ========= Thus, we agreed to go with the below approach: - Every enum field will have an explicit integer value assigned to it. - Insertions can be "in between" (maintains readability) but the next available unused integer value would be used for the new insertions. - Thus, once an integer is assigned to an enum field, it does not change with spec revision thereby not breaking warmboot. - Between SAI spec 1.6.5 to 1.8.1, saihash.h and saiacl.h had new enms inserted "in between". Those will be "fixed" by assigning integer values to enum fields. This fix will be applied to top of the sai.git tree. This fix will then be ported to 1.8.1 and 1.8.2 will be released. This patch =========== Specifically, below patch was first introduced in v1.7.0 and inserted enum fields 'in betweeen': opencomputeproject@21bdb48 - In 1.6.5: SAI_NATIVE_HASH_FIELD_L4_SRC_PORT is 7, SAI_NATIVE_HASH_FIELD_L4_DST_PORT is 8. - In 1.8.1: SAI_NATIVE_HASH_FIELD_L4_SRC_PORT is 15, SAI_NATIVE_HASH_FIELD_L4_DST_PORT is 16. If the application configured SAI_NATIVE_HASH_FIELD_L4_SRC_PORT (7) prior to warmboot, even after warmboot, a query returns 7 which no longer means SAI_NATIVE_HASH_FIELD_L4_SRC_PORT in 1.8.1. This patch fixes it by assigning explicit integer values, and thus SAI_NATIVE_HASH_FIELD_L4_SRC_PORT continues to be 7 in newer SAI specs as well. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
8f2a3c8 to
060eb98
Compare
|
The meta checks fail here: ./meta/saisanitycheck.c META_ENUM_ASSERT_FAIL(emd, "values are not increasing by 1: last: %d current: %d, should be marked as @flags?", last, value); This is expected as we are now introducing consecutive enum fields that are not increasing by 1 by design. |
There was a problem hiding this comment.
This problem is addressed already here: #1256
This change will make sure enum numbers don't change over time, and no need to explicit numbering
There is no need for gaps in enums like SAI_NATIVE_HASH_FIELD_IPV6_FLOW_LABEL vs SAI_NATIVE_HASH_FIELD_NONE. Old enums should exists, and should be marked as depreacakted.
no, this check must stay, to make sure enums are consistent, there is no need to introduce gaps in enums. |
|
Thanks Kamil. Posted few questions about this here: #1256 (comment) |
done |
|
|
||
| /** | ||
| * @brief End of Rule Match Fields | ||
| */ | ||
| SAI_ACL_ENTRY_ATTR_FIELD_END = SAI_ACL_ENTRY_ATTR_FIELD_TAM_INT_TYPE, | ||
| SAI_ACL_ENTRY_ATTR_FIELD_END = SAI_ACL_ENTRY_ATTR_FIELD_DST_IPV6_WORD0, |
There was a problem hiding this comment.
previous was correct, i assume that SAI_ACL_ENTRY_ATTR_FIELD_END will shift (can only increase from SAI to SAI)
There was a problem hiding this comment.
|
|
||
| /** | ||
| * @brief End of Rule Actions | ||
| */ | ||
| SAI_ACL_ENTRY_ATTR_ACTION_END = SAI_ACL_ENTRY_ATTR_ACTION_SET_VRF, | ||
| SAI_ACL_ENTRY_ATTR_ACTION_END = SAI_ACL_ENTRY_ATTR_ACTION_ADD_VLAN_PRI, |
There was a problem hiding this comment.
previous was correct, i assume that SAI_ACL_ENTRY_ATTR_ACTION_END will shift (can only increase from SAI to SAI)
There was a problem hiding this comment.
|
|
||
| /** | ||
| * @brief End of ACL Table Match Field | ||
| */ | ||
| SAI_ACL_TABLE_ATTR_FIELD_END = SAI_ACL_TABLE_ATTR_FIELD_TAM_INT_TYPE, | ||
| SAI_ACL_TABLE_ATTR_FIELD_END = SAI_ACL_TABLE_ATTR_FIELD_DST_IPV6_WORD0, |
There was a problem hiding this comment.
previous was correct, i assume that SAI_ACL_TABLE_ATTR_FIELD_END will shift (can only increase from SAI to SAI)
There was a problem hiding this comment.
|
Actually this PR is not needed, i addressed this in comment #1259 (review), after checking #1256 those checks will be enforced |
|
if you want to keep those values, it would be better to reorder those enums to achieve desired values, it would have better looking, and after enum lock PR merged, you never have to assign them explicitly, since they always will be locked |
|
|
||
| /** | ||
| * @brief Native hash field source IPv4. | ||
| * | ||
| * Also, refers to the outer source IPv4 | ||
| * in case for encapsulated packets | ||
| */ | ||
| SAI_NATIVE_HASH_FIELD_SRC_IPV4, | ||
| SAI_NATIVE_HASH_FIELD_SRC_IPV4 = 0x00000019, |
There was a problem hiding this comment.
why this enum must bat a this place and not at the end when it was previously there? SRC/DST enums are grouped together even in this configuration so what the point to not make those enums not continuous ?
|
This change #1274 allows to not ordered enum values and should accept your changes |
|
@JaiOCP , @tushar-ty , @marian-pritsak , please help to review/approve as well. Thanks. |
@itaibaz , @ashutosh-agrawal , @rck-innovium - please feel free to review/approve as well. Thanks. |
you guess XD if you guess wrong, metadata validator will show you previous and current value |
Thanks Kamil, |
magic ! :P all enums must be contagious, even in respective ranges (there are some exceptions like sai_status_t or bit field based enums, and if enum value is duplicate then metadata will throw error on that, also not always you need to hunt last value, if you are adding enum at the end of list, you dont need to specify it explicitly, only when you are inserting enum in the middle im not sure about concept with XXX_ATTR_LAST_ENUM, if you mean that this enum is artificially generated and explicitly set to a last value, then it will not work, since not always you want to add enum at the end, there are ranges based on 0x1000 index for example in hostif and acl, and you want to add specific value at the end of that specific range |
I like magic :) |
|
@JaiOCP @lguohan @shri-khare i make git diff 2cc5ba0 origin/master where that commit is branch (tag: v1.6.5) and i found other attributes added in the middle besides acl: attributes between 1.6.5 and origin/master that caused shift in enums:
many other attributes were added but at the end of list, so does not break anything Should this be fixed before merging this? or you just want acl and hash, ignoring other changes? we can actually make fix for that even after marge this, but i will need a base commit ID from which we want to be binary backward compatible, but that fix would need to be on master, and from master we would check compatibility. There is also some issue, that entire segment route object was renamed to srv6 object in the meantime |
Given that no one has complained for these attributes and warm boot, I would say its safe to fix these attributes while we are at it. @lguohan What do you think? |
|
I wrote a tool to list those attributes, and this also will be later doing validation on each PR. couple infos: so diff ec7e055 to 2cc5ba0 (master to 1.6.5) (excluding acl_table, acl_entry, tam_int and macsec: this vlan can be skipped, i remember bringing it back from custom range to attr range 5a98bc3 to 5254200 has no changes and ec7e055 to origin/master (total 347 enum values affected, too much to list here but those are numbers: |
Problem ======== Consider a scenario where an application (adapter host) using SAI spec X (say) warmboots to an application using new SAI spec Y (say). If an enum is extended between X and Y with fields inserted "in between" rather than "at the end" of the enum, the old fields can get "renumbered". However, the SAI implementation (adapter) continues to return old enums for previously returned objects thereby breaking the warmboot. Possible approaches =================== We discussed this during June 24, 2021 weekly SAI meeting and observed: - This can e avoided by mandating that any new inserts to an enum will be "at the end" (this could mean at the end of _END block). This will keep the enum field values the same. - However, that will come at the cost of readability: newly added fields may be related to existing fields and thus for readability, those fields belong togeter. Adding those to the end scatters the related fields across the enum. Solution ========= Thus, we agreed to go with the below approach: - Every enum field will have an explicit integer value assigned to it. - Insertions can be "in between" (maintains readability) but the next available unused integer value would be used for the new insertions. - Thus, once an integer is assigned to an enum field, it does not change with spec revision thereby not breaking warmboot. - Between SAI spec 1.6.5 to 1.8.1, saihash.h and saiacl.h had new enms inserted "in between". Those will be "fixed" by assigning integer values to enum fields. This fix will be applied to top of the sai.git tree. This fix will then be ported to 1.8.1 and 1.8.2 will be released. This patch =========== Towards that end, this diff fixes saiacl.h. Specifically, below patch was first introduced in v1.7.0 and inserted enum fields 'in betweeen': 1aeb1c8 Thus, - In 1.6.5: SAI_ACL_ENTRY_ATTR_FIELD_DSCP is 4126, SAI_ACL_ENTRY_ATTR_ACTION_SET_DSCP is 8214 etc. - In 1.8.1: SAI_ACL_ENTRY_ATTR_FIELD_DSCP is 4134, SAI_ACL_ENTRY_ATTR_ACTION_SET_DSCP is 8216 etc. If the application created ACL entry with SAI_ACL_ENTRY_ATTR_FIELD_DSCP (4126) prior to warmboot, even after warmboot, a get query on that ACL entry returns 4126 which no longer means SAI_ACL_ENTRY_ATTR_FIELD_DSCP. This patch fixes it by assigning explicit integer values, and thus SAI_ACL_ENTRY_ATTR_FIELD_DSCP continues to be 4126 in newer SAI specs as well. Signed-off-by: Shrikrishna Khare <skhare@fb.com> * Assign explicit integer values to Hash enum fields Problem ======== Consider a scenario where an application (adapter host) using SAI spec X (say) warmboots to an application using new SAI spec Y (say). If an enum is extended between X and Y with fields inserted "in between" rather than "at the end" of the enum, the old fields can get "renumbered". However, the SAI implementation (adapter) continues to return old enums for previously returned objects thereby breaking the warmboot. Possible approaches =================== We discussed this during June 24, 2021 weekly SAI meeting and observed: - This can e avoided by mandating that any new inserts to an enum will be "at the end" (this could mean at the end of _END block). This will keep the enum field values the same. - However, that will come at the cost of readability: newly added fields may be related to existing fields and thus for readability, those fields belong togeter. Adding those to the end scatters the related fields across the enum. Solution ========= Thus, we agreed to go with the below approach: - Every enum field will have an explicit integer value assigned to it. - Insertions can be "in between" (maintains readability) but the next available unused integer value would be used for the new insertions. - Thus, once an integer is assigned to an enum field, it does not change with spec revision thereby not breaking warmboot. - Between SAI spec 1.6.5 to 1.8.1, saihash.h and saiacl.h had new enms inserted "in between". Those will be "fixed" by assigning integer values to enum fields. This fix will be applied to top of the sai.git tree. This fix will then be ported to 1.8.1 and 1.8.2 will be released. This patch =========== Specifically, below patch was first introduced in v1.7.0 and inserted enum fields 'in betweeen': 21bdb48 - In 1.6.5: SAI_NATIVE_HASH_FIELD_L4_SRC_PORT is 7, SAI_NATIVE_HASH_FIELD_L4_DST_PORT is 8. - In 1.8.1: SAI_NATIVE_HASH_FIELD_L4_SRC_PORT is 15, SAI_NATIVE_HASH_FIELD_L4_DST_PORT is 16. If the application configured SAI_NATIVE_HASH_FIELD_L4_SRC_PORT (7) prior to warmboot, even after warmboot, a query returns 7 which no longer means SAI_NATIVE_HASH_FIELD_L4_SRC_PORT in 1.8.1. This patch fixes it by assigning explicit integer values, and thus SAI_NATIVE_HASH_FIELD_L4_SRC_PORT continues to be 7 in newer SAI specs as well. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
Assign values to sai_fdb_entry_attr_t enum fields This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_fdb_entry_attr_t. Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
This stack is towards supporting FDB Source User Meta. This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_switch_attr_t Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
This stack is towards supporting FDB Source User Meta. This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_switch_attr_t Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
This stack is towards supporting FDB Source User Meta. This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_switch_attr_t Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
This stack is towards supporting FDB Source User Meta. This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_switch_attr_t Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
Assign values to sai_fdb_entry_attr_t enum fields This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_fdb_entry_attr_t. Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
This stack is towards supporting FDB Source User Meta. This patch: opencomputeproject#1259 introduced explicit numbering to enum fields to avoid breaking warmboot across SAI spec enhancements while maintaining readability. It fixed saiacl.h and saihash.h. Introduce similar explicit numbering for enum sai_switch_attr_t Subsequent stacked commit needs to insert a new enum field in the middle. Signed-off-by: Shrikrishna Khare <skhare@fb.com>
Problem
Consider a scenario where an application (adapter host) using SAI spec X
(say) warmboots to an application using new SAI spec Y (say).
If an enum is extended between X and Y with fields inserted "in between"
rather than "at the end" of the enum, the old fields can get "renumbered".
However, the SAI implementation (adapter) continues to return old enums
for previously returned objects thereby breaking the warmboot.
Possible approaches
We discussed this during June 24, 2021 weekly SAI meeting and observed:
"at the end" (this could mean at the end of _END block). This will keep
the enum field values the same.
may be related to existing fields and thus for readability, those fields
belong togeter. Adding those to the end scatters the related fields
across the enum.
Solution
Thus, we agreed to go with the below approach:
available unused integer value would be used for the new insertions.
with spec revision thereby not breaking warmboot.
inserted "in between".
Those will be "fixed" by assigning integer values to enum fields.
This fix will be applied to top of the sai.git tree.
This fix will then be ported to 1.8.1 and 1.8.2 will be released.
Towards that end, these patches assigns integer values to enums in
saihash.h and saiacl.h that were changed from 1.6.5 to 1.8.1