Add a RIF attribute to specify if the corresponding My MAC entry should not be created..

[erohsik]

Add an attribute used to specify if My MAC entry need not be created for this {port, vlan, MAC address}.

[erohsik]

@itaibaz
I believe you had commented during the call.. Does this sound reasonable?

/**
 * @brief Attribute used to specify external My MAC entry that will
 * be used in place of any implicit entry created during RIF processing
 * for this {port, vlan, MAC address}
 *
 * @type sai_object_id_t
 * @flags CREATE_ONLY
 * @objects SAI_OBJECT_TYPE_MY_MAC
 * @allownull true
 * @default SAI_NULL_OBJECT_ID
 */
SAI_ROUTER_INTERFACE_ATTR_MY_MAC,

[itaibaz]

Hi, the comment must have been from someone else, not me

[tjchadaga]

@itaibaz, @rck-innovium, @JaiOCP - could you please help review?

[tjchadaga]

@erohsik - please help resolve conflicts on the branch

[erohsik]

@erohsik - please help resolve conflicts on the branch

Done...

[tjchadaga]

@rck-innovium, @JaiOCP - could you please help sign-off?

[rck-innovium]

@erohsik  

1. If SAI_ROUTER_INTERFACE_ATTR_MY_MAC is not null, does it mean that the device must not create a termination entry with MAC == SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS?
2. What is the meaning of binding a RIF to MY_MAC entry? Does it mean that the MY_MAC entry takes effect only for packets that ingress on this RIF? I am trying to see how it relates to SAI_MY_MAC_ATTR_PORT/VLAN_ID
3. And lastly, why not achieve this by having a SAI_ROUTER_INTERFACE_ATTR_NO_TERM_MAC of type boolean to achieve the goal listed in the heading.

[erohsik]

1. Yes, the device will skip creating a termination entry with the RIF SMAC
   2, 3. A bool attribute was in the original change, but during the PR review, a suggestion was made to add the corresponding MY_MAC entry so that the MY_MAC entry does not get deleted inadvertently while the RIF is still relying on it for L3 forwarding determination.

[rck-innovium]

Thanks. I prefer your original model over the suggestion. I think all NOS would anyway maintain this reference counting;  we can avoid adding unnecessary state in the SAI implementation.

 Further, the suggested model gives a impression that the pointed to MyMAC entry should only be used by the RIFs that point to this MyMAC entry and vice versa. Today, one RIF can be terminated by more than one MyMAC entries. One MyMAC entry (if we skip SAI_MY_MAC_ATTR_PORT/VLAN_ID) can terminate multiple RIFs.

@JaiOCP @itaibaz Can you please give your feedback on the above. 

Today, SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS is create-and-set, however SAI_ROUTER_INTERFACE_ATTR_MY_MAC is create-only. Any reasons?

[JaiOCP]

@rck-innovium Even with this proposal many RIF can point to the same my mac object, right?

[rck-innovium]

The requirement, as I understand it, is that the NOS needs a way to control whether a RIF-specific Router MAC entry should be created or not. Today, it is created by default. This is an useful feature to have and the original proposal of having a boolean attribute was clear. The modification from boolean to OID of type RIF is ambiguous for the below reasons.

Background

SAI has a separate My MAC entry that is added independently of the RIF. This entry can be either MAC (with mask) only or can include Port/VLAN. So, it is not necessarily restricted to a particular RIF. It can match any of the packets coming into the switch. IIRC, one of the common reasons to use this was to scale the number of RIFs that can be created without having to burn separate per-RIF router MAC entries.
SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS will still be set and is to be used as the Source MAC of packets routed out of this RIF.

Example

Now, with the modified proposal, consider the example below:

Step 1: MyMAC objects created

OID	MAC	PORT_ID	VLAN_ID	Priority
MyMAC1	00:01:02:03:04:05	Port1	0	100
MyMAC2	00:01:02:03:04:06	NULL	0	1

Step 2: RIF objects created

RIF#	SAI_ROUTER_INTERFACE_ATTR_MY_MAC
RIF1	MyMAC1
RIF2	MyMAC2
RIF3	MyMAC2
RIF4	MyMAC1

Ambiguities

The behavior of MyMAC is independent of RIF

