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

Unify qos json by using qos_config.j2 template #2023

Merged
merged 18 commits into from
Oct 17, 2018

Conversation

wendani
Copy link
Contributor

@wendani wendani commented Sep 6, 2018

- What I did
Unify qos json by using qos_config.j2 template.

All devices use the same WRED profile. This changes the corresponding parameters on all Mellanox devices.

- How I did it

- How to verify it

Verified on 7050

Verified by qos subtest in test_j2files.py

- Description for the changelog

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

Signed-off-by: Wenda <wenni@microsoft.com>
Signed-off-by: Wenda <wenni@microsoft.com>

	modified:   device/arista/x86_64-arista_7050_qx32/Arista-7050-QX32/qos.json.j2
	modified:   device/arista/x86_64-arista_7050_qx32s/Arista-7050-QX-32S/qos.json.j2
Signed-off-by: Wenda <wenni@microsoft.com>

	modified:   ../../../mellanox/x86_64-mlnx_msn2100-r0/ACS-MSN2100/qos.json.j2
	modified:   ../../../mellanox/x86_64-mlnx_msn2410-r0/ACS-MSN2410/qos.json.j2
	modified:   ../../../mellanox/x86_64-mlnx_msn2700-r0/ACS-MSN2700/qos.json.j2
	modified:   ../../../mellanox/x86_64-mlnx_msn2700-r0/Mellanox-SN2700-D48C8/qos.json.j2
Signed-off-by: Wenda <wenni@microsoft.com>

	modified:   barefoot/x86_64-accton_wedge100bf_32x-r0/montara/qos.json.j2
	modified:   barefoot/x86_64-accton_wedge100bf_65x-r0/mavericks/qos.json.j2
Signed-off-by: Wenda <wenni@microsoft.com>

	modified:   accton/x86_64-accton_as7212_54x-r0/AS7212-54x/qos.json.j2
Signed-off-by: Wenda <wenni@microsoft.com>
Signed-off-by: Wenda <wenni@microsoft.com>
Signed-off-by: Wenda <wenni@microsoft.com>
@wendani
Copy link
Contributor Author

wendani commented Sep 6, 2018

@simonJi2018 please check the accton devices

@wendani
Copy link
Contributor Author

wendani commented Sep 6, 2018

@mkbalani please check the barefoot devices

@wendani
Copy link
Contributor Author

wendani commented Sep 6, 2018

@stepanblyschak please check the mlnx devices

Copy link

@SAI-shared-user SAI-shared-user left a comment

Choose a reason for hiding this comment

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

Can qos.j2 be merged into buffers.j2?

in buffers.j2. currently, we only configure the 'active' ports (ports with neighbor defined). This new qos.j2 didn't take consistent actions.

If merging these 2 files is not feasible, can we at least abstract the port list builder code out and share between the 2?

@wendani
Copy link
Contributor Author

wendani commented Sep 6, 2018

It is using active ports. Which part do you think is not consistent? buffers and qos are different entities. We definitely can merge, but this is not within the scope of this pr.

#2023 (review)

@andriymoroz-mlnx
Copy link
Collaborator

