From bc8df0e6652a19a5d6fe69e8c2b0873eb8078318 Mon Sep 17 00:00:00 2001 From: Neetha John Date: Wed, 23 Dec 2020 08:43:10 -0800 Subject: [PATCH] Add support for headroom pool watermark (#1567) Signed-off-by: Neetha John Includes all the changes in #1453 along with fix for error msgs seen in syslog - Got invalid response type from redis 5 The error msg was due to incorrect return type in lua script. What I did Added 'SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES' in the interested counters to be queried Updated the the buffer lua script to update the headroom pool watermark counters Updated watermark orch to act on headroom pool clear request Why I did it To expose the headroom pool watermark counters in SONiC How I verified it On platforms with headroom pool support, verified that headroom pool watermark counters are getting updated as expected admin@sonic:~$ show headroom-pool persistent-watermark Headroom pool maximum occupancy: Pool Bytes --------------------- ------- ingress_lossless_pool 863616 On platforms without headroom pool support, headroom pool watermark counters show as N/A admin@sonic:~$ show headroom-pool persistent-watermark Headroom pool maximum occupancy: Pool Bytes --------------------- ------- ingress_lossless_pool N/A New sonic mgmt test added and it passed. Azure/sonic-mgmt#2614 Verified the error msg seen in syslog by enabling buffer pool watermark on vs docker. With the fix, build a new vs docker and verified that the msgs are no longer seen when buffer pool watermark is enabled. --- orchagent/bufferorch.cpp | 1 + orchagent/watermark_bufferpool.lua | 69 ++++++++++++++---------------- orchagent/watermarkorch.cpp | 45 ++++++++++++------- tests/test_watermark.py | 9 +++- 4 files changed, 72 insertions(+), 52 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index c722d7156e2d..2449dee113bf 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -23,6 +23,7 @@ extern sai_object_id_t gSwitchId; static const vector bufferPoolWatermarkStatIds = { SAI_BUFFER_POOL_STAT_WATERMARK_BYTES, + SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES }; type_map BufferOrch::m_buffer_type_maps = { diff --git a/orchagent/watermark_bufferpool.lua b/orchagent/watermark_bufferpool.lua index 2466ea79c251..56a5557f5c79 100644 --- a/orchagent/watermark_bufferpool.lua +++ b/orchagent/watermark_bufferpool.lua @@ -12,6 +12,7 @@ local persistent_table_name = 'PERSISTENT_WATERMARKS' local periodic_table_name = 'PERIODIC_WATERMARKS' local sai_buffer_pool_watermark_stat_name = 'SAI_BUFFER_POOL_STAT_WATERMARK_BYTES' +local sai_hdrm_pool_watermark_stat_name = 'SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES' local rets = {} @@ -21,42 +22,38 @@ redis.call('SELECT', counters_db) local n = table.getn(KEYS) for i = n, 1, -1 do -- Get new watermark value from COUNTERS - local wm = redis.call('HGET', counters_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) - if wm then - wm = tonumber(wm) - - -- Get last value from *_WATERMARKS - local user_wm_last = redis.call('HGET', user_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) - - -- Set higher value to *_WATERMARKS - if user_wm_last then - user_wm_last = tonumber(user_wm_last) - if wm > user_wm_last then - redis.call('HSET', user_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, wm) - end - else - redis.call('HSET', user_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, wm) - end - - local persistent_wm_last = redis.call('HGET', persistent_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) - if persistent_wm_last then - persistent_wm_last = tonumber(persistent_wm_last) - if wm > persistent_wm_last then - redis.call('HSET', persistent_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, wm) - end - else - redis.call('HSET', persistent_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, wm) - end - - local periodic_wm_last = redis.call('HGET', periodic_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) - if periodic_wm_last then - periodic_wm_last = tonumber(periodic_wm_last) - if wm > periodic_wm_last then - redis.call('HSET', periodic_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, wm) - end - else - redis.call('HSET', periodic_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, wm) - end + local buffer_pool_wm = redis.call('HGET', counters_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) + local hdrm_pool_wm = redis.call('HGET', counters_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name) + + -- Get last value from *_WATERMARKS + local user_buffer_pool_wm = redis.call('HGET', user_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) + local persistent_buffer_pool_wm = redis.call('HGET', persistent_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) + local periodic_buffer_pool_wm = redis.call('HGET', periodic_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name) + + local user_hdrm_pool_wm = redis.call('HGET', user_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name) + local persistent_hdrm_pool_wm = redis.call('HGET', persistent_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name) + local periodic_hdrm_pool_wm = redis.call('HGET', periodic_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name) + + if buffer_pool_wm then + buffer_pool_wm = tonumber(buffer_pool_wm) + + redis.call('HSET', user_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, + user_buffer_pool_wm and math.max(buffer_pool_wm, user_buffer_pool_wm) or buffer_pool_wm) + redis.call('HSET', persistent_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, + persistent_buffer_pool_wm and math.max(buffer_pool_wm, persistent_buffer_pool_wm) or buffer_pool_wm) + redis.call('HSET', periodic_table_name .. ':' .. KEYS[i], sai_buffer_pool_watermark_stat_name, + periodic_buffer_pool_wm and math.max(buffer_pool_wm, periodic_buffer_pool_wm) or buffer_pool_wm) + end + + if hdrm_pool_wm then + hdrm_pool_wm = tonumber(hdrm_pool_wm) + + redis.call('HSET', user_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name, + user_hdrm_pool_wm and math.max(hdrm_pool_wm, user_hdrm_pool_wm) or hdrm_pool_wm) + redis.call('HSET', persistent_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name, + persistent_hdrm_pool_wm and math.max(hdrm_pool_wm, persistent_hdrm_pool_wm) or hdrm_pool_wm) + redis.call('HSET', periodic_table_name .. ':' .. KEYS[i], sai_hdrm_pool_watermark_stat_name, + periodic_hdrm_pool_wm and math.max(hdrm_pool_wm, periodic_hdrm_pool_wm) or hdrm_pool_wm) end end diff --git a/orchagent/watermarkorch.cpp b/orchagent/watermarkorch.cpp index 9888df5fedee..8b8246ce35ed 100644 --- a/orchagent/watermarkorch.cpp +++ b/orchagent/watermarkorch.cpp @@ -13,6 +13,7 @@ #define CLEAR_QUEUE_SHARED_UNI_REQUEST "Q_SHARED_UNI" #define CLEAR_QUEUE_SHARED_MULTI_REQUEST "Q_SHARED_MULTI" #define CLEAR_BUFFER_POOL_REQUEST "BUFFER_POOL" +#define CLEAR_HEADROOM_POOL_REQUEST "HEADROOM_POOL" extern PortsOrch *gPortsOrch; extern BufferOrch *gBufferOrch; @@ -182,32 +183,38 @@ void WatermarkOrch::doTask(NotificationConsumer &consumer) if (data == CLEAR_PG_HEADROOM_REQUEST) { clearSingleWm(table, - "SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES", - m_pg_ids); + "SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES", + m_pg_ids); } else if (data == CLEAR_PG_SHARED_REQUEST) { clearSingleWm(table, - "SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES", - m_pg_ids); + "SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES", + m_pg_ids); } else if (data == CLEAR_QUEUE_SHARED_UNI_REQUEST) { clearSingleWm(table, - "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", - m_unicast_queue_ids); + "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", + m_unicast_queue_ids); } else if (data == CLEAR_QUEUE_SHARED_MULTI_REQUEST) { clearSingleWm(table, - "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", - m_multicast_queue_ids); + "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", + m_multicast_queue_ids); } else if (data == CLEAR_BUFFER_POOL_REQUEST) { clearSingleWm(table, - "SAI_BUFFER_POOL_STAT_WATERMARK_BYTES", - gBufferOrch->getBufferPoolNameOidMap()); + "SAI_BUFFER_POOL_STAT_WATERMARK_BYTES", + gBufferOrch->getBufferPoolNameOidMap()); + } + else if (data == CLEAR_HEADROOM_POOL_REQUEST) + { + clearSingleWm(table, + "SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES", + gBufferOrch->getBufferPoolNameOidMap()); } else { @@ -243,15 +250,23 @@ void WatermarkOrch::doTask(SelectableTimer &timer) } clearSingleWm(m_periodicWatermarkTable.get(), - "SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES", m_pg_ids); + "SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES", + m_pg_ids); + clearSingleWm(m_periodicWatermarkTable.get(), + "SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES", + m_pg_ids); clearSingleWm(m_periodicWatermarkTable.get(), - "SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES", m_pg_ids); + "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", + m_unicast_queue_ids); clearSingleWm(m_periodicWatermarkTable.get(), - "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", m_unicast_queue_ids); + "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", + m_multicast_queue_ids); clearSingleWm(m_periodicWatermarkTable.get(), - "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", m_multicast_queue_ids); + "SAI_BUFFER_POOL_STAT_WATERMARK_BYTES", + gBufferOrch->getBufferPoolNameOidMap()); clearSingleWm(m_periodicWatermarkTable.get(), - "SAI_BUFFER_POOL_STAT_WATERMARK_BYTES", gBufferOrch->getBufferPoolNameOidMap()); + "SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES", + gBufferOrch->getBufferPoolNameOidMap()); SWSS_LOG_DEBUG("Periodic watermark cleared by timer!"); } } diff --git a/tests/test_watermark.py b/tests/test_watermark.py index ee5dfd055e83..33e4154c895c 100644 --- a/tests/test_watermark.py +++ b/tests/test_watermark.py @@ -12,6 +12,7 @@ class SaiWmStats: pg_shared = "SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES" pg_headroom = "SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES" buffer_pool = "SAI_BUFFER_POOL_STAT_WATERMARK_BYTES" + hdrm_pool = "SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES" class WmTables: @@ -23,7 +24,7 @@ class WmTables: class WmFCEntry: queue_stats_entry = {"QUEUE_COUNTER_ID_LIST": SaiWmStats.queue_shared} pg_stats_entry = {"PG_COUNTER_ID_LIST": "{},{}".format(SaiWmStats.pg_shared, SaiWmStats.pg_headroom)} - buffer_stats_entry = {"BUFFER_POOL_COUNTER_ID_LIST": SaiWmStats.buffer_pool} + buffer_stats_entry = {"BUFFER_POOL_COUNTER_ID_LIST": "{},{}".format(SaiWmStats.buffer_pool, SaiWmStats.hdrm_pool)} class TestWatermark(object): @@ -80,6 +81,7 @@ def populate_asic_all(self, dvs, val): self.populate_asic(dvs, "SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP", SaiWmStats.pg_shared, val) self.populate_asic(dvs, "SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP", SaiWmStats.pg_headroom, val) self.populate_asic(dvs, "SAI_OBJECT_TYPE_BUFFER_POOL", SaiWmStats.buffer_pool, val) + self.populate_asic(dvs, "SAI_OBJECT_TYPE_BUFFER_POOL", SaiWmStats.hdrm_pool, val) time.sleep(self.DEFAULT_POLL_INTERVAL) def verify_value(self, dvs, obj_ids, table_name, watermark_name, expected_value): @@ -181,6 +183,7 @@ def test_telemetry_period(self, dvs): self.verify_value(dvs, self.pgs, WmTables.periodic, SaiWmStats.pg_headroom, "0") self.verify_value(dvs, self.qs, WmTables.periodic, SaiWmStats.queue_shared, "0") self.verify_value(dvs, self.buffers, WmTables.periodic, SaiWmStats.buffer_pool, "0") + self.verify_value(dvs, self.buffers, WmTables.periodic, SaiWmStats.hdrm_pool, "0") self.populate_asic_all(dvs, "123") @@ -193,6 +196,7 @@ def test_telemetry_period(self, dvs): self.verify_value(dvs, self.pgs, WmTables.periodic, SaiWmStats.pg_headroom, "0") self.verify_value(dvs, self.qs, WmTables.periodic, SaiWmStats.queue_shared, "0") self.verify_value(dvs, self.buffers, WmTables.periodic, SaiWmStats.buffer_pool, "0") + self.verify_value(dvs, self.buffers, WmTables.periodic, SaiWmStats.hdrm_pool, "0") finally: self.clear_flex_counter(dvs) @@ -214,6 +218,7 @@ def test_lua_plugins(self, dvs): self.verify_value(dvs, self.selected_pgs, table_name, SaiWmStats.pg_headroom, "192") self.verify_value(dvs, self.selected_pgs, table_name, SaiWmStats.pg_shared, "192") self.verify_value(dvs, self.buffers, table_name, SaiWmStats.buffer_pool, "192") + self.verify_value(dvs, self.buffers, table_name, SaiWmStats.hdrm_pool, "192") self.populate_asic_all(dvs, "96") @@ -222,6 +227,7 @@ def test_lua_plugins(self, dvs): self.verify_value(dvs, self.selected_pgs, table_name, SaiWmStats.pg_headroom, "192") self.verify_value(dvs, self.selected_pgs, table_name, SaiWmStats.pg_shared, "192") self.verify_value(dvs, self.buffers, table_name, SaiWmStats.buffer_pool, "192") + self.verify_value(dvs, self.buffers, table_name, SaiWmStats.hdrm_pool, "192") self.populate_asic_all(dvs, "288") @@ -230,6 +236,7 @@ def test_lua_plugins(self, dvs): self.verify_value(dvs, self.selected_pgs, table_name, SaiWmStats.pg_headroom, "288") self.verify_value(dvs, self.selected_pgs, table_name, SaiWmStats.pg_shared, "288") self.verify_value(dvs, self.buffers, table_name, SaiWmStats.buffer_pool, "288") + self.verify_value(dvs, self.buffers, table_name, SaiWmStats.hdrm_pool, "288") finally: self.clear_flex_counter(dvs)