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.

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

[QoS] Support dynamic headroom calculation for Barefoot platforms #2317

Closed
wants to merge 1 commit into from

Conversation

mmysle
Copy link

@mmysle mmysle commented Jun 8, 2022

Signed-off-by: Michał Mysłek michal.myslek@intel.com

What I did
Adding the dynamic headroom calculation support for Barefoot platforms.

Why I did it
Enabling dynamic mode for barefoot case.

How I verified it
The community tests are adjusted and pass.

Details if related

Signed-off-by: Michał Mysłek <michal.myslek@intel.com>
@mmysle mmysle requested a review from prsunny as a code owner June 8, 2022 09:19
@ghost
Copy link

ghost commented Jun 8, 2022

CLA assistant check
All CLA requirements met.

@prsunny prsunny requested review from neethajohn and stephenxs June 9, 2022 02:09
-- 2 PPGs per port, 70% of possible maximum value.
local shp_size = ports_num * 2 * ppg_headroom * 0.7

local ingress_lossless_pool_size_fixed = 43067728
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case the shared buffer pool sizes are always fixed, I would like to suggest reading them from CONFIG_DB and exposing it for ingress_lossless_pool only.
By doing so you do not need to change lua script next time a new platform with different buffer pool sizes is introduced.

-- Connect to STATE_DB
redis.call('SELECT', state_db)
local max_headroom_size = tonumber(redis.call('HGET', 'BUFFER_MAX_PARAM_TABLE|' .. port, 'max_headroom_size'))
if max_headroom_size == 0 or max_headroom_size == nil then
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI.
BUFFER_MAX_PARAM_TABLE|<port>|max_headroom_size is the maximum accumulative headroom that can be applied to a port. It will always be exposed to STATE_DB during orchagent initialization as long as being supported by the vendor SAI. It won't be changed after initialization. It is not relevant to whether the shared headroom pool is enabled.
So In case the shared headroom pool is a must on Barefoot's platforms, I would like to suggest enabling it in buffers_config.j2 in DEFAULT_LOSSLESS_BUFFER_PARAMETER and always returning true here.

local ports_num = #ports

-- 2 PPGs per port, 70% of possible maximum value.
local shp_size = ports_num * 2 * ppg_headroom * 0.7
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you can use DEFAULT_LOSSLESS_BUFFER_PARAMETER.over_subscribe_ratio here?
local shp_size = ports_num * 2 * ppg_headroom / over_subscribe_ratio
A problem here is that the over_subscribe_ratio is an intergeter. Not sure whether it satisfies your req.

@mmysle
Copy link
Author

mmysle commented Jun 10, 2022

@stephenxs, thank you for the review! The comments will be addressed here: #2323
Sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants