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

+ Modified buffer config template to include internal ASIC with 5m ca… #4959

Merged
merged 4 commits into from
Jul 27, 2020

Conversation

smaheshm
Copy link
Contributor

- Why I did it

To support buffer config generation for multi ASIC platform. The cable length used to select buffer profile is determined by the role of local and peer switch. In case of multi ASIC platforms cable length needs to determined between ASICs.

- How I did it

Added a new role called "internal" with a cable length of "5m". The role becomes "internal" if one of the ASICs in the pair is defined as 'internal' in minigraph.

- How to verify it

Verified config generation on multi-ASIC and single ASIC platforms. Sent traffic with different priorities, verified the queue counters. Visual confirmation of buffer configuration values.


admin@str-nSonic-acs-2:/etc/sonic$ grep "Ethernet-BP..|3-4" config_db0.json -A2
        "Ethernet-BP12|3-4": {
            "profile": "[BUFFER_PROFILE|pg_lossless_40000_5m_profile]"
        },
--
        "Ethernet-BP16|3-4": {
            "profile": "[BUFFER_PROFILE|pg_lossless_40000_5m_profile]"
        },
--
        "Ethernet-BP20|3-4": {
            "profile": "[BUFFER_PROFILE|pg_lossless_40000_5m_profile]"
        },
--
.
.


admin@str-nSonic-acs-2:~$ sudo ip netns exec asic0 show mmu
Pool: ingress_lossless_pool
----  --------
type  ingress
mode  dynamic
size  12766208
----  --------

Pool: egress_lossless_pool
----  --------
type  egress
mode  static
size  12766208
----  --------

Pool: egress_lossy_pool
----  -------
type  egress
mode  dynamic
size  7326924
----  -------

Profile: pg_lossless_40000_300m_profile
----------  -----------------------------------
xon_offset  2496
dynamic_th  -3
xon         18432
xoff        55120
pool        [BUFFER_POOL|ingress_lossless_pool]
size        56368
----------  -----------------------------------

Profile: pg_lossless_40000_5m_profile
----------  -----------------------------------
xon_offset  2496
dynamic_th  -3
xon         18432
xoff        55120
pool        [BUFFER_POOL|ingress_lossless_pool]
size        56368
----------  -----------------------------------

Profile: egress_lossy_profile
----------  -------------------------------
dynamic_th  3
pool        [BUFFER_POOL|egress_lossy_pool]
size        1518
----------  -------------------------------

Profile: ingress_lossy_profile
----------  -----------------------------------
dynamic_th  3
pool        [BUFFER_POOL|ingress_lossless_pool]
size        0
----------  -----------------------------------

Profile: egress_lossless_profile
---------  ----------------------------------
static_th  12766208
pool       [BUFFER_POOL|egress_lossless_pool]
size       0
---------  ----------------------------------

- Description for the changelog

Modified the 'buffers_config' template to generate cable length between internal ASICs.

Modified sort API to account for internal interfaces that are named as "Ethernet-BPx".

- A picture of a cute animal (not mandatory but encouraged)

@smaheshm
Copy link
Contributor Author

recreated this PR to resolve merge conflicts. Addressed comments as mentioned in #4929

@arlakshm
Copy link
Contributor

@smaheshm, the sonic-config-engine tests are failing. Please check

@smaheshm
Copy link
Contributor Author

retest mellanox please

@smaheshm
Copy link
Contributor Author

retest broadcom please

@smaheshm
Copy link
Contributor Author

retest mellanox please

@smaheshm
Copy link
Contributor Author

@arlakshm @neethajohn Let me know if I should add any other reviewer.

@@ -57,7 +57,9 @@ def sort_by_port_index(value):
if not value:
return
if isinstance(value, list):
value.sort(key = lambda k: int(k[8:]))
value.sort(
key = lambda k: int(k[8:]) if "BP" not in k else int(k[11:]) + 1024
Copy link
Collaborator

@lguohan lguohan Jul 18, 2020

Choose a reason for hiding this comment

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

it is difficult to understand the logic here, for example, what does BP mean? what is 1024? why 1024?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Backend ethernet ports are identified as 'Ethernet-BPxy'. The change extracts the port number for the backend ports. '1024' is just to sort the back end ports after the front end ports, assuming number of front end ports will be < 1024.
'+ 1024' has no functional impact other than for cleaner look in config_db. All backend ports are grouped at the end. This can be removed if that is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment above this sort for future reference?

{%- set roles1 = roles1 | lower %}
{%- set roles2 = roles2 | lower %}
{%- if 'asic' == neighbor_role | lower %}
{%- set roles1 = 'internal' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have unit test to cover this change in sonic-config-engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test.

…latforms.

  All links that include backend ports must be of role 'internal'.
@smaheshm
Copy link
Contributor Author

retest broadcom please

neethajohn
neethajohn previously approved these changes Jul 22, 2020
@@ -57,7 +57,9 @@ def sort_by_port_index(value):
if not value:
return
if isinstance(value, list):
value.sort(key = lambda k: int(k[8:]))
value.sort(
key = lambda k: int(k[8:]) if "BP" not in k else int(k[11:]) + 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment above this sort for future reference?

@smaheshm
Copy link
Contributor Author

retest vsimage please

@smaheshm smaheshm merged commit ee4197e into sonic-net:master Jul 27, 2020
abdosi pushed a commit that referenced this pull request Aug 19, 2020
#4959)

+ Modified buffer config template to include internal ASIC with 5m cable length
+ added unit test to verify the changes.
@abdosi
Copy link
Contributor

abdosi commented Sep 5, 2020

@smaheshm Please create PR for 201911

@smaheshm
Copy link
Contributor Author

smaheshm commented Sep 8, 2020

@abdosi This is already in 201911 branch. commit: 42ab5d4

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

Successfully merging this pull request may close these issues.

5 participants