Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions orchagent/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ orchagent_SOURCES = \
copporch.cpp \
tunneldecaporch.cpp \
qosorch.cpp \
buffer/bufferhelper.cpp \
bufferorch.cpp \
mirrororch.cpp \
fdborch.cpp \
Expand Down
71 changes: 71 additions & 0 deletions orchagent/buffer/buffercontainer.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#pragma once

#include <cstdbool>
#include <cstdint>

#include <unordered_map>
#include <unordered_set>
#include <string>

class BufferContainer
{
public:
BufferContainer() = default;
virtual ~BufferContainer() = default;

std::unordered_map<std::string, std::string> fieldValueMap;
};

class BufferProfileConfig final : public BufferContainer
{
public:
BufferProfileConfig() = default;
~BufferProfileConfig() = default;

inline bool isTrimmingProhibited() const
{
return ((pgRefCount > 0) || (iBufProfListRefCount > 0) || (eBufProfListRefCount)) ? true : false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we avoid this refcount to determine the trim eligibility?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if possible, buffer_profile-->buffer_pool(check ingress/egress)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@stephenxs any other option?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can we avoid this refcount to determine the trim eligibility?

@kperumalbfn currently the most reliable way. Checking counter value is a low cost operation comparing to collection iteration/search

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

if possible, buffer_profile-->buffer_pool(check ingress/egress)?

@kperumalbfn Buffer Profile to Buffer Pool relation check (resolveFieldRefArray) is implemented as part of:
task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tuple)
task_process_status BufferOrch::processIngressBufferProfileList(KeyOpFieldsValuesTuple &tuple)
task_process_status BufferOrch::processEgressBufferProfileList(KeyOpFieldsValuesTuple &tuple)

Trimming eligibility validation serves different purposes and takes place long after this check, so it is not relevant

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not using the pools that the buffer profiles reference? That will be much easier to maintain.

}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we could keep all the counts as private and have methods to set/get. Would be clean to set/get by one method instead of direct access

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Currently not all egress buffer profiles are added to profile list. Could you please check?

https://github.com/sonic-net/sonic-buildimage/pull/22869/files

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we could keep all the counts as private and have methods to set/get. Would be clean to set/get by one method instead of direct access

@kperumalbfn Getter/Setter is a way of doing business logic encapsulation. In this case it is redundant and won't provide any benefits. That was the reason for omitting the implementation

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Currently not all egress buffer profiles are added to profile list. Could you please check?

https://github.com/sonic-net/sonic-buildimage/pull/22869/files

The current implementation relies on application db events, meaning all buffer profiles are processed in a single place - bufferorch. Please elaborate on how this relates to the current validation case ?

std::uint64_t pgRefCount = 0;
std::uint64_t iBufProfListRefCount = 0;
std::uint64_t eBufProfListRefCount = 0;

bool isTrimmingEligible = false;
};

class BufferPriorityGroupConfig final : public BufferContainer
{
public:
BufferPriorityGroupConfig() = default;
~BufferPriorityGroupConfig() = default;

struct {
std::string value;
bool is_set = false;
} profile;
};

class IngressBufferProfileListConfig final : public BufferContainer
{
public:
IngressBufferProfileListConfig() = default;
~IngressBufferProfileListConfig() = default;

struct {
std::unordered_set<std::string> value;
bool is_set = false;
} profile_list;
};

class EgressBufferProfileListConfig final : public BufferContainer
{
public:
EgressBufferProfileListConfig() = default;
~EgressBufferProfileListConfig() = default;

struct {
std::unordered_set<std::string> value;
bool is_set = false;
} profile_list;
};
303 changes: 303 additions & 0 deletions orchagent/buffer/bufferhelper.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,303 @@
// includes -----------------------------------------------------------------------------------------------------------

#include <unordered_map>
#include <string>

#include <tokenize.h>

#include "bufferschema.h"

#include "bufferhelper.h"

using namespace swss;

// helper -------------------------------------------------------------------------------------------------------------

