p4lang / p4-spec

Apache License 2.0
175 stars 80 forks source link

Get size of header #660

Closed theojepsen closed 5 years ago

theojepsen commented 6 years ago

How do you get the size of a header in P4? Here is an example of what I'm trying to do:

action set_len() {
    hdr.ipv4.totalLen = len(hdr.ipv4) + len(hdr.udp) + len(hdr.my_proto);
}

len should return the size of the header (in bytes). In Python you can do this with Scapy. E.g. use len(scapy.UDP()) to get the size of a UDP header (i.e. 8 bytes).

The best workaround that I've found is to use a #define to set all the header sizes at the begging of my program, e.g.

#define IP_HDR_SIZE  20
#define MY_PROTO_HDR_SIZE 12

However, this has caused my lots of headaches. I can't tell you how many times I've changed the number/size of header fields, forgetting to update the constant, causing me to waste hours troubleshooting parser errors.

This seems like something straight-forward to implement in the compiler, possibly in the front-end. Since this can be calculated statically at compile time, it should be target-agnostic.

jnfoster commented 6 years ago

What about variable length headers?

On Fri, Jul 27, 2018 at 6:21 PM theojepsen notifications@github.com wrote:

How do you get the size of a header in P4? Here is an example of what I'm trying to do:

action set_len() { hdr.ipv4.totalLen = len(hdr.ipv4) + len(hdr.udp) + len(hdr.my_proto); }

len should return the size of the header (in bytes). In Python you can do this with Scapy. E.g. use len(scapy.UDP()) to get the size of a UDP header (i.e. 8 bytes).

The best workaround that I've found is to use a #define to set all the header sizes at the begging of my program, e.g.

define IP_HDR_SIZE 20

define MY_PROTO_HDR_SIZE 12

However, this has caused my lots of headaches. I can't tell you how many times I've changed the number/size of header fields, forgetting to update the constant, causing me to waste hours troubleshooting parser errors.

This seems like something straight-forward to implement in the compiler, possibly in the front-end. Since this can be calculated statically at compile time, it should be target-agnostic.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-spec/issues/660, or mute the thread https://github.com/notifications/unsubscribe-auth/ABwi0py3gf9xEQuoOePQHszRxJSwi1UTks5uK5JegaJpZM4Vkvto .

theojepsen commented 6 years ago

@jnfoster I was thinking about why this feature (get header size) is not part of the language, and variable length headers was the first thing that popped into mind. Obviously, this is not known statically at compile time.

Would it be possible to raise a compile-time exception if len() is called on variable length headers?

mihaibudiu commented 6 years ago

We could also define it to return the max size of the header. We probably should return the size in bits. An important question is whether we promise this to be a compile-time constant. This may cause some difficulties in the implementation since we need to know all types before we can compute widths, but we can probably work it out. I will try to see whether we can work it out.

Another question is whether this applies to types aka sizeof, or just to instances. In the latter case we can make it like a method call.

jafingerhut commented 5 years ago

Thoughts on this idea. I am happy to create a PR along these lines, but wanted to write something up quickly that others may comment on:

Some possibilities for syntax:

If we support the ability to have a separate operation get the size in bits, I would propose including "bits" somewhere in its name, to distinguish it from the one that returns bytes. We could also explicitly include "bytes" in the names of the operation that return bytes.

mihaibudiu commented 5 years ago

This looks very reasonable. How about querying a type, and not a variable?

jafingerhut commented 5 years ago

I have no problem with querying a type. I suspect the choice of syntax might matter more for that case than for querying a variable. Any suggestions on what syntax choices would work well for either a variable or a type, vs. which would not work well for a type?

mihaibudiu commented 5 years ago

I like the dot syntax best, because it does not polute the namespace of global identifiers. Maybe we can add methods on types too... type.size()

mihaibudiu commented 5 years ago

We should check that the grammar is not ambiguous then.

jnfoster commented 5 years ago

I'm not strongly opposed, but why query the type rather than the instance? We have no other methods on types. I guess I don't see a reason to invent that category of operation unless there is a deep reason to do so....

jafingerhut commented 5 years ago

