Skip to content

Commit

Permalink
Assign values to enum fields (opencomputeproject#1259)
Browse files Browse the repository at this point in the history
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>

* 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':
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>
  • Loading branch information
shri-khare authored Aug 26, 2021
1 parent af037ad commit dfaa98c
Show file tree
Hide file tree
Showing 2 changed files with 322 additions and 322 deletions.
Loading

0 comments on commit dfaa98c

Please sign in to comment.