void BufferHelper::parseBufferConfig(BufferProfileConfig &cfg) const
{
auto &map = cfg.fieldValueMap;

const auto &cit = map.find(BUFFER_PROFILE_PACKET_DISCARD_ACTION);
if (cit != map.cend())
{
cfg.isTrimmingEligible = cit->second == BUFFER_PROFILE_PACKET_DISCARD_ACTION_TRIM ? true : false;
}
}

void BufferHelper::parseBufferConfig(BufferPriorityGroupConfig &cfg) const
{
auto &map = cfg.fieldValueMap;

const auto &cit = map.find(BUFFER_PG_PROFILE);
if (cit != map.cend())
{
cfg.profile.value = cit->second;
cfg.profile.is_set = true;
}
}

void BufferHelper::parseBufferConfig(IngressBufferProfileListConfig &cfg) const
{
auto &map = cfg.fieldValueMap;

const auto &cit = map.find(BUFFER_PORT_INGRESS_PROFILE_LIST_PROFILE_LIST);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does bufferorch use ingress/egress buffer profile list for trim eligible validation?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@kperumalbfn validation is currently implemented for the next cases:

  1. Buffer Profile is attached to either Ingress PG or Ingress/Egress Buffer Profile List and user tries to set trimming eligibility
  2. Buffer Profile is configured as trimming eligible and user tries to attach it either to Ingress PG or Ingress/Egress Buffer Profile list

In both cases the error will be generated.
This guarantees that trimming eligible Buffer Profile can be used only with the Queue objects.

if (cit != map.cend())
{
auto profList = tokenize(cit->second, ',');

cfg.profile_list.value.insert(profList.begin(), profList.end());
cfg.profile_list.is_set = true;
}
}

void BufferHelper::parseBufferConfig(EgressBufferProfileListConfig &cfg) const
{
auto &map = cfg.fieldValueMap;

const auto &cit = map.find(BUFFER_PORT_EGRESS_PROFILE_LIST_PROFILE_LIST);
if (cit != map.cend())
{
auto profList = tokenize(cit->second, ',');

cfg.profile_list.value.insert(profList.begin(), profList.end());
cfg.profile_list.is_set = true;
}
}

template<>
void BufferHelper::setObjRef(const BufferProfileConfig &cfg)
{
// No actions are required
}

template<>
void BufferHelper::setObjRef(const BufferPriorityGroupConfig &cfg)
{
if (cfg.profile.is_set)
{
const auto &cit = profMap.find(cfg.profile.value);
if (cit != profMap.cend())
{
cit->second.pgRefCount++;
}
}
}

template<>
void BufferHelper::setObjRef(const IngressBufferProfileListConfig &cfg)
{
if (cfg.profile_list.is_set)
{
for (const auto &cit1 : cfg.profile_list.value)
{
const auto &cit2 = profMap.find(cit1);
if (cit2 != profMap.cend())
{
cit2->second.iBufProfListRefCount++;
}
}
}
}

template<>
void BufferHelper::setObjRef(const EgressBufferProfileListConfig &cfg)
{
if (cfg.profile_list.is_set)
{
for (const auto &cit1 : cfg.profile_list.value)
{
const auto &cit2 = profMap.find(cit1);
if (cit2 != profMap.cend())
{
cit2->second.eBufProfListRefCount++;
}
}
}
}

template<>
void BufferHelper::delObjRef(const BufferProfileConfig &cfg)
{
// No actions are required
}

template<>
void BufferHelper::delObjRef(const BufferPriorityGroupConfig &cfg)
{
if (cfg.profile.is_set)
{
const auto &cit = profMap.find(cfg.profile.value);
if (cit != profMap.cend())
{
cit->second.pgRefCount--;
}
}
}

template<>
void BufferHelper::delObjRef(const IngressBufferProfileListConfig &cfg)
{
if (cfg.profile_list.is_set)
{
for (const auto &cit1 : cfg.profile_list.value)
{
const auto &cit2 = profMap.find(cit1);
if (cit2 != profMap.cend())
{
cit2->second.iBufProfListRefCount--;
}
}
}
}