For the primary use case as demonstrated in the first comment of this issue, only allowing this new operation on variables, and not on types, will do the job perfectly.

I could imagine someone maybe wanting to know the length of a header that was neither parsed, nor is ever valid in an output packet, but that seems much much rarer to me. Even then, if we only supported the new operation on variables, the simple workaround is to require the P4 programmer to create one instance of that type, only for the purpose of getting its size.

jnfoster commented 5 years ago

I suppose the familiarity with C's sizeof is another potential reason... although I personally find that borrowing features that work similarly to but not quite exactly the same as in another language is a recipe for confusion.

jafingerhut commented 5 years ago

I am not married to any particular name, including sizeof. If a we think a name other than sizeof would avoid confusion, cool.

mihaibudiu commented 5 years ago

Actually using the syntax .size is a problem if the structure has a field named size...

jafingerhut commented 5 years ago

I wanted to add another comment to summarize some goals I have for discussing the addition of this feature, to hopefully streamline those discussions:

Another proposal for syntax, if it avoids worries that sizeof will mislead people coming from C/C++:

That makes the units immediately obvious to everyone, and a name like sizeBits(hdr_variable) another things that could be added with very clear units for the return value.

I suppose we could use sizeOctets instead of sizeBytes if people care much about the name octet vs. byte. I think I'm too young to care about the difference, having never programmed a system where byte meant anything other than 8 bits :-) https://www.differencebetween.com/difference-between-octet-and-vs-byte/

mihaibudiu commented 5 years ago

We should perhaps allow overloading: hdr.sizeInBits and hdr.sizeInBits()

jafingerhut commented 5 years ago

Following initial discussion during 2018-Dec-03 language design WG meeting on this issue, I volunteered to write a PR for the language spec with the details. I will put a "stake in the sand" by using the syntax sizeInByte(hdr_variable), but this could of course be changed.

jafingerhut commented 5 years ago

@liujed Jed, regarding your question during the meeting about not bothering to define this new operator on structs, it is really a combination of two reasons, not only the one I mentioned during the meeting:

liujed commented 5 years ago

I think it's fine to leave off structs for now. Later on, if there turns out to be a need to get the size of a struct, we can always add that. With #383 coming down the pipe, though, I can see it soon being useful to at least talk about the size of a bit-vector struct.

jafingerhut commented 5 years ago

But with the limit of sizeInByte() only implemented on header types, even if/when #383 is implemented, there would still only need to be a way to define the size of header types that contain bit-vector structs. So we still wouldn't need to get into discussions of issues like what the size of error and all enum types are, only the structs that can be inside of headers, which are intentionally limited to avoid those questions.

jafingerhut commented 5 years ago

FYI, for those curious about the current p4c behavior if you try to use fields in a header with names of built-in methods like isValid, setValid, and setInvalid, it gives an error, at least if you try to use those fields as fields in a table key. Attempting to compile the attached P4_16 program using the latest p4c as of 2018-Dec-07 gives these error messages:

% p4c --target bmv2 --arch v1model header-field-name-collision.p4
header-field-name-collision.p4(79): error: Key hdr.foohdr.isValid field type must be a scalar type; it cannot be bool function()
            hdr.foohdr.isValid : exact;
            ^^^^^^^^^^^^^^^^^^
header-field-name-collision.p4(80): error: Key hdr.foohdr.setValid field type must be a scalar type; it cannot be void function()
            hdr.foohdr.setValid : exact;
            ^^^^^^^^^^^^^^^^^^^
header-field-name-collision.p4(81): error: Key hdr.foohdr.setInvalid field type must be a scalar type; it cannot be void function()
            hdr.foohdr.setInvalid : exact;
            ^^^^^^^^^^^^^^^^^^^^^

I believe that this is reasonable behavior for p4c, by the way, and am not recommending that it be changed. I am simply adding this comment to point out that if this is considered correct behavior, then adding new methods on headers will make those method names no longer usable as field names in headers. header-field-name-collision.p4.txt

mihaibudiu commented 5 years ago

We could have the compiler allow overloading of these field names, under the assumption that the function field names must always be called. Since we don't have lambdas or function pointers this is reasonable for now.

