p4lang / pna

Portable NIC Architecture
Apache License 2.0
54 stars 21 forks source link

Proposal for setting packets and bytes counter widths separately #119

Open qobilidop opened 1 year ago

qobilidop commented 1 year ago

Currently in PNA, a counter extern is defined as the following:

enum PNA_CounterType_t {
    PACKETS,
    BYTES,
    PACKETS_AND_BYTES
}

extern Counter<W, S> {
  Counter(bit<32> n_counters, PNA_CounterType_t type);
  void count(in S index, @optional in bit<32> increment);
}

When type == PNA_CounterType_t.PACKETS_AND_BYTES, the counter is used to count both packets and bytes. The way it works (as I understand) is that some bits of the counter works as the packets counter, while the rest of the bits work as the bytes counter. The issue here is that we can only specify the total width of the combined counter through W, but not the 2 counters separately.

Would it be a good idea to enable setting packets and bytes counter widths separately?

qobilidop commented 1 year ago

In PSA spec (C. Appendix: Example implementation of Counter extern), I see the following example and comments:

Counter(bit<32> n_counters, PSA_CounterType_t type) {
    this.num_counters = n_counters;
    this.counter_vals = new array of size n_counters, each element with type W;
    this.type = type;
    if (this.type == PSA_CounterType_t.PACKETS_AND_BYTES) {
        // Packet and byte counts share storage in the same counter
        // state.  Should we have a separate constructor with an
        // additional argument indicating how many of the bits to use
        // for the byte counter?
        W shift_amount = TBD;
        this.shifted_packet_count = ((W) 1) << shift_amount;
        this.packet_count_mask = (~((W) 0)) << shift_amount;
        this.byte_count_mask = ~this.packet_count_mask;
    }
}

So it seems this might have been discussed previously?

jfingerh commented 1 year ago

We can check with implementers, but I suspect that specifying the data plane bit width of counters might have been (a) something a lot of targets do not support, because (b) some targets, to save precious on-chip die area, use small counters with either fast enough low-level driver read-and-clearing to guarantee they never wrap, accumulating the full count values in cheaper general purpose CPU DRAM, or in some cases have special hardware mechanisms to "push" such read-and-cleared values of counters that are the largest current values on the chip, in such a way that it prevents loss of count information.

I would not be surprised if most P4 targets simply ignored these bit widths, and used values that were as small as possible, yet wide enough that the particular target's hardware & driver software prevent counter information loss.

qobilidop commented 1 year ago

Do comments (a) and (b) apply to counters in general or is there something specific to the PACKETS_AND_BYTES counters?

If this applies to all counters, does it mean when I read the counter, I could get a value outside the range indicated by the counter bit width? For example, for Counter<bit<8>, bit<1>>(1, PNA_CounterType_t.PACKETS) counter_set, could I read a counter value > 255 if the hardware counter used is wider than 8 bits? My guess is that P4Runtime will (and should) take care of truncating the value to respect the specified type, right?

In the case where hardware cannot support a wide enough counter that the P4 code specifies, I wonder if the compiler could reject such code and give a proper error message? I think this might be a good way to communicate hardware constraints to users.

jfingerh commented 1 year ago

Again, we can check with implementers, but I would not be surprised if implementations do NOT make such a restriction on their count values based upon these bit widths in the P4 code.

My comments apply equally for packet counts, byte counts, and packet & byte counts.

qobilidop commented 1 year ago

I see. Thanks for the clarification!