template<>
void BufferHelper::delObjRef(const EgressBufferProfileListConfig &cfg)
{
if (cfg.profile_list.is_set)
{
for (const auto &cit1 : cfg.profile_list.value)
{
const auto &cit2 = profMap.find(cit1);
if (cit2 != profMap.cend())
{
cit2->second.eBufProfListRefCount--;
}
}
}
}

template<>
auto BufferHelper::getBufferObjMap() const -> const std::unordered_map<std::string, BufferProfileConfig>&
{
return profMap;
}

template<>
auto BufferHelper::getBufferObjMap() const -> const std::unordered_map<std::string, BufferPriorityGroupConfig>&
{
return pgMap;
}

template<>
auto BufferHelper::getBufferObjMap() const -> const std::unordered_map<std::string, IngressBufferProfileListConfig>&
{
return iBufProfListMap;
}

template<>
auto BufferHelper::getBufferObjMap() const -> const std::unordered_map<std::string, EgressBufferProfileListConfig>&
{
return eBufProfListMap;
}

template<>
auto BufferHelper::getBufferObjMap() -> std::unordered_map<std::string, BufferProfileConfig>&
{
return profMap;
}

template<>
auto BufferHelper::getBufferObjMap() -> std::unordered_map<std::string, BufferPriorityGroupConfig>&
{
return pgMap;
}

template<>
auto BufferHelper::getBufferObjMap() -> std::unordered_map<std::string, IngressBufferProfileListConfig>&
{
return iBufProfListMap;
}

template<>
auto BufferHelper::getBufferObjMap() -> std::unordered_map<std::string, EgressBufferProfileListConfig>&
{
return eBufProfListMap;
}

template<typename T>
void BufferHelper::setBufferConfig(const std::string &key, const T &cfg)
{
auto &map = getBufferObjMap<T>();

const auto &cit = map.find(key);
if (cit != map.cend())
{
delObjRef(cit->second);
}
setObjRef(cfg);

map[key] = cfg;
}

template void BufferHelper::setBufferConfig(const std::string &key, const BufferProfileConfig &cfg);
template void BufferHelper::setBufferConfig(const std::string &key, const BufferPriorityGroupConfig &cfg);
template void BufferHelper::setBufferConfig(const std::string &key, const IngressBufferProfileListConfig &cfg);
template void BufferHelper::setBufferConfig(const std::string &key, const EgressBufferProfileListConfig &cfg);

template<typename T>
bool BufferHelper::getBufferConfig(T &cfg, const std::string &key) const
{
auto &map = getBufferObjMap<T>();

const auto &cit = map.find(key);
if (cit != map.cend())
{
cfg = cit->second;
return true;
}

return false;
}

template bool BufferHelper::getBufferConfig(BufferProfileConfig &cfg, const std::string &key) const;
template bool BufferHelper::getBufferConfig(BufferPriorityGroupConfig &cfg, const std::string &key) const;
template bool BufferHelper::getBufferConfig(IngressBufferProfileListConfig &cfg, const std::string &key) const;
template bool BufferHelper::getBufferConfig(EgressBufferProfileListConfig &cfg, const std::string &key) const;

void BufferHelper::delBufferProfileConfig(const std::string &key)
{
const auto &cit = profMap.find(key);
if (cit == profMap.cend())
{
return;
}

delObjRef(cit->second);
profMap.erase(cit);
}

void BufferHelper::delBufferPriorityGroupConfig(const std::string &key)
{
const auto &cit = pgMap.find(key);
if (cit == pgMap.cend())
{
return;
}

delObjRef(cit->second);
pgMap.erase(cit);
}

void BufferHelper::delIngressBufferProfileListConfig(const std::string &key)
{
const auto &cit = iBufProfListMap.find(key);
if (cit == iBufProfListMap.cend())
{
return;
}

delObjRef(cit->second);
iBufProfListMap.erase(cit);
}

void BufferHelper::delEgressBufferProfileListConfig(const std::string &key)
{
const auto &cit = eBufProfListMap.find(key);
if (cit == eBufProfListMap.cend())
{
return;
}

delObjRef(cit->second);
eBufProfListMap.erase(cit);
}
Loading
Loading