mihaibudiu commented 5 years ago

Called = used within a MethodCallExpression.

jafingerhut commented 5 years ago

Created a draft PR here: https://github.com/p4lang/p4-spec/pull/712

The only change from the comments above is that I defined the operation to treat any varbit fields as length 0, not their maximum value. I now believe that to be more useful than using the maximum value. One motivating example: If one defines an IPv4 header type with a varbit field for options, being able to get the size of 20 bytes for the base IPv4 header without options seems to me much more useful than getting back the value of 60 bytes for the maximum possible length of any valid IPv4 header with options.

hesingh commented 5 years ago

From the PR Andy created above in #712, there is one comment copied below.

"Maybe this should be done in conjunction with modules - we can then have stuff in core.p4 be in a module std, and you can perhaps say std.sizeInBits after you import std..."

Using std.sizeInBits is a perfect solution because it can be used anywhere in code such as in parser, deparser, control, and action. However, the module system for p4c is one year away.

If the community cannot wait a year, here is a thought.

Develop a new extern for use in core.p4. Note core.p4 already uses the packet_in and packet_out externs. I would define a new extern and use it for header size methods. The name of the new extern is open to suggestions. One obvious choice is "ext_header".

For P4-16 code, the use would be like this: bit<32> len = 0; len = ext_header.sizeInBits(hdr);

Note, packet_in already uses methods such as advance and length.

mihaibudiu commented 5 years ago

The name of the extern will already polute the namespace.

hesingh commented 5 years ago

I see the requirement saying "use a variable with an example such as sizeInBits(h) where h is a header". If the extern doesn't work, then, I don't see how we can support the example above.

So, do we go back to type.sizeInBits()?

hesingh commented 5 years ago

I am trying to understand the P4 program for type.size(). Would the program look like the code below? The compiler can raise an error if size() is used with anything but a P4 header. The reason I asked the question is because in the code below, for hdr.ipv4, is hdr a variable and hdr.ipv4 a type? It would be best to give an example of using type.size() in a P4 program.

action set_len() {
    hdr.ipv4.totalLen = hdr.ipv4.size() + hdr.udp.size() + hdr.my_proto.size();
}
mihaibudiu commented 5 years ago

Yes, except that ipv4 is not a type, it is an expression.

hesingh commented 5 years ago

Ok.

However, internally in the compiler we use the expression to get to the ipv4 header and then use the size() operator.

hesingh commented 5 years ago

I have filed a new Issue to keep implementation details out of spec changes being discussed here.

p4lang/p4c#1717

hanw commented 5 years ago

I came across a use case that might worth considering when designing the size operation on headers. The use case is kind-of target-specific. Nonetheless, I am hoping the proposal would not prohibit us from expressing the intended behavior in P4-16.

On certain target, it is possible that the compiler would try to re-adjust the layout of a header by adding padding bytes in the header (for whatever reason I am not going to explain). It is not the common case, but it does happen.

header hdr {
    bit<7> field;
    bit<1> pad;
}

may become

header hdr {
    bit<7> field;
    bit<9> pad;
}

Now if we design P4-16 to have the sizeInBits() method on the type, it wouldn't work, because different instances of the same header may be treated differently by the compiler midend/backend. Each instance may have a different padding. Similarly, eliminating the sizeInBits() method in the frontend would not work, because the size is computed solely based on the syntax in the frontend code, thus prohibits the compiler to perform optimization on the header layout, which makes the sizeInBits() method unusable. (if used, the generated constant would be wrong).

I would propose to implement the sizeInBits() method on the header instance, just like what we did for isValid(), setValid() and setInvalid(). This way, the method is handled by individual target backend, and all we need to do in the frontend is to do the appropriate typechecking.

mihaibudiu commented 5 years ago

I am afraid that header padding will break many other passes. If something like this is done, it should probably be done before the front-end. We were talking about size being a compile-time constant, ideally substituted very early in the front-end, C-like.

hanw commented 5 years ago

