Adding packet length to Meter execute and Counter count methods for targets that may need it #49
Conversation
…argets that may need it
|
@usha1830 Do you still want this PR? I think we discussed it privately several weeks ago, but I haven't followed up with you since then. |
| @@ -444,7 +444,7 @@ enum PNA_CounterType_t { | |||
|
|
|||
| extern Counter<W, S> { | |||
| Counter(bit<32> n_counters, PNA_CounterType_t type); | |||
| void count(in S index); | |||
| void count(in S index, @optional in bit<32> increment); | |||
There was a problem hiding this comment.
Optional increment is not applicable to packet counter. It may apply to byte counter.
Can we revise this and look into a way address application of optional increment only on byte counters ?
There was a problem hiding this comment.
We will handle this in p4c and expect the packet length only in case of byte counters. We can choose to either reject the programs with packet length parameter passed to packet counters or ignore that parameter with a warning.
There was a problem hiding this comment.
For P4c or backend to reject optional increment argument, instance of Counter type need to be qualified so that P4C/ backend knows it a byte counter instance versus packet counter instance. Is this something that's possible with the way the Counter<W,S> class is defined ? If not, I am not sure how we can error out increment argument if the method is invoked on packet counter instance. (not on byte counter object or on packet_and_byte counter object)
There was a problem hiding this comment.
Whether a counter instance is packet, byte, or packet & byte, is known during compile time based upon the arguments to the constructor call that created the instance. The P4 compiler should have all of the information required to know which kind each counter instance is.
| @@ -444,7 +444,7 @@ enum PNA_CounterType_t { | |||
|
|
|||
| extern Counter<W, S> { | |||
| Counter(bit<32> n_counters, PNA_CounterType_t type); | |||
| void count(in S index); | |||
| void count(in S index, @optional in bit<32> increment); | |||
There was a problem hiding this comment.
Also should we look into adding a new method instead of modifying existing one ?
| @@ -444,7 +444,7 @@ enum PNA_CounterType_t { | |||
|
|
|||
| extern Counter<W, S> { | |||
| Counter(bit<32> n_counters, PNA_CounterType_t type); | |||
| void count(in S index); | |||
| void count(in S index, @optional in bit<32> increment); | |||
There was a problem hiding this comment.
Yes. We will remove @optional.
| @@ -444,7 +444,7 @@ enum PNA_CounterType_t { | |||
|
|
|||
| extern Counter<W, S> { | |||
| Counter(bit<32> n_counters, PNA_CounterType_t type); | |||
| void count(in S index); | |||
| void count(in S index, @optional in bit<32> increment); | |||
@jfingerh I will close this PR. In case of counter 'count' method, an overloaded function would work as @apinski-cavium pointed out. However, in case of Meter 'execute' , we may have to change the method name to avoid the conflict between the below two signatures. p4c doesn't seem to resolve overloaded methods based on type.
|
|
Closing this PR as discussed. |
Proposing to add an optional parameter to specify the packet length for Meter extern's execute method and and Counter extern's count method.
If we can add this parameter as optional to the pna specification, it would help DPDK target to comply with the spec. For targets that don’t need packet length, it can be simply ignored.