sFlow Phase Three Sairedis Changes - #2004
Conversation
Signed-off-by: Ritvik Uppal <rituppal@cisco.com>
Signed-off-by: Ritvik Uppal <rituppal@cisco.com>
Signed-off-by: Ritvik Uppal <rituppal@cisco.com>
Signed-off-by: Ritvik Uppal <rituppal@cisco.com>
Signed-off-by: Ritvik Uppal <rituppal@cisco.com>
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
8e64b6b to
34073a9
Compare
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
vslib/vpp/SwitchVppSflow.cpp:262
rateis auint32_t, but the log message uses%dwhich is a signed int format specifier. This can trigger-Wformatwarnings/errors and may print incorrect values for large rates.
SWSS_LOG_NOTICE("Changed sampling rate to 1-in-%d for port %s", rate, sai_serialize_object_id(port_id).c_str());
vslib/vpp/SwitchVppRif.cpp:609
UpdatePort()can callsflowPortSamplePacketSet()separately for ingress and egress when both attributes are present (e.g., duringcreatePort()which callsUpdatePort()beforecreate_internal()). BecausesflowPortSamplePacketSet()fetches the opposite direction viaget(SAI_OBJECT_TYPE_PORT, ...), that lookup can fail/not reflect the in-flight attribute list, causing the second call to overwrite the first and leaving the port programmed for only one direction instead of the combined mask.
attr_type = sai_metadata_get_attr_by_id(SAI_PORT_ATTR_EGRESS_SAMPLEPACKET_ENABLE, attr_count, attr_list);
if(attr_type != NULL)
{
sflowPortSamplePacketSet(object_id, attr_type);
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
vslib/vpp/vppxlate/SaiVppXlate.c:3275
- The error message here refers to "hw_index", but the function argument is hwif_name. Updating the message makes debugging clearer (and matches the actual validation).
} else {
SAIVPP_ERROR("No hw_index provided");
VPP_UNLOCK();
return -EINVAL;
}
vslib/vpp/SwitchVppSflow.cpp:229
- When both ingress and egress samplepacket enables are set to different SAMPLEPACKET objects (and those objects have different sample rates), this code silently programs VPP with only the rate from the just-updated direction. Since VPP supports only one per-interface sampling rate, it should at least detect and warn about mismatched ingress/egress rates (and apply a clear policy such as last-writer-wins), otherwise configuration can be confusing and appear to be ignored.
sai_object_id_t active_oid = updated_oid != SAI_NULL_OBJECT_ID ? updated_oid : other_oid;
auto serialized_id = sai_serialize_object_id(active_oid);
sai_attribute_t rate_attr{};
vslib/vpp/SwitchVppSflow.cpp:262
- This log uses %d for a uint32_t sampling rate, which can print incorrectly and triggers format warnings on some toolchains. Use %u for unsigned rates.
SWSS_LOG_NOTICE("Changed sampling rate to 1-in-%d for port %s", rate, sai_serialize_object_id(port_id).c_str());
vslib/vpp/vppxlate/SaiVppXlate.c:3233
- The error message here refers to "hw_index", but the function argument is hwif_name. Updating the message makes debugging clearer (and matches the actual validation).
This issue also appears on line 3271 of the same file.
} else {
SAIVPP_ERROR("No hw_index provided");
VPP_UNLOCK();
return -EINVAL;
vslib/vpp/SwitchVppRif.cpp:610
- UpdatePort currently ignores the return status from sflowPortSamplePacketSet, so a failure to program VPP (or to fetch the sample rate) will be silently dropped and the port update will still report success. Since this PR adds more per-port sFlow programming, it would be better to propagate failures so callers can react appropriately.
attr_type = sai_metadata_get_attr_by_id(SAI_PORT_ATTR_EGRESS_SAMPLEPACKET_ENABLE, attr_count, attr_list);
if(attr_type != NULL)
{
sflowPortSamplePacketSet(object_id, attr_type);
}
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
vslib/vpp/vppxlate/SaiVppXlate.c:3274
- Same issue as above: the log text says "hw_index" and omits newline/period. Keeping this consistent with existing "No hwif_name provided.\n" messages will improve operational debugging.
}
} else {
SAIVPP_ERROR("No hw_index provided");
VPP_UNLOCK();
return -EINVAL;
vslib/vpp/SwitchVppRif.cpp:609
- Calling sflowPortSamplePacketSet() separately for ingress and egress in the same UpdatePort() invocation can program the wrong combined direction. UpdatePort() runs before set_internal/create_internal, so sflowPortSamplePacketSet() falls back to get(SAI_OBJECT_TYPE_PORT, ...) and will not see the other direction when both attributes are supplied together (e.g., during createPort). The second call can therefore overwrite the first and leave only the last-updated direction enabled.
attr_type = sai_metadata_get_attr_by_id(SAI_PORT_ATTR_EGRESS_SAMPLEPACKET_ENABLE, attr_count, attr_list);
if(attr_type != NULL)
{
sflowPortSamplePacketSet(object_id, attr_type);
vslib/vpp/vppxlate/SaiVppXlate.c:3234
- This error message is inconsistent with the rest of SaiVppXlate.c (it refers to "hw_index" even though the argument is hwif_name, and it omits the trailing newline/period used elsewhere), which makes logs harder to grep and compare.
This issue also appears on line 3270 of the same file.
}
} else {
SAIVPP_ERROR("No hw_index provided");
VPP_UNLOCK();
return -EINVAL;
vslib/vpp/SwitchVppSflow.cpp:261
- rate is a uint32_t, but this log uses %d (signed int). Using the correct %u format avoids misleading logs and printf-format warnings on some toolchains.
SWSS_LOG_NOTICE("Changed sampling rate to 1-in-%d for port %s", rate, sai_serialize_object_id(port_id).c_str());
Signed-off-by: Ritvik Uppal <rituppal@cisco.com>
a88194c to
d09263e
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Description of PR
Summary:
This PR contains the changes made to sonic-sairedis regarding phase 3 of sFlow. Specifically, it adds support for per-port sampling rate and per-port direction.
Fixes # (issue)
Type of change
Approach
What is the motivation for this PR?
The motivation is to add support to SONiC-VPP so that every port can have its own sampling rate and direction.
Work item tracking
How did you do it?
In
SwitchVppSflow.cpp,sflowPortSamplePacketSet()now translates theINGRESS/EGREE_SAMPLEPACKET_ENABLEsets into per-port VPP config. It reads the SAMPLEPACKET rate, combines ingress/egress into one direction mask, and programs the port viasflowInterfaceSamplingRateSet(),sflowInterfaceDirectionSet(), andsflowEnableDisable(). InSaiVppXlate.cI added the client send functions for those per-interface APIs plus their reply handlers and registrations, without the handlers the calls hung until syncd's 30s watchdog and interfaces never armed. Together these let per-port rate and direction flow from SONiC through SAI into VPP.How did you verify/test it?
My testing environment consisted of two routers who were wired together. r1 is the traffic sender and r2 is the collector that had sFlow running. I configured Ethernet4 and Ethernet8 on both routers:
r1:
r2:
The HLD has 3 exit criteria conditions that must be met.
I configured Ethernet4 to have a sampling rate of 1000 and Ethernet8 to have a sampling rate of 5000:

I then sent each port 20 000 packets:

Here is how much each port sampled:

Ethernet4 sampled 24 of 20,000 packets and Ethernet8 sampled 5 of 20,000 packets, consistent with their configured rates of 1-in-1000 (≈20 expected) and 1-in-5000 (≈4 expected). sFlow sampling is statistical, not a strict every-Nth count, so the per-port totals scatter around the expected value rather than matching it exactly. The rate is calculated through a per-interface skip counter: it is drawn as a random value in the range [1, 2N-1] and counts down as packets pass through the port. When it reaches zero the packet is sampled and a new skip is calculated. Because the mean skip equals N, the interface samples 1-in-N on average while randomizing which packets are selected.
sflow directionsetting does not override either.For this criteria, I configured Ethernet4 to be ingress only and Ethernet8 to be both, I also set the global direction to be egress, to test if the global direction will be over-ridden by per-port direction:

I then sent each port 40 000 packets:

The results were:

Each port is behaving as expected, Ethernet4 is sampling only packets on ingress and Ethernet8 is sampling packets in both directions.
samplingNandskipvisible in showsflow per-port output.docker exec -it syncd vppctl show sflowreturns the following:This command prints the global sFlow settings followed by a per-interface block for each enabled port. Each block shows the interface's effective sampling-rate (its per-port
samplingN, or the global rate if none is set) and, for every VPP thread, that thread's live skip counter — the number of packets remaining until the next sample. In this run Ethernet4 (bobm1) reports sampling-rate 1000 and Ethernet8 (bobm2) reports sampling-rate 5000, each with a per-thread skip in the [1, 2N-1] range, confirming that per-port sampling state is now observable directly from the data plane.Any platform specific information?
Documentation