Header padding is done in the backend. Frontend wouldn't know that padding ever happened, so it wouldn't break any frontend pass. From a user's perspective, the header layout is specified in the p4 program. It can be emitted, extracted according to the layout, and even advanced with the size of the header instance (symbolically). From the compiler's backend perspective, the layout that user provided for a header may not be the most optimal layout to program to the hardware, we need to allow compiler to optimize the layout. If the size of a header is substituted with a constant very early in the frontend, the compiler backend would have a very difficult time to undo the substitution and perform the optimization to header layouts.

For instance, if the sizeInBytes() method is substituted with a constant in the frontend.

header hdr {
   bit<7> field;
   bit<1> dummy;
}

pkt.advance(hdr.sizeInBytes());

would become

header hdr {
   bit<7> field;
   bit<1> dummy;
}

pkt.advance(1);

and if the compiler backend decides to optimize the layout to

header hdr {
   bit<7> field;
   bit<9> dummy;
}
/// the constant in advance() method would be wrong.
pkt.advance(1);
hesingh commented 5 years ago

I agree with Mihai.

Any backend padding/optimization that a target does, can be performed before the frontend as well, agree?

So, why not do that and (a) save p4c from adding any padding, (b) support the sizeof feature which the community is asking for, and (c) p4runtime has an impact if padding is done on a header by the backend.

Thus, let's decide upon a schema/format for what a target wants to do towards padding etc. and do the work before the frontend. thanks.

hanw commented 5 years ago

I don't agree with padding can be done before frontend. The information that the backend uses to decide what to pad and how many bits to pad is not available before the frontend.

I am asking not to substitute sizeof method with a constant in the frontend. It is fine by me to do the substitution in a midend pass.

hesingh commented 5 years ago

What do you do with P4Runtime which flattens headers before midend is run?

I would like to understand better why padding needs to happen after frontend. I see all the frontend passes in p4c/frontends/p4/frontend.cpp. Off the cuff, I don't see a pass that has impact on header or struct in terms of changing sizes. It would help to articulate what frontend passes operate that help a backend figure out how many bits to pad.

hanw commented 5 years ago

In the use case that I cam across, the header that is padded does not go to the control plane. So flattening in P4Runtime has no impact.

With respect to why we need to add padding, I am afraid that involves hardware specifics, which I cannot say too much about in an open-forum like p4c repo.

None of the frontend pass is going to help the backend to figure out how many bits to pad. It is purely a target-specific backend thing, and requires some target-specific analysis on hardware resource, etc. All I am asking is do not transform the program in a way that make certain backend difficult to write, and leave the choice to the backend implementer.

jafingerhut commented 5 years ago

Out of curiosity, is this header going out of the switch on a link, to another device? If so, that seems like kind of unpredictable behavior that the other device may or may not know how to parse, yes?

I am guessing you have already considered the consequences, and have reasons to go forth with this plan, so will not argue too hard against you, but I just wanted to point out that the P4_16 spec has had the following sentence in section "Header types" since before Oct. 2016, when it was added in the git commit log: "There is no padding or alignment of the header fields."

hanw commented 5 years ago

@jafingerhut This header does not go out of the switch. It's the user-defined header that goes from ingress deparser to egress parser.

mihaibudiu commented 5 years ago

How is the user supposed to use this function in your setting? If we called this function declaredSizeInBits would you be happy? If in your architecture headers change sizes and the user needs to know when and where you should just define an architecture-specific function for the user to fond out. That should not preclude us from defining this function in the language definition.

hesingh commented 5 years ago

I think it's fine to leave off structs for now. Later on, if there turns out to be a need to get the size of a struct, we can always add that. With #383 coming down the pipe, though, I can see it soon being useful to at least talk about the size of a bit-vector struct.

From day one, p4c/ir/type.def supports an implementation for width_bits() which supports nested structs for width computation. The new code for sizeof in p4c sent out for PR uses width_bits() to compute the sizeof.

hanw commented 5 years ago

After pondering on this a bit more, I think what I need is the serializable struct concept that AndyKeep(?) proposed before. What I need is something that's like a header, because it is serialized/de-serialized, but it is not a header, because the layout might be changed by the compiler. It is also not a struct, because it can only have serializable types in it.

