Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fdb_flush.lua executes so long leading to REDIS BUSY #1397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fdb_flush.lua executes so long leading to REDIS BUSY #1397

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 40 additions & 11 deletions syncd/RedisClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,17 +893,46 @@ void RedisClient::processFlushEvent(
SWSS_LOG_THROW("unknown fdb flush entry type: %d", type);
}

for (int flush_static: vals)
// If has a lot of macs(example:217600) and use lua scripts, will cause REDIS BUSY.
// Change to this without atomicity operation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atomic here would be desired sine it could lead to some wired issues while new Mac is learned during deletion

const auto &keys = m_dbAsic->keys(pattern);
size_t countFlushed = 0;
if (vals.size() == 2 && portStr.empty())
{
swss::RedisCommand command;

command.format(
"EVALSHA %s 3 %s %s %s",
m_fdbFlushSha.c_str(),
pattern.c_str(),
portStr.c_str(),
std::to_string(flush_static).c_str());

swss::RedisReply r(m_dbAsic.get(), command);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand the root reason of "REDIS BUSY". If m_dbAsic is using redis pipeline, you could explicitly call its flush function to improve the redis responsibilities.

for (const auto &it: keys)
{
m_dbAsic->del(it);
countFlushed++;
}
}
else if (vals.size() == 2 && !portStr.empty())
{
for (const auto &it: keys)
{
auto bridgePortIdFromDb = m_dbAsic->hget(it, "SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID");
if (bridgePortIdFromDb && *bridgePortIdFromDb == portStr)
{
m_dbAsic->del(it);
countFlushed++;
}
}
}
else if (vals.size() == 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else

What if vals.size is not 1 or 2? currently the code just skip silently.

{
auto typeWants = vals.front() == 0 ?
"SAI_FDB_ENTRY_TYPE_DYNAMIC" : "SAI_FDB_ENTRY_TYPE_STATIC";
for (const auto &it: keys)
{
auto bridgePortIdFromDb = m_dbAsic->hget(it, "SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID");
auto typeFromDb = m_dbAsic->hget(it, "SAI_FDB_ENTRY_ATTR_TYPE");
if ((typeFromDb && *typeFromDb == typeWants) &&
(portStr.empty() || (!portStr.empty() && bridgePortIdFromDb &&
*bridgePortIdFromDb == portStr)))
{
m_dbAsic->del(it);
countFlushed++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S riot is added in Lua since it's faster to flush fdb than doing it 1by1, are you sure it's flush leading to Redis busy? How many entries fdb do you have in database ?

}
}
}
SWSS_LOG_NOTICE("RedisClient have flushed %lu fdb entries.", countFlushed);
}
Loading