sonic-net / DASH

Disaggregated APIs for SONiC Hosts
Apache License 2.0
80 stars 89 forks source link

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

Closed r12f closed 8 months ago

r12f commented 8 months ago

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 commented 8 months ago

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

r12f commented 8 months ago

Thanks a lot Chris! And Merry Christmas!

r12f commented 8 months ago

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 commented 8 months ago

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