In the P4-16 spec, we could say that the size of a serializable struct cannot be determined by the frontend. It must be decided by the compiler midend or backend. The size of a 'pure' header is decidable in the frontend, except the header that has a serializable struct inside and the header that has a varbit in it.

Now if we allow header to have a nested serializable struct, we probably do not need header to have a nested struct anymore. The nested struct support in the frontend is basically implementing serializable struct, as it checks for Type_Struct and checks the members in the struct to be types that can be serialized. This can potentially simplify how to write the spec for nested struct in P4-16. There will be less confusion on what can be nested with what.

At this moment, if we allow header to nest struct, then we can have

It raises more questions like: Can we have a header that nests a struct that nests a header?

So it is basically a mess.

If we use serializable struct, then the definition of nesting would be cleaner.

We had a proposal for serializable struct, maybe it is the time to revive that discussion in LDWG?

hesingh commented 5 years ago

Mihai is gone for two weeks. He should weigh in first. Recall, in the nested structs discussion, it has been said creating a new IR for every new type of feature such as nested struct does not make sense. A new serializable struct would need new IR.

At this moment, if we allow header to nest struct, then we can have

  • struct inside a struct,
  • struct inside a header and the struct can only have bit types in it,
  • header inside a struct,
  • but not header inside a header

It raises more questions like: Can we have a header that nests a struct that nests a header?

No. The onlyBitsOrBitStructs() check will fail. The check is very clear to only allow serializable fields.

So it is basically a mess.

We had a proposal for serializable struct, maybe it is the time to revive that discussion in LDWG?

IMHO, the Feb. 25th LDWG should first discuss any pending issues with recirculate etc. and this sizeof issue.

jafingerhut commented 5 years ago

Han, just a quick question -- I do not know all of the constraints you may be trying to satisfy all at once, but here is a copy of most of your first paragraph: "What I need is something that's like a header, because it is serialized/de-serialized, but it is not a header, because the layout might be changed by the compiler. It is also not a struct, because it can only have serializable types in it."

Why not use a struct, and if the struct is used in this particular way you have in mind, and it has field in it with types you don't support for that use case, you give an error message for that use?

Alternately, for your use case it sounds like you could allow types like error and enum (without a base/backing type), because the struct is being passed from inside the device to a destination inside the device, so you can use whatever custom representation you want for those types and it will work for your target?

hanw commented 5 years ago

@jafingerhut Struct would not work, because a struct like below cannot be emitted or extracted.

struct mystruct_t {
   bit<9> field;
}

parser (mystruct_t st) {
   state start {
       pkt.extract(st);
   }
}

This on the other hand, could be legal.

serializable struct mystruct_t {
   bit<9> field;
}

parser (mystruct_t st) {
   state start {
       pkt.extract(st);
   }
}

No. The onlyBitsOrBitStructs() check will fail. The check is very clear to only allow serializable fields.

@hesingh Precisely! The onlyBitsOrBitStructs is basically checking if a struct is a serializable struct. Why not just call the IR node IR::Type_SerStruct? Then the validator functor on line 1356 in typeChecker.cpp becomes

auto validator = [this] (const IR::Type* t) {
    while (t->is<IR::Type_Newtype>())
        t = getTypeType(t->to<IR::Type_Newtype>()->type);
        return t->is<IR::Type_Bits>() ||  t->is<IR::Type_Varbits>() || 
                   t->is<IR::Type_SerStruct>() || t->is<IR::Type_SerEnum>() || 
                   t->is<IR::Type_Boolean>(); }

I don't see any problem with introducing a new IR node if that makes the meaning of the IR more precise.

jafingerhut commented 5 years ago

So echoing something that Mihai suggested earlier -- suppose you use a header that can silently have padding added in the target-specific back end for your use case. Does it matter if a sizeBits() operator returned the unpadded length for your use case? Who would use the operator for such an internal-to-the-device-only header? Certainly they wouldn't use it for sending out on a wire, and the main use case I know of for the sizeBits() / sizeBytes() operators are for people doing tunnel encapsulation and decapsulation for packets sent on ports to other devices.

