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

Jump to bottom

Rename ENI attribute PPS to BW to reflect the actual BW limitation in bytes #526

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Rename ENI attribute PPS to BW to reflect the actual BW limitation in bytes #526

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0e59c4
Update dash_pipeline.p4
marian-pritsak Feb 26, 2024
7753b2a
Update dash_metadata.p4
marian-pritsak Feb 26, 2024
cc43c75
Rename all SAI attributes
marian-pritsak Mar 19, 2024
e10428d
Merge remote-tracking branch 'origin-1/main' into patch-4
marian-pritsak Apr 30, 2024
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
2 changes: 1 addition & 1 deletion dash-pipeline/bmv2/dash_metadata.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ enum bit<16> dash_tunnel_dscp_mode_t {

struct eni_data_t {
bit<32> cps;
bit<32> pps;
bit<32> bw;
Copy link
Collaborator

Choose a reason for hiding this comment

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

although bw is the one we use in the HLD, I wonder if we like to get the unit clear, such as bw_in_kbps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@r12f good idea. At least PPS is unambiguous. The item name should include the units to make it clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All BW throughout SAI is in Mb/s

Copy link
Collaborator

@mgheorghe mgheorghe Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why not keep it bw and bw can accept values like 978, 1K, 1M, 15G, 2T, (if we don't want to type too many zeroes) and the last char if letter detonates the unit this way we don't end up with 20 api names, like bw_in_3_and_a_half_bytes_increments (exaggeration to make the point),

take it further and in the value you can add last 2 letters as 1000Kb or 1000KB, or have convention that uppercase means bytes lowercase bits so we need only 1 letter( if K is bytes if k is in bits ... seen this way of handling it in many apps)

bit<32> flows;
bit<1> admin_state;
IPv6Address pl_sip;
Expand Down
4 changes: 2 additions & 2 deletions dash-pipeline/bmv2/dash_pipeline.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ control dash_ingress(
#endif

action set_eni_attrs(bit<32> cps,
bit<32> pps,
bit<32> bw,
bit<32> flows,
bit<1> admin_state,
@SaiVal[type="sai_ip_address_t"] IPv4Address vm_underlay_dip,
Expand All @@ -140,7 +140,7 @@ control dash_ingress(
ACL_GROUPS_PARAM(outbound_v6),
bit<1> disable_fast_path_icmp_flow_redirection) {
meta.eni_data.cps = cps;
meta.eni_data.pps = pps;
meta.eni_data.bw = bw;
meta.eni_data.flows = flows;
meta.eni_data.admin_state = admin_state;
meta.eni_data.pl_sip = pl_sip;
Expand Down
Loading