WRED_PROFILE is defined in global template (files/build_templates/qos_config.j2) but the numbers in it are platform specific. I suggest to mote it to device//.../qos.json.j2
Also I guess you missed section where WRED_PROFILE is used, e.g:

 "QUEUE": {	
        "Ethernet0,...|3-4" : {	
            "scheduler"     :   "[SCHEDULER|scheduler.0]",	
            "wred_profile"  :   "[WRED_PROFILE|AZURE_LOSSLESS]"

{% for port_idx in range(0, 32) %}
{% if PORT_ALL.append("Ethernet%d" % (port_idx * 4)) %}{% endif %}
{% endfor %}
{%- endmacro %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use PORT?

sonic-cfggen -d -v PORT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is using PORT as higher precedence. https://github.com/Azure/sonic-buildimage/pull/2023/files#diff-786c596a58cef95307fd598ac8204e35R3

This is a layer underneath PORT that provides the default port name and index in case PORT is not defined. This logic exists before unification, and is inherited here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

PORT is always defined.

@wendani
Copy link
Contributor Author

wendani commented Sep 7, 2018

@andriymoroz-mlnx the missing WRED_PROFILE is done on purpose, because the external team does not want the ecn to be applied on queues by default.

#2023 (comment)

@wendani
Copy link
Contributor Author

wendani commented Sep 7, 2018

@andriymoroz-mlnx Sorry I did not point out the WRED parameter changes on MLNX platforms. Just added in the head description. For our use, the WRED (or more general, QoS) parameters are not platform specific but global. This is the drive for the unification, and hence the WRED parameter changes as you observe as a result.

If other users want to have customized parameters for WRED or other QoS parameters, they can later put them in the local device//.../qos.json.j2, and add a hook in the qos_config.j2 template to dictate the precedence. Such changes would be straightforward.

#2023 (comment)

{% for port_idx in range(0, 32) %}
{% if PORT_ALL.append("Ethernet%d" % (port_idx * 4)) %}{% endif %}
{% endfor %}
{%- endmacro %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to make the generate_port_lists macro the same for all platforms.We can introduce port range, port_multiplier and exception ports as parameters. It will be more readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this has been removed completely.

…the generate_port_lists macro, because PORT is always defined

Signed-off-by: Wenda <wenni@microsoft.com>
}
}
}
{%- set pfc_to_pg_map = true %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

some of the files do not have this line, {%- set pfc_to_pg_map = true %}

for example, the qos.json.j2 for arista 7050 qx32 do no have, why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is better to hide it from the user since it is asic specific

Copy link

@SAI-shared-user SAI-shared-user left a comment

Choose a reason for hiding this comment

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

Sorry, Wenda,

I somehow missed the active port part of the change. You are right. This change is consistent with buffers configuration.

Cheers,
Ying

/etc/sonic/sonic_version.yml rather than specifying per hwsku

Signed-off-by: Wenda Ni <wenni@microsoft.com>


{
"TC_TO_PRIORITY_GROUP_MAP": {
Copy link
Contributor

Choose a reason for hiding this comment

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

These values would not work in barefoot platforms. Is there a way a platform can overwrite these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mkbalani, This is actually the question I have for you. I see TC_TO_PRIORITY_GROUP_MAP to have only 3 and 4 on barefoot. We cannot include other mappings: "0": "0", "1": "1", "2": "2", "5": "5", "6": "6", "7": "7" here? Can you elaborate the reasons?

"TC_TO_PRIORITY_GROUP_MAP": {
    "AZURE": {
        "3": "3",
        "4": "4" 
    }   
},  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have customized TC_TO_PRIORITY_GROUP_MAP and PFC_PRIORITY_TO_PRIORITY_GROUP_MAP for barefoot devices. Can you check again?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand why customization is needed for barefoot platform. @mkbalani , can you elaborate? @wendani , we should not do customization unless there is legitimate reason, otherwise it is too difficult the manage the complexity here.

}
},
{% if pfc_to_pg_map is defined and pfc_to_pg_map %}
"PFC_PRIORITY_TO_PRIORITY_GROUP_MAP": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here. Can this be overwritten?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand why customization is needed for barefoot platform. @mkbalani , can you elaborate? @wendani , we should not do customization unless there is legitimate reason, otherwise it is too difficult the manage the complexity here.

PFC_PRIORITY_TO_PRIORITY_GROUP_MAP for barefoot

Signed-off-by: Wenda <wenni@microsoft.com>
@lguohan
Copy link
Collaborator

lguohan commented Sep 11, 2018

can you update the sonic-utilities submodule in the PR, i think that is needed.

@wendani
Copy link
Contributor Author

wendani commented Sep 11, 2018

Will update the sonic-utilities submodule reference point once that is merged

#2023 (comment)

"weight": "20"
}
},
{%- endmacro %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to define this macro in qos_config.j2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this question for @wendani or me?

these two pgs do not generate PFC frames.

Signed-off-by: Wenda <wenni@microsoft.com>
@lguohan lguohan merged commit 77652c5 into sonic-net:master Oct 17, 2018
stephenxs added a commit to stephenxs/sonic-buildimage that referenced this pull request Nov 23, 2021
bb0733a [aclorch] Add ACL_TABLE_TYPE configuration  (sonic-net#1982)
59cab5d Support for setting switch level DSCP to TC QoS map (sonic-net#2023)
da21172 [aclorch] add generic AclOrch::updateAclRule() method (sonic-net#1993)
4f6cb05 [Reclaiming buffer] Support reclaiming buffer in traditional model (sonic-net#2011)
32d7a69 [Reclaiming buffer] Common code update (sonic-net#1996)
b91d8ba [swss] L2 Forwarding Enhancements (sonic-net#1716)
797dab4 [muxorch] Bind all ports to drop ACL table (sonic-net#2027)
99929cd [lgtm.yml] add libgmock-dev (sonic-net#2035)
8727ae5 [flex counter] Flex counter threads consume too much CPU resources sonic-net#9202 (sonic-net#2031)
103fdf0 Remove redundant calls to get child scheduler group during initialization (sonic-net#1965)
18ea840 [macsec]: MACsec statistics support (sonic-net#1867)
0c46242 [orchagent] Flush pipeline every 1 second, not only when select will timeout (sonic-net#2003)
339101c [cbf] Add class-based forwarding support (sonic-net#1963)
24a615b Fix issue: accumulative headroom can exceed limit in rare scenario (sonic-net#2020)
708e232 Test divide by zero processing path (sonic-net#2028)
8f1d035 [macsecmgr]: Wait for port up before enabling macsec (sonic-net#2032)
4912a77 Remove buffer drop counter when port is removed (sonic-net#1860)
f9462c4 [Dynamic buffer] [Mellanox] Calculate the peer response time according to the speed (sonic-net#1930)
8b5a401 Routed subinterface enhancements (sonic-net#2017)
cdea5e9 Fix next hop compilation (sonic-net#2025)
37c197d [SRV6] Sonic-swss changes for SRV6 (sonic-net#1964)
f502c32 [vnetorch] Add ECMP support for vnet tunnel routes (sonic-net#1960)

Signed-off-by: Stephen Sun <stephens@nvidia.com>
liat-grozovik pushed a commit that referenced this pull request Nov 24, 2021
bb0733a [aclorch] Add ACL_TABLE_TYPE configuration  (#1982)
59cab5d Support for setting switch level DSCP to TC QoS map (#2023)
da21172 [aclorch] add generic AclOrch::updateAclRule() method (#1993)
4f6cb05 [Reclaiming buffer] Support reclaiming buffer in traditional model (#2011)
32d7a69 [Reclaiming buffer] Common code update (#1996)
b91d8ba [swss] L2 Forwarding Enhancements (#1716)
797dab4 [muxorch] Bind all ports to drop ACL table (#2027)
99929cd [lgtm.yml] add libgmock-dev (#2035)
8727ae5 [flex counter] Flex counter threads consume too much CPU resources #9202 (#2031)
103fdf0 Remove redundant calls to get child scheduler group during initialization (#1965)
18ea840 [macsec]: MACsec statistics support (#1867)
0c46242 [orchagent] Flush pipeline every 1 second, not only when select will timeout (#2003)
339101c [cbf] Add class-based forwarding support (#1963)
24a615b Fix issue: accumulative headroom can exceed limit in rare scenario (#2020)
708e232 Test divide by zero processing path (#2028)
8f1d035 [macsecmgr]: Wait for port up before enabling macsec (#2032)
4912a77 Remove buffer drop counter when port is removed (#1860)
f9462c4 [Dynamic buffer] [Mellanox] Calculate the peer response time according to the speed (#1930)
8b5a401 Routed subinterface enhancements (#2017)
cdea5e9 Fix next hop compilation (#2025)
37c197d [SRV6] Sonic-swss changes for SRV6 (#1964)
f502c32 [vnetorch] Add ECMP support for vnet tunnel routes (#1960)

Signed-off-by: Stephen Sun <stephens@nvidia.com>
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
Signed-off-by: Vineet Mittal <vineetmittal@microsoft.com>
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.

7 participants