hanw commented 5 years ago

Knowing the size of the "internal header" is useful if the program needs to recompute the size of the packet in egress pipeline. Typically, egress pipeline has access to some metadata that represents the length of the packet (could be original packet size at beginning of ingress, could be the modified packet size after ingress processing, it really depends on the data path that the packet goes through). If the program needs to update the ipv4 length field in egress, it needs to know about the size of the 'internal header', because the intrinsic metadata that count the packet size may/may not accounts for the 'internal header', we need the flexibility to express this packet length computation in P4.

jafingerhut commented 5 years ago

OK, another thought, and hopefully I can stop thinking about this for at least the weekend :-)

You want something like a header, in that it can have fields inside of it of various types, and you want to be able to emit() it and get out a target-specific sequence of bits, and later extract() the result on that target-specific sequence of bits and get back the original contents.

Sounds to me like a struct fits the bill. Probably not as implemented in the current p4c and bmv2, but the idea has these advantages:

Unless I completely misinterpreted what the serializable struct goals were, I thought that one of them was trying to make the sequence of bits that were produced from emit to be defined by the spec, portably across targets. That is 100% not the goal with what you are doing.

hanw commented 5 years ago

@jafingerhut I looked up the proposal for serializable type (attached). serializable.pdf There are a few paragraphs that are relevant to this discussion.

Where serializable data is used elsewhere, such as tuples passed in to an extern like the IP checksum extern, we expect that data to be packed-bit data, without any padding. In particular, tuples and structs with multiple fields are expected to be read and written without any padding bits. In the case of headers, if they are treated as serializable types they will need to read and write the validity information along with the header data. ... It is worth noting that none of these recommendations are intended to force implementers to use a particular representation for data. A CPU target, for instance, might want to specify aggregate data types with padding to better match the memory model on the CPU. Or an implementation might decide to completely flatten and re-arrange data to better fit a bus between parts of a hard- ware device. The intention is to provide some flexibility to the P4 programmer. ... Serializable types need not be represented as in their serializable format internal to the data plane. The serializable layout is only required for reading from the wire or writing to the wire, or in other extern functions or methods that specify a serializable modifier. 7.1. Why might the compiler decide on a different layout? In some cases the serialized layout might impact performance when referenc- ing or updating data. For instance, on a CPU, extracting bit fields, may require shifting and masking to extract a field. In cases where the field is accessed frequently, it might make more sense to choose an internal representation that is more efficient for referencing the field, and then do the work necessary to serialize this when the serialized layout is needed. It is up to the compiler to determine what the representation should be, the serializable indicator only requires that when the value is passed to an extern function or method requiring the serialized data that it be serialized.

In fact, it seems the proposal on serializable struct fits quite nicely to my use case.

I could not find any sentence mentioning the sequence of bits produced from emit needs to be defined by the spec, portably across targets.

hesingh commented 5 years ago

@jafingerhut Struct would not work, because a struct like below cannot be emitted or extracted.

struct mystruct_t {
   bit<9> field;
}

parser (mystruct_t st) {
   state start {
       pkt.extract(st);
   }
}

This on the other hand, could be legal.

serializable struct mystruct_t {
   bit<9> field;
}

parser (mystruct_t st) {
   state start {
       pkt.extract(st);
   }
}

Even when a struct is a new serializable struct, how does extract work when the serializable struct is inside a header or another serializable struct? The serializable struct is extracted, but other bits in the outer header/struct are not. Is the isValid bit set on the outer header/struct? You have a comment in https://github.com/p4lang/p4-spec/issues/383 that says "changing extract to support a struct is a fundamental change to P4-16." Or are you saying a serializable struct will never be nested? If so, you have added another wrinkle.

You use an internal header for internal processing; the header is not emitted. So why do you care if the internal header has emittable fields? Its your internal header, do what you want with it, including using an internal sizeof with this header. It's interesting to propose changes to a language for an asic internal representation of data store.

Currently, p4c does not support sizeof. I think the Tofino supports tunnel decap+encap, doesn't it? Without sizeof, how is that working? I don't see why the same internals won't work for ingress deparser to egress parser path.