[copporch] Add submit to ingress port for packet outs #1952
Conversation
|
cc: @mint570 |
orchagent/copporch.h
Outdated
| @@ -1,3 +1,19 @@ | |||
| /* | |||
There was a problem hiding this comment.
Please remove this copyright sections
There was a problem hiding this comment.
@mint570 Can we remove the Google copyright headers from these files? There is an Apache license file at the root of the repo and the convention is to not have these headers on every file.
There was a problem hiding this comment.
Sorry for the late reply.
Yes, please remove the copyright header from the existing SONiC code.
(I think even for the P4Orch code, we can remove the copyright header.)
We put the copyright header before we got the approval for upstreaming.
|
@kperumalbfn Please take a look at this change |
| attr.value.booldata = true; | ||
| ingress_attribs.push_back(attr); | ||
|
|
||
| // Get CPU port object id to signal submit to ingress |
There was a problem hiding this comment.
We can get cpu_port from portsOrch method getCpuPort. Can we use that?
|
|
||
| if (!ingress_attribs.empty()) | ||
| { | ||
| if (!createSubmitToIngressHostIf(ingress_attribs)) |
There was a problem hiding this comment.
Do we need a new hostif for every copp rule? Based on the packet io HLD, we need one hostif for CPU TX packets.
There was a problem hiding this comment.
The CPU interface is being created in response to processing of the "submit to ingress" CoPP table entry specified in copp_cfg.j2:
https://github.com/Azure/sonic-buildimage/pull/9084/files
On similar lines as the GENL interfaces.
| sai_status); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add the platform check for this, may not be needed for all the vendors? SAI attribute SAI_HOSTIF_ATTR_OPER_STATUS should take care of bringing up the hostif interface and avoid ioctl calls from orchagent.
There was a problem hiding this comment.
Makes sense. This can be handled by vendor code.
|
|
||
| // bind it to the netdev by name | ||
| len = strnlen(ifname, IFNAMSIZ); | ||
| setsockopt(sock_fd, SOL_SOCKET, SO_BINDTODEVICE, ifname, (socklen_t)len); |
There was a problem hiding this comment.
We haven't had any socket binding/kernel interactions from orchagent context. Need to discuss this
There was a problem hiding this comment.
@vivekmoorthy can you take a look?
There was a problem hiding this comment.
Ack. Started looking into this and will address the comments in a few days.
There was a problem hiding this comment.
Vivek is working on having the SAI layer bring the interface up.
There was a problem hiding this comment.
As we discussed, I think we can expect the the vendor SAI to handle the SAI_HOSTIF_ATTR_OPER_STATUS to bring up the interface, and this workaround block could be removed from orchagent.
|
@vivekmoorthy please follow up with reviewers |
Submission containing materials of a third party:
Copyright Google LLC; Licensed under Apache 2.0
Co-authored-by: Glenn Connery <gconnery@google.com>
Co-authored-by: Yilan Ji <yilanji@google.com>
Co-authored-by: Runming Wu <runmingwu@google.com>
Signed-off-by: Don Newton don@opennetworking.org
What I did Wrong port speed is set if included as a substring in a speed literal. For example if we have a valid supported speeds for a port with 4 lanes: 40000 and 100000. Then if we try to set 4000. sudo config interface speed Ethernet0 4000 As a result, we will get a wrong set speed 4G: dut:~$ show interfaces status Interface Lanes Speed MTU FEC Alias Vlan Oper Admin Type Asym PFC --------- ------ ------ ---- ---- -------- ----- ----- ------ ----------- ----- Ethernet0 0,1,2,3 4G 9100 rs Ethernet1/1 routed up up QSFP28 or later off How I did it Fixed finding the target (set) speed in the supported speeds list. How to verify it Set the wrong speed, which included as a substring in a speed literal. For example, for a speed 40000 it is 4000. If the wrong speed is set, an error message should appear: admin@sonic:~$ sudo config interface speed Ethernet0 4000 Invalid speed specified: 4000 Valid speeds:40000,100000 Only speeds from the list of "Valid speeds" have been successfully set. sudo config interface speed Ethernet0 40000 Signed-off-by: Mykola Gerasymenko <mykolax.gerasymenko@intel.com>
Submission containing materials of a third party:
Copyright Google LLC; Licensed under Apache 2.0
Co-authored-by: Glenn Connery gconnery@google.com
Co-authored-by: Yilan Ji yilanji@google.com
Co-authored-by: Runming Wu runmingwu@google.com
Signed-off-by: Don Newton don@opennetworking.org
What I did
This commit adds support for a submit to ingress netdev port. This port uses the ASIC ingress pipeline to determine the destination port for the packet. For more details, see the “Transmit Path” section of PINS Packet IO HLD: https://github.com/pins/SONiC/blob/p4rt_packetio/doc/pins/Packet_io.md#transmit-path
Why I did it
PINS controllers sometimes want to verify the state of the ingress pipeline using a packet out to this new port.
How I verified it
The PINS system test framework has automated tests to validate this behavior. It will be upstreamed in a later pull request.