vslib port counter support by vishnushetty · Pull Request #1274 · sonic-net/sonic-sairedis

vishnushetty · 2023-08-15T05:01:27Z

In sonic-vs the 'show interface counters' is not supported (port counters). This PR adds support for it. This would be useful in VS debugging and automation. The port counters are fetched from corresponding host interface, not supported counters set to zero as earlier.

kcudnik · 2023-08-15T20:36:02Z

vslib/SwitchState.cpp

+        _In_ std::string& if_name, 
+        _Out_ uint64_t &counter)
+{
+    struct {


please declare struct in separate file or in *.h file of SwithcState

kcudnik · 2023-08-15T20:36:50Z

vslib/SwitchState.cpp

+    struct {
+        sai_stat_id_t id;
+        std::string name;
+    } counter_map [] = {


this map also should be declared in SwithcState.h and ssigned in cpp file as const, no need to delcare it in function

ok. replacing with std::map.

kcudnik · 2023-08-15T20:37:34Z

vslib/SwitchState.cpp

+    uint32_t i;
+    sai_status_t rc = SAI_STATUS_SUCCESS;
+
+    for(i = 0; i < counter_map_size; i++)


pleae keep code quality, add space after if, empty line before if and after }, delcare uint_32 i inside for, this is not C

kcudnik · 2023-08-15T20:37:53Z

vslib/SwitchState.cpp

+        if(counter_map[i].id == counter_id)
+        {
+            sprintf(cmd, "/bin/cat /sys/class/net/%s/statistics/%s", if_name.c_str(), counter_map[i].name.c_str()); 
+            fp = popen(cmd, "r");


declare FILE here not at the top of funcion

kcudnik · 2023-08-15T20:38:16Z

vslib/SwitchState.cpp

+        { SAI_PORT_STAT_IF_OUT_ERRORS, "tx_errors" },
+        { SAI_PORT_STAT_IF_OUT_DISCARDS, "tx_dropped" }
+    };
+    uint32_t counter_map_size = 8;


this should be obtained automatically, if you declare this entries as std::vector

replaced with std::map no size

Let me know how to make map const? for now i made it just private initialized in constructor. Do you have an example for const map?

try:
static const std::map<x,y> m_map;
and it must be initialized in cpp file take a look here: https://stackoverflow.com/questions/2636303/how-to-initialize-a-private-static-const-map-in-c
const std::map<x,y>

kcudnik

please address comments

kcudnik · 2023-08-15T20:39:22Z

vslib/SwitchState.cpp

+        if(counter_map[i].id == counter_id)
+        {
+            sprintf(cmd, "/bin/cat /sys/class/net/%s/statistics/%s", if_name.c_str(), counter_map[i].name.c_str()); 
+            fp = popen(cmd, "r");


it's preffered to use isostream, you don't need to worry about closing file, iostream will close that when will get out of scope, and you cen return errors directly

is there any c++ stream for this purpose? This is not efficient but dont know alternate option for now.

https://en.cppreference.com/w/cpp/io/basic_ifstream

It didn't work. Will go with popen for now.

std::string filename = "/sys/class/net/" + ifName + "/statistics/" + counterName; std::ifstream istrm(filename, std::ios::in); SWSS_LOG_NOTICE("counter id found %d name %s", counterId, counterName.c_str()); if (!istrm.is_open()) { SWSS_LOG_ERROR("failed to open %s", filename.c_str()); counter = -1; return SAI_STATUS_FAILURE; } else { if (istrm.read(reinterpret_cast<char*>(&counter), sizeof counter)) // binary input { SWSS_LOG_NOTICE("counter id found %d name %s val %lu", counterId, counterName.c_str(), counter); } } }

read not returning right values

have you tries using ">>" operator ? istrem >> counter;

kcudnik · 2023-08-15T20:39:56Z

vslib/SwitchState.cpp

+    /* zero out counters */
+    counter = 0;
+
+    if(getTapNameFromPortId(port_id, if_name) == false)


keep code quality, add space after each if

kcudnik · 2023-08-15T20:40:13Z

vslib/SwitchState.cpp

+        {
+            if(getNetStat(counter_id, if_name, counter) != SAI_STATUS_SUCCESS)
+            {
+                SWSS_LOG_ERROR("Port stat get failed %s", if_name.c_str()); 


should you return error after this ?

kcudnik · 2023-08-15T20:40:43Z

vslib/SwitchState.cpp

+    {
+        if(counter_map[i].id == counter_id)
+        {
+            sprintf(cmd, "/bin/cat /sys/class/net/%s/statistics/%s", if_name.c_str(), counter_map[i].name.c_str()); 


use std string to concat this to not deal wit buffers sizes

kcudnik · 2023-08-15T20:41:10Z

vslib/SwitchState.cpp

+            /* In non unit test mode fetch port counters from host interface */
+            if (!enabled && (object_type == SAI_OBJECT_TYPE_PORT))
+            {
+                getPortStat(object_id, id, counter);


you don't check getPortStat error code

added rc check

vishnushetty · 2023-08-16T03:12:25Z

/easycla

kcudnik · 2023-08-16T09:26:58Z

vslib/SwitchState.cpp

+        _In_ sai_object_id_t port_id,
+        _In_ const sai_stat_id_t counter_id,


keep also naming in camel case, portId, counterId

kcudnik · 2023-08-16T09:27:06Z

vslib/SwitchState.cpp

+        _In_ const sai_stat_id_t counter_id,
+        _Out_ uint64_t &counter)
+{
+    std::string if_name;


use camel case ifName

kcudnik · 2023-08-16T09:29:51Z

vslib/SwitchState.h

+            sai_status_t getPortStat(
+                    _In_ sai_object_id_t port_id, 
+                    _In_ const sai_stat_id_t counter_id, 
+                    _Out_ uint64_t &counters);
+
+           sai_status_t getNetStat(
+                   _In_ sai_stat_id_t counter_id, 
+                   _In_ std::string& if_name, 
+                   _Out_ uint64_t &counter);
+


those methods can be protected, no need to make them public

made it private

kcudnik · 2023-08-16T09:30:48Z

vslib/SwitchState.h

+           sai_status_t getNetStat(
+                   _In_ sai_stat_id_t counter_id, 
+                   _In_ std::string& if_name, 
+                   _Out_ uint64_t &counter);


if you are using reference, use & on the left of the param like in case of if_name, also use camelCase, ifName, counterId etc

vishnushetty · 2023-08-17T17:05:38Z

I will close this PR, raise on wrong fork. Use below PR
#1275
for further comments.

vslib port counter support

2c5abd4

kcudnik reviewed Aug 15, 2023

View reviewed changes

kcudnik requested changes Aug 15, 2023

View reviewed changes

kcudnik reviewed Aug 16, 2023

View reviewed changes

sonic-net deleted a comment from linux-foundation-easycla bot Aug 17, 2023

vishnushetty mentioned this pull request Aug 17, 2023

port counter support on sonic-vs #1275

Merged

vishnushetty closed this Aug 17, 2023

		_In_ sai_object_id_t port_id,
		_In_ const sai_stat_id_t counter_id,

Conversation

vishnushetty commented Aug 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishnushetty Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcudnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishnushetty Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishnushetty commented Aug 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishnushetty commented Aug 17, 2023

Uh oh!

vishnushetty Aug 15, 2023 •

edited

Loading

vishnushetty Aug 16, 2023 •

edited

Loading