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

[sai-gen] Add P4 counter parsing support in SAI generation script and remove mock counter attributes in DASH pipeline #492

Merged
merged 6 commits into from
Jan 3, 2024

Conversation

r12f
Copy link
Collaborator

@r12f r12f commented Dec 22, 2023

Problem

Today, how DASH generates SAI API for counters is via mock action parameters.

One example is shown as below, where the counters are not updated by the meter_bucket_action, however we need to generate SAI APIs for fetching these counters. Hence, 2 action parameters that is marked as readonly are added for this purpose.

    #define MAX_METER_BUCKETS 262144
#ifdef TARGET_BMV2_V1MODEL
    counter(MAX_METER_BUCKETS, CounterType.bytes) meter_bucket_inbound;
    counter(MAX_METER_BUCKETS, CounterType.bytes) meter_bucket_outbound;
#endif // TARGET_BMV2_V1MODEL
    action meter_bucket_action(
            @SaiVal[type="sai_uint64_t", isreadonly="true"] bit<64> outbound_bytes_counter,
            @SaiVal[type="sai_uint64_t", isreadonly="true"] bit<64> inbound_bytes_counter,
            @SaiVal[type="sai_uint32_t", skipattr="true"] bit<32> meter_bucket_index) {
        // read only counters for SAI api generation only
        meta.meter_bucket_index = meter_bucket_index;
    }

However, this creates 2 problems:

  1. Obviously, these 2 parameters are never used and confusing in P4 code. However, they cannot be removed.
  2. In libsai, we will generate code for setting these parameters, which is not needed and also should not do.

Solution

This change adds counter parsing support in the SAI generation script, which,

  1. Gains the knowledge of the actual counters.
  2. Recreates the relationships between the counter and the actions.

Then we use this info to generate the correct SAI headers and libsai. Here is the P4 code after this change, the unused action parameters can be removed:

    #define MAX_METER_BUCKETS 262144
#ifdef TARGET_BMV2_V1MODEL
    @SaiCounter[name="outbound_bytes_counter", action_names="meter_bucket_action", as_attr="true"]
    counter(MAX_METER_BUCKETS, CounterType.bytes) meter_bucket_outbound;
    @SaiCounter[name="inbound_bytes_counter", action_names="meter_bucket_action", as_attr="true"]
    counter(MAX_METER_BUCKETS, CounterType.bytes) meter_bucket_inbound;
#endif // TARGET_BMV2_V1MODEL
    action meter_bucket_action(
            @SaiVal[type="sai_uint32_t", skipattr="true"] bit<32> meter_bucket_index) {
        meta.meter_bucket_index = meter_bucket_index;
    }

As the diff shows below, we will be able maintain the SAI header unchanged, also removing the unwanted code in libsai:

$ diff SAI/lib/ ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/
diff SAI/lib/saidashmeter.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashmeter.cpp
82c82
<     //expectedParams = 1;
---
>     //expectedParams = 3;
93a94,109
>             case SAI_METER_BUCKET_ATTR_OUTBOUND_BYTES_COUNTER:
>             {
>                 auto param = action->add_params();
>                 param->set_param_id(1);
>                 u64SetVal(attr_list[i].value, param, 64);
>                 //matchedParams++;
>                 break;
>             }
>             case SAI_METER_BUCKET_ATTR_INBOUND_BYTES_COUNTER:
>             {
>                 auto param = action->add_params();
>                 param->set_param_id(2);
>                 u64SetVal(attr_list[i].value, param, 64);
>                 //matchedParams++;
>                 break;
>             }

Future works

As we will be adding more counters later, for features like HA. This will become particularly helpful, since we don't have to abuse the action parameters with unused code anymore.

@r12f
Copy link
Collaborator Author

r12f commented Dec 22, 2023

Hi @chrispsommers , this is the change I mentioned in #489 which improves SAI API generation for counters.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

I've added @vijasrin as reviewer since he contributed the metering code in #365.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

Need to fix CI failures.

@r12f
Copy link
Collaborator Author

r12f commented Dec 26, 2023

Thanks a lot Chris! And Merry Christmas!

@r12f
Copy link
Collaborator Author

r12f commented Jan 2, 2024

CI is passed. Happy new year, @vijasrin ! Do you have any comments on this PR? If not, I will merge the change in. :D

@r12f r12f merged commit f63bc46 into sonic-net:main Jan 3, 2024
5 checks passed
@r12f r12f deleted the user/r12f/counter branch January 3, 2024 19:28
@r12f
Copy link
Collaborator Author

r12f commented Jan 3, 2024

thanks Kamil for reviewing it as well! since no more comments, I have the PR merged now.

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.

3 participants