As per the MyMAC entry definition (PR1243), packets coming in on RIF1 with DMAC as 00:01:02:03:04:06 (MyMAC2) will still get matched even when RIF1's SAI_ROUTER_INTERFACE_ATTR_MY_MAC is only pointing to MyMAC1. But the configuration above is giving a wrong impression that MyMAC2 is applicable only for pkts coming in on RIF2 and RIF3, not for pkts on RIF1.

Application can still create inconsistencies

A comment given for this PR (PR2021) was to make the new RIF attribute a MyMAC OID instead of it being a bool. This was to avoid inconsistencies like the MyMAC entry getting deleted when a RIF is relying on it.
Say in the above example, RIF4 is of type SAI_ROUTER_INTERFACE_TYPE_PORT and is created on Port2. Then RIF4 has inconsistent programming since MyMAC1 only matches packets coming in on Port1.

Other Drawbacks

A RIF can have more than one MyMAC entry

In the current SAI model, a RIF can have more than one MyMAC entry. So, SAI_ROUTER_INTERFACE_ATTR_MY_MAC needs to be an OID List rather than an OID.

[erohsik]

Thanks for the detailed review comments..

I think the scenario you mentioned may also happen today without MY_MAC objects..
Say we have
Sub port RIF with port=p1,vlan=x, mac=m1
VLAN RIF with vlan=x, mac=m2
and the SAI implementation wild cards the port when installing entries for the VLAN RIF into the hw table used to determine if the packet can be L3 forwarded..
Packets on port p1, vlan=x, mac=m2 can then be considered for L3 forwarding?

If we go with the OID list approach, each time a new applicable MY_MAC object is added, the OID list needs to be updated?

During the PR discussion on 6/20/24, a suggestion was made to have this attribute reference the MY_MAC object instead of the bool attribute. This was also supported by other vendors..

[rck-innovium]

I believe that the proposed SAI_ROUTER_INTERFACE_ATTR_MY_MAC should be of type bool (an hint to the SAI implementation to not create RIF specific termination MAC addresses) rather than SAI MyMAC object. Otherwise, it confuses the semantics of SAI MyMAC objects as explained in my comment earlier.

At the least, please clarify this in the comments for the proposed attribute.

    /**
     * @brief Attribute used to specify external My MAC entry that will
     * be used in place of any implicit entry created during RIF processing
     * for this {port, vlan, MAC address}
     *     
 +   * Packets received on the RIF with DMAC matching any My MAC table entry 
 +   *  (not necessarily this particular MY_MAC entry) will be routed.
     *
 +   * Packets routed out of the RIF will use the source MAC from SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS if set, 
 +   *  otherwise from SAI_VIRTUAL_ROUTER_ATTR_SRC_MAC_ADDRESS, and if neither is configured,
 +   *  from the SAI_SWITCH_ATTR_SRC_MAC_ADDRESS.
     *
     * @type sai_object_id_t
     * @flags CREATE_ONLY
     * @objects SAI_OBJECT_TYPE_MY_MAC
     * @allownull true
     * @default SAI_NULL_OBJECT_ID
     */
    SAI_ROUTER_INTERFACE_ATTR_MY_MAC,

[erohsik]

Thanks for the suggestion.. How about this update?

@@ -320,6 +320,9 @@ typedef enum _sai_router_interface_attr_t
      * @brief Attribute used to specify external My MAC entry that will
      * be used in place of any implicit entry created during RIF processing
      * for this {port, vlan, MAC address}
+     * Note that the other matching entries (programmed via RIF and/or My MAC)
+     * can allow the incoming packets to be considered for L3 forwarding.
+     * There is no change in the behavior for packets egressing the RIF.
      *
      * @type sai_object_id_t
      * @flags CREATE_ONLY

[rck-innovium]

Looks good, please do the above changes.

[gnaaman-dn]

Hey @erohsik, I'm curious about @rck-innovium question which I think was left unanswered.

Today, SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS is create-and-set, however SAI_ROUTER_INTERFACE_ATTR_MY_MAC is create-only. Any reasons?

Is there a reason for this attribute to not be CREATE_AND_SET?
Thanks in advance

[erohsik]

@rck-innovium, @JaiOCP - could you please help sign-off?

Can you please review? Thanks!

[rck-innovium]

Please update the comments as discussed.

[erohsik]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 2021 in repo opencomputeproject/SAI

[erohsik]

Please update the comments as discussed.

Thanks.. Updated the comments.. please take a look.

[tjchadaga]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tjchadaga]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kcudnik]

please fix errors, your brief message is too long

[tjchadaga]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).