[syncd][PFC WD]: Add support for plugin subscription by marian-pritsak · Pull Request #229 · sonic-net/sonic-sairedis

marian-pritsak · 2017-09-26T17:27:28Z

Calling lua scripts is moved from orchagent to syncd.
Client registers plugins so that later it can be notified of
certain events by those plugins

Signed-off-by: marian-pritsak marianp@mellanox.com

marian-pritsak · 2017-09-26T17:46:22Z

Still under development. Must be merged after corresponding changes in swss will be available

lguohan · 2017-09-27T07:12:12Z

conflict?

lguohan · 2017-09-27T07:14:43Z

lib/src/sai_redis_notifications.cpp

 sai_fdb_event_notification_fn               on_fdb_event = NULL;
 sai_port_state_change_notification_fn       on_port_state_change = NULL;
 sai_packet_event_notification_fn            on_packet_event = NULL;
+sai_queue_pfc_deadlock_notification_fn      on_queue_deadlock = NULL;


what if the real sai supports this notification natively?

Orchagent subscribes to sairedis notification for this callback like to any other SAI notification. The difference is that if SAI supports native notifications, then it will trigger event, and if we have .lua plugins for the platform, then syncd will run them and trigger same callback itself.

lguohan · 2017-09-27T08:05:21Z

syncd/pfc_detect_mlnx.lua

+
+        local m = table.getn(counter_keys)
+        for j = m, 1, -1 do
+            if string.sub(counter_keys[j],-string.len('RX_PKTS'))=='RX_PKTS' then


