Add support for Counter extern to PSA/eBPF backend #3165
Conversation
Co-authored-by: Mateusz Kossakowski <[email protected]> Co-authored-by: Jan Palimąka <[email protected]>
| for k in keys: | ||
| key_str = key_str + " {}".format(k) | ||
| cmd = "psabpf-ctl counter get pipe {} {} {}".format(TEST_PIPELINE_ID, name, key_str) | ||
| _, stdout, _ = self.exec_ns_cmd(cmd, "Counter get failed") |
There was a problem hiding this comment.
so if the command fails you still return something, which may be wrong?
Perhaps this should instead return something like a rust Result type, which could contain an error.
I like using type annotations in Python, you should consider adding them.
There was a problem hiding this comment.
No. If the command fails, exec_ns_cmd will fails the entire test case. This piece of code handles that:
p4c/backends/ebpf/tests/ptf/common.py
Line 130 in 2732235
So, if the command fails, the following lines will not be reached. This behavior is achieved if we pass a second argument to the function. Otherwise, the first value returned from the function is a return code, so a user can use it as a result type and decide what to do with an error.
| void ActionTranslationVisitorPSA::processMethod(const P4::ExternMethod* method) { | ||
| auto declType = method->originalExternType; | ||
| auto decl = method->object; | ||
| BUG_CHECK(decl->is<IR::Declaration_Instance>(), |
There was a problem hiding this comment.
I find it useful to print something about the IR node that causes the failure, otherwise you make debugging a bit harder. But I understand that probably this never failed so far.
... declared: %1%", decl);
| auto ts = di->type->to<IR::Type_Specialized>(); | ||
|
|
||
| if (ts->arguments->size() != 2) { | ||
| ::error(ErrorType::ERR_MODEL, "Expected 2 specialization types: %1%", ts); |
There was a problem hiding this comment.
I would say "expected a type specialized with two arguments"
| // check dataplane counter width | ||
| auto dpwtype = ts->arguments->at(0); | ||
| if (!dpwtype->is<IR::Type_Bits>()) { | ||
| ::error(ErrorType::ERR_UNSUPPORTED, "Must be bit or int type: %1%", ts); |
There was a problem hiding this comment.
Perhaps it's better if you actually use dpwtype here instead of ts.
| unsigned dataplaneWidth = dpwtype->width_bits(); | ||
| if (dataplaneWidth > 64) { | ||
| ::error(ErrorType::ERR_UNSUPPORTED, | ||
| "Counters dataplane width up to 64 bits are supported: %1%", ts); |
| // check index type | ||
| auto istype = ts->arguments->at(1); | ||
| if (!dpwtype->is<IR::Type_Bits>()) { | ||
| ::error(ErrorType::ERR_UNSUPPORTED, "Must be bit or int type: %1%", ts); |
| if (indexWidth != 32) { | ||
| // ARRAY_MAP can have only 32 bits key, so assume this and warn user | ||
| ::warning(ErrorType::WARN_UNSUPPORTED, | ||
| "Only 32-bit type for index is supported, changed to 32 bit: %1%", ts); |
|
Regarding this sentence in your comment: "We currently implement Counter as a BPF array map, so the index width must be less or equal to bit<32>. We're planning to use BPF hash maps for Counters with wider index type." I won't stop you, of course, but It seems to me to have very low utility to support indirect anything in P4 with an index wider than 32 bits. I guess long term on a general purpose CPU someone might want to have more than 2^32 counters/meters/register-array-elements, but I would be surprised if that is an urgent desire. |
"32-bit indices ought to be enough for anybody." —@jafingerhut, March 2022. |
|
I agree, Andy. I would say that this is something to consider in future, but, definitely, it's not an urgent desire. |
|
@mbudiu-vmw can we have it merged if approved? Other externs are in the queue waiting for this :) |
|
I will assume you have addressed the comments. |
This PR adds the support for indirect Counter extern to PSA/eBPF backend.
We currently implement Counter as a BPF array map, so the index width must be less or equal to
bit<32>. We're planning to use BPF hash maps for Counters with wider index type.Similarly to
ebpf_model, the PSA Counter implementation uses__sync_fetch_and_addto provide atomicity.Co-authored-by: Mateusz Kossakowski [email protected]
Co-authored-by: Jan Palimąka [email protected]