string.sub(counter_k [](start = 15, length = 20)

change to the exact match?

I don't know the exact name of stat ID, becasuse it is something like SAI_PORT_STAT_PFC_[i]_RX_PKTS

lguohan · 2017-09-27T08:54:04Z

if we let syncd to collect counter and then run the storm detection and then generate the notification, we do not really need to put the data into redis db, and use lua script to analyze the data.

the reason we have orchagent and syncd is to split the functionality, making syncd to only do simple jobs and most of bussiness logic in orchagent.

In the original design, the syncd is only pulling the counters and orchagent is handle all to detection logic. I think it should be still kept in this way.

after syncd pull the counters every 100ms, it should call the lua script (the lua script should be installed into db by swss process, not the syncd process). the lua script itself can generate notification to swss to handle, not via sai notification. Here the only thing syncd needs to do is to pull the counters and then call the lua script.

This will be easier for us the change the pfc storm detection algorithm if there is bug. In that case, we can remove the lua script (syncd will stop calling lua script since it does not exist), and install a new lua script. The orchagent needs to be restart, but not the syncd.

marian-pritsak · 2017-09-27T10:30:43Z

So basically we have 3 steps of execution here:

Collect counters from SAI;
Run verification algorithm;
Notify orchagent if something changes;

Step 1 must be done in syncd. Step 2 now is implemented as lua script called by syncd. Step 3 is now done by syncd.

If we want to change detection/restoration algorithm, we still can do it by changing lua without restarting any daemon. Why is it necessary to implement notifications inside lua scripts if I can reuse generic way of notifying orchagent?

About installation of lua script, I can keep it in swss, but it will require passing its SHA to syncd via Redis DB. If you're ok with it, I'll do it.

This commit implements native support for SAI_SWITCH_ATTRIBUTE_QUEUE_PFC_DEADLOCK_NOTIFY. Calling lua scripts is moved from orchagent to syncd, which uses native notification by a SW watchdog as well. Signed-off-by: marian-pritsak <marianp@mellanox.com>

lguohan

as comments

Signed-off-by: marian-pritsak <marianp@mellanox.com>

marian-pritsak · 2017-10-04T14:55:28Z

Depends on sonic-net/sonic-swss-common#134

marian-pritsak · 2017-10-05T07:45:47Z

retest this please

lguohan · 2017-10-05T07:53:20Z

syncd/Makefile.am

+
+dist_swss_DATA = \
+		 pfc_detect_mlnx.lua  \
+		 pfc_restore_mlnx.lua


do we still need these two files in syncd?

lguohan · 2017-10-05T08:19:34Z

can you separate native support for
SAI_SWITCH_ATTRIBUTE_QUEUE_PFC_DEADLOCK_NOTIFY in sairedis.

the code is completely in a different folder.

marian-pritsak · 2017-10-05T08:21:14Z

Sure, I will

lguohan · 2017-10-05T08:27:32Z

syncd/syncd.cpp

+        }
+        else
+        {
+            SWSS_LOG_ERROR("Object tyoe for not supported");


tyoe -> type

lguohan · 2017-10-05T08:43:17Z

syncd/syncd.cpp

+        const auto field = fvField(valuePair);
+        const auto value = fvValue(valuePair);
+
+        if (field != SAI_OBJECT_TYPE)


you do not need to reuse SAI_OBJECT_TYPE, this counter plugin has nothing to do with SAI. You can define your own field and value, like "type:queue", "type:port"

I will change in a follow-up PR, since it requires change in swss-common schema

lguohan

some minor changes, overall the code looks good to me.

lguohan · 2017-10-05T08:44:18Z

retest this please

lguohan · 2017-10-05T08:51:44Z

build failure

marian-pritsak · 2017-10-05T08:59:03Z

I need to remove .lua scripts from Makefile.am

Signed-off-by: marian-pritsak <marianp@mellanox.com>

kcudnik · 2017-10-05T11:42:12Z

why you need to remove lua scripts ?

marian-pritsak · 2017-10-05T11:42:43Z

They are part of swss

marian-pritsak · 2017-10-05T11:56:32Z

HW part moved to
#236

Signed-off-by: marian-pritsak <marianp@mellanox.com>

sihuihan88 · 2017-10-05T21:26:11Z

syncd/syncd_pfc_watchdog.cpp

    {
        collectCounters(countersTable);
+        runPlugins(db);



runPlugins and main syncd thread both access m_portCounterIdsMap and m_queueCounterIdsMap. Add lock?

can you confirm? they are protected by g_mutex lock.

runPlugins doesn't acquire the g_mutex lock.

sihuihan88 · 2017-10-05T21:27:48Z

syncd/syncd_pfc_watchdog.cpp

+
+    for (const auto& sha : m_portPlugins)
+    {
+        runRedisScript(db, sha, portList, argv);


Are we using this?

I think we do not use them, but does not hurt to have them.

Yes, currently no one uses port plugins

sihuihan88 · 2017-10-05T21:29:31Z

syncd/syncd_pfc_watchdog.cpp

+
+    for (const auto& sha : m_queuePlugins)
+    {
+        runRedisScript(db, sha, queueList, argv);


Where is the result of redis script being used?

syncd doesn't care about a result of a script. It only executes them to make changes in DB or to notify other modules.

sihuihan88 · 2017-10-05T21:30:31Z

syncd/syncd_pfc_watchdog.h

        static void removeQueue(
                _In_ sai_object_id_t queueVid);

+        static void addPortCounterPlugin(


Are we using addPortCounterPlugin?

no, we are not, but It seems good to have it for future use.

We don't use it now

sihuihan88 · 2017-10-05T21:34:13Z

syncd/syncd_pfc_watchdog.h


+        static void addPortCounterPlugin(
+                _In_ std::string sha);
+        static void addQueueCounterPlugin(


The name of the function is a little misleading. The plugin is not only for queue counters, it might also query port counters etc. Consider change the name to addPlugin, or addQueuePlugin, addPortPlugin if we need both

Type of plugin means what objects will be passed as arguments for execution. Of course, a client can register port counters for polling as well, but the plugin will only receive queue counters to do its logic.

sihuihan88 · 2017-10-05T21:39:40Z

syncd/syncd_pfc_watchdog.cpp

+
+    PfcWatchdog &wd = getInstance();
+
+    if (wd.m_portPlugins.find(sha) != wd.m_portPlugins.end() ||


Why also need to check the m_portPlugins here? If that's the case, why separate them in the first place?

Client is not allowed to register the exactly same plugin for different types. Later plugin will have no idea which objects were passed to it as arguments.

sihuihan88 · 2017-10-05T21:57:06Z

syncd/syncd.cpp

        auto idStrings  = swss::tokenize(value, ',');

-        if (field == PFC_WD_PORT_COUNTER_ID_LIST)
+        if (objectType == SAI_OBJECT_TYPE_PORT && field == PFC_WD_PORT_COUNTER_ID_LIST)


Why we need to determine the objectType here? Only field should be enough?

To make sure that client doesn't accidentally subscribe to the wrong object.

sihuihan88 · 2017-10-05T21:57:58Z

syncd/syncd.cpp

+            PfcWatchdog::setPortCounterList(vid, rid, portCounterIds);
        }
-        else if (field == PFC_WD_QUEUE_COUNTER_ID_LIST)
+        else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == PFC_WD_QUEUE_COUNTER_ID_LIST)


Why we need to determine the objectType here? Only field should be enough?

To make sure that client doesn't accidentally subscribe to the wrong object.

sihuihan88 · 2017-10-05T21:59:13Z

syncd/syncd.cpp

+
+        if (value == sai_serialize_object_type(SAI_OBJECT_TYPE_PORT))
+        {
+            PfcWatchdog::addPortCounterPlugin(key);


Are we going to use portcounterplugin?

No, at least not for now, because PFC WD works on queue level.

lguohan · 2017-10-06T03:20:51Z

syncd/syncd.cpp

+            }
+            else
+            {
+                SWSS_LOG_ERROR("Object tyoe for removal not supported");


tyoe -> type

here you also need continue.

I think it is better to do as follows, and it will be more clear.

if (op == DEL_COMMAND)
{
}
else if (op == SET_COMMAND)
{
}
else
{
}

Signed-off-by: marian-pritsak <marianp@mellanox.com>

stcheng force-pushed the master branch from ca5e43f to 9ce76e7 Compare September 26, 2017 23:15

lguohan reviewed Sep 27, 2017

View reviewed changes

lguohan requested changes Sep 30, 2017

View reviewed changes

[syncd_pfcwatchdog]: Add plugin subscription

0463f46

Signed-off-by: marian-pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the pfc-wd branch from 43f3bc9 to 0463f46 Compare October 4, 2017 14:48

lguohan reviewed Oct 5, 2017

View reviewed changes

lguohan requested changes Oct 5, 2017

View reviewed changes

[syncd]: Remove native notidications

61bcac8

Signed-off-by: marian-pritsak <marianp@mellanox.com>

marian-pritsak changed the title ~~[syncd][PFC WD]: Add support for SAI PFC storm notification~~ [syncd][PFC WD]: Add support for plugin subscription Oct 5, 2017

[syncd.cpp]: Fix typo

bec73e1

Signed-off-by: marian-pritsak <marianp@mellanox.com>

lguohan approved these changes Oct 5, 2017

View reviewed changes

sihuihan88 suggested changes Oct 5, 2017

View reviewed changes

lguohan reviewed Oct 6, 2017

View reviewed changes

Address comments

2c9669a

Signed-off-by: marian-pritsak <marianp@mellanox.com>

sihuihan88 approved these changes Oct 6, 2017

View reviewed changes

lguohan merged commit f43fb01 into sonic-net:master Oct 6, 2017

dzhangalibaba mentioned this pull request Sep 5, 2018

sonic-sairedis : moving AISC_DB to this new DB instance #343

Closed


		PfcWatchdog &wd = getInstance();

		if (wd.m_portPlugins.find(sha) != wd.m_portPlugins.end() \|\|

Conversation

marian-pritsak commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marian-pritsak commented Sep 26, 2017

Still under development. Must be merged after corresponding changes in swss will be available

Uh oh!

lguohan commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marian-pritsak Sep 27, 2017 • 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!

lguohan commented Sep 27, 2017

Uh oh!

marian-pritsak commented Sep 27, 2017

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

marian-pritsak commented Oct 4, 2017

Uh oh!

marian-pritsak commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 5, 2017

Uh oh!

marian-pritsak commented Oct 5, 2017

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!

marian-pritsak Oct 5, 2017 • 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!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 5, 2017

Uh oh!

lguohan commented Oct 5, 2017

Uh oh!

marian-pritsak commented Oct 5, 2017

Uh oh!

kcudnik commented Oct 5, 2017

Uh oh!

marian-pritsak commented Oct 5, 2017

Uh oh!

marian-pritsak commented Oct 5, 2017

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!

sihuihan88 Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

marian-pritsak commented Sep 26, 2017 •

edited

Loading

marian-pritsak Sep 27, 2017 •

edited

Loading

marian-pritsak Oct 5, 2017 •

edited

Loading

sihuihan88 Oct 5, 2017 •

edited

Loading

marian-pritsak Oct 6, 2017 •

edited

Loading