Checksums #421

jnfoster · 2017-09-18T03:23:47Z

This PR implements changes discussed in preceding working group meetings. Note that I missed the last meeting, so I am going from the notes and discussions with those who attended -- i.e., it possible something got lost in translation.

The major changes include:

Remove ComputeChecksum control and require checksum computation and update to be done in deparser.
Clarifying that incremental checksumming is only supported for CRC{16,32}.

To discuss:

Should we indeed move checksum updates to deparser?
What do to with deparser direction annotations? Can copy the headers to a new variable, but it's a shame that we have to do that.
Is my undersatnding of the intended restrictions on incrmental checksums correct? In particular, calling update many times is fine, but calling remove is only okay for CRC{16,32}.
Add an example of incremental TCP checksum?

jafingerhut · 2017-09-18T11:24:34Z

I would think that remove would be difficult to implement for CRCs, and in fact I think it is impossible unless you indicate the 'bit position range' of the data you are removing/inserting.

Restricting remove to whatever name we use for the IPv4/TCP/UDP Internet checksum seems reasonable. I think it might currently be called ones_complement16 in psa.p4 right now (there may be benefits to renaming that to whatever P4_14 uses now).

Changing the direction of the headers parameter to the deparser from 'in' to 'inout' is a straightforward way to allow the checksum fields to be modified in place. I understand that has the down side that the compiler would then permit arbitrary header fields to be modified in the deparser, without issuing any errors for doing so.

An alternative is to require the checksums to be updated at the end of the ingress or egress control blocks, but the down side there is that it makes P4_16 statements like 'return' and 'exit' harder or impossible to use, without also skipping the checksum updating.

mihaibudiu · 2017-09-18T16:06:52Z

You could also consider having two types of checksum blocks: Checksum and IncrementalChecksum, where the latter has a remove method, but the former does not.

cc10512 · 2017-09-18T17:01:55Z

p4-16/psa/examples/psa-example-parser-checksum.p4

+// BEGIN:Compute_New_IPv4_Checksum_Example
+control DeparserImpl(packet_out packet, in headers hdr_) {
+    Checksum<bit<16>>(HashAlgorithm_t.ONES_COMPLEMENT16) ck;
+    headers hdr = hdr_; //TBD: or better, change declaration to inout?


What are the implications of changing the argument to inout? Are we then going to rely on convention that certain operations are not allowed?

jnfoster · 2017-09-18T21:37:43Z

Resolved at 9/18 meeting:

Eliminate ComputeChecksum and move checksum computation/update to deparser.
Change in parameters to deparser to inout to allow for said updating.
Remove remove method from checksum extern.
Add a new extern for csum16checksumming that supports remove.
No support for TCP payload -- must be done incrementally.

I will take these marching orders and update the PR.

jnfoster · 2017-09-29T16:25:27Z

@jafingerhut

I believe I've made the changes we agreed on.

Are you okay with calling the one's-complement incremental extern "Internet Checksum"?
If the Internet checksum is always 16 bits, does it make sense to delete the width type parameter W?

jafingerhut · 2017-09-29T20:39:20Z

Given that it is used for IPv4 headers, and TCP and UDP, Internet Checksum seems as good a name as any.

I am not aware of any other checksums that are incrementally updatable and commonly used in networking protocols. If such a thing arises, it seems reasonable to add a new extern to P4_16 for that purpose. Thus fixing InternetChecksum to input that is multiples of 16 bits wide, and output that is 16 bits wide, seems reasonable.

missing the declaration of type parameter `T`.

* Add comments to Checksum extern clarifying that `remove` is only supported for CRC hash functions.

deparser. * Update PSA.mdk text, adding a note on when incremental computation is (not) supported. * Still pending: example of incremental TCP checksum?

* Add `InternetChecksum` with support for `remove`.

Remove extraneous whitespace

jnfoster · 2017-10-04T16:15:39Z

I've made the following changes:

Modified the type signature for the deparser to the following:

control Deparser<H, M>(packet_out buffer, inout H hdr, in M user_meta);

Specifically I added the user metadata as an in parameter.

Added an example of computing an incremental TCP checksum.
Fixed various typos and ran examples through p4test to verify that they are syntactically correct.

cc10512 · 2017-10-13T18:59:35Z

@jnfoster what's the conclusion on adding the init method?

EDIT by @jafingerhut after this PR was already merged in -- @cc10512 My conclusion is that while we could have a separate init() method that we recommend people call when doing incremental checksum updates, but that the implementation of init() would be exactly the same as remove(). I don't mind having a separate init() method if we think that is a more understandable or cleaner API, somehow.

jafingerhut · 2017-10-13T20:53:05Z

p4-16/psa/examples/psa-example-incremental-checksum.p4

+       ck.clear();
+       ck.remove(hdr.tcp.checksum);
+       ck.remove(user_meta.fwd_metadata.old_srcAddr);
+       ck.remove(hdr.ipv4.hdrChecksum);


The TCP pseudo-header includes the IPv4 source and dest address, but not the IPv4 header checksum, so this remove call, and the ck.update(hdrChecksum) call below, should be removed. RFC 793 is a good reference for the contents of the TCP pseudo-header.

I am going to go ahead and approve and merge in these changes, and then make the small fix suggested above on top of that.

jafingerhut · 2017-10-13T20:54:42Z

p4-16/psa/examples/psa-example-incremental-checksum.p4

+        hdrChecksum = ck.get();
+        // Update TCP checksum
+       ck.clear();
+       ck.remove(hdr.tcp.checksum);


Nate, you asked me in email earlier whether remove() is correct to do here, and at the time I wasn't sure. I am pretty darned sure now that this is a correct way to do an incremental TCP checksum, primarily due to RFC 1624 and my own writing and thinking on the properties of the one's complement checksum.

@jafingerhut ,

While this example is totally valid, it is probably not the way incremental checksums are computed in practice.

Instead, the following happens:

In the parser you compute the value of payload checksum by removing all the fields that TCP checksum is computed over from hdr.tcp.checksum and store it in the metadata, e.g. meta.tcpPayloadChecksum.

In the control, some of the fields that participate in tcp checksum are modified (which is the whole reason to recompute the checksum in the first place)

In the deparser, the TCP checksum is initialized with meta.tcpPayloadChecksum and then all the fields that TCP checksum is computed over are added to that checksum. Finally you do ck.get() and store it in hdr.tcp.checksum and then hdr.tcp is emit()ed.

As an optimization, one can remove/add only the fields that are actually modified in step 2.

@vgurevich So a separate parser state for parsing a TCP header after IPv4 vs. parsing a TCP header after IPv6, if both IPv4 and IPv6 are supported in a P4 program, then? Either that, or you need a single parser state that has an 'if' condition that distinguishes those two cases, and I'm not sure whether you'd be interested in mandating support for if statements inside of a parser state.

@jafingerhut , Yes, something along those lines (unless we want to revisit the separate ChecksumVerification control :) ). If() statement is more readable, but it is way more dangerous.

Another option is to instantiate the extern outside (of any parser state) and then remove() IPv4/IPv6 fields in the corresponding states whether or not there is a TCP/UDP header afterwards. Then in the TCP parsing state, you add() tcp.CheckSum and remove() the rest of tcp fields. Same for UDP.

It's all not very pretty, but can probably be partially blamed on the quirkiness of the original (i.e. TCP/UDP) design.....

We can try to write a couple of examples and see which one is more palatable.

jafingerhut

I will make a couple of minor fixes after merging this in.

jnfoster · 2017-10-14T22:09:04Z

Ooh, that's nice. Shall we change the example?

…

-N

On Sat, Oct 14, 2017 at 2:37 PM, Vladimir Gurevich ***@***.*** > wrote: ***@***.**** commented on this pull request. ------------------------------ In p4-16/psa/examples/psa-example-incremental-checksum.p4 <#421 (comment)>: > + ck.update({ hdr.ipv4.version, + hdr.ipv4.ihl, + hdr.ipv4.diffserv, + hdr.ipv4.totalLen, + hdr.ipv4.identification, + hdr.ipv4.flags, + hdr.ipv4.fragOffset, + hdr.ipv4.ttl, + hdr.ipv4.protocol, + //hdr.ipv4.hdrChecksum, // intentionally leave this out + hdr.ipv4.srcAddr, + hdr.ipv4.dstAddr }); + hdrChecksum = ck.get(); + // Update TCP checksum + ck.clear(); + ck.remove(hdr.tcp.checksum); @jafingerhut <https://github.com/jafingerhut> , While this example is totally valid, it is probably not the way incremental checksums are computed in practice. Instead, the following happens: 1. In the parser you compute the value of payload checksum by removing all the fields that TCP checksum is computed over from hdr.tcp.checksum and store it in the metadata, e.g. meta.tcpPayloadChecksum. 2. In the control, some of the fields that participate in tcp checksum are modified (which is the whole reason to recompute the checksum in the first place) 3. In the deparser, the TCP checksum is initialized with meta.tcpPayloadChecksum and then all the fields that TCP checksum is computed over are added to that checksum. Finally you do ck.get() and store it in hdr.tcp.checksum and then hdr.tcp is emit()ed. As an optimization, one can remove/add only the fields that are actually modified in step 2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#421 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi0iwitujWu-ZCWugllsyQCXXqb8Zkks5ssP9ugaJpZM4PaZbn> .

vgurevich · 2017-10-15T19:03:26Z

p4-16/psa/examples/psa-example-incremental-checksum.p4

+    apply {
+        // Update IPv4 checksum
+        ck.clear();
+        ck.update({ hdr.ipv4.version,


Also, maybe for clarity we should format this like so:

ck.update({ /* 16-bit word 0 */ hdr.ipv4.version, hdr.ipv4.ihl, hdr.ipv4.diffserv, /* 16-bit word 1 */ hdr.ipv4.totalLen, /* 16-bit word 2 */ hdr.ipv4.identification, /* 16-bit word 3 */ hdr.ipv4.flags, hdr.ipv4.fragOffset, /* 16-bit word 4 */ hdr.ipv4.ttl, hdr.ipv4.protocol, /* 16-bit word 5 skip hdr.ipv4.hdrChecksum, */ /* 16-bit words 6-7 */ hdr.ipv4.srcAddr, /* 16-bit words 8-9 */ hdr,ipv4.dstAddr });

The point I am trying to make is that the update() method of the InternetChecksum extern can only take data that is short-sized (i.e. 16*N bits wide).

Obviously, we do not have a way to express it syntactically, but I think it should be documented and the compiler should enforce that bit-width of T is a multiple of 16.

Agreed that the update() and remove() methods should restrict their inputs to be multiples of 16 bits long, and this ought to be documented. That change is included in this PR: #440

It does not update the format as shown above, though -- certainly an easy change to make, and no objections from me.

cc10512 reviewed Sep 18, 2017

View reviewed changes

jnfoster mentioned this pull request Oct 3, 2017

Introduced new checksum APIs in v1model.p4 p4lang/p4c#939

Merged

jnfoster added 13 commits October 4, 2017 10:57

Fix typo in PSA: the emit method for the recirculate extern was

154ae2b

missing the declaration of type parameter `T`.

* Remove ComputeChecksum control and blocks in PSA switch.

137caf5

* Add comments to Checksum extern clarifying that `remove` is only supported for CRC hash functions.

* Update P4 code referenced in PSA.mdk examples with checksum in

14d4774

deparser. * Update PSA.mdk text, adding a note on when incremental computation is (not) supported. * Still pending: example of incremental TCP checksum?

* Eliminate remove method from Checksum extern.

5654b26

* Add `InternetChecksum` with support for `remove`.

Add prose to PSA Markdown

4851c98

Remove W parameter from InternetChecksum

e0ecca1

Remove extraneous whitespace

Fix checksum code examples

585769c

Tidy prose

574cb70

Add incremental checksum example

4d09b83

Update examples

f126013

Fix errors in P4 examples

794919a

Add incremental checksum example to Markdown

e40c3d9

Add missing file

e0919d1

jnfoster force-pushed the checksum branch from 5db6e09 to e0919d1 Compare October 4, 2017 16:13

jafingerhut reviewed Oct 13, 2017

View reviewed changes

jafingerhut approved these changes Oct 13, 2017

View reviewed changes

jafingerhut merged commit 549f348 into p4lang:master Oct 13, 2017

vgurevich reviewed Oct 15, 2017

View reviewed changes

cc10512 mentioned this pull request Oct 19, 2017

PSA: Minor corrections to incremental checksum example code #440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checksums #421

Checksums #421

jnfoster commented Sep 18, 2017

jafingerhut commented Sep 18, 2017

mihaibudiu commented Sep 18, 2017

cc10512 Sep 18, 2017

jnfoster commented Sep 18, 2017 •

edited

Loading

jnfoster commented Sep 29, 2017

jafingerhut commented Sep 29, 2017

jnfoster commented Oct 4, 2017

cc10512 commented Oct 13, 2017 •

edited by jafingerhut

Loading

jafingerhut Oct 13, 2017

jafingerhut Oct 13, 2017

jafingerhut Oct 13, 2017 •

edited

Loading

vgurevich Oct 14, 2017

jafingerhut Oct 18, 2017

vgurevich Oct 18, 2017

jafingerhut left a comment

jnfoster commented Oct 14, 2017 via email

vgurevich Oct 15, 2017 •

edited

Loading

jafingerhut Oct 16, 2017 •

edited

Loading

Checksums #421

Checksums #421

Conversation

jnfoster commented Sep 18, 2017

jafingerhut commented Sep 18, 2017

mihaibudiu commented Sep 18, 2017

cc10512 Sep 18, 2017

Choose a reason for hiding this comment

jnfoster commented Sep 18, 2017 • edited Loading

jnfoster commented Sep 29, 2017

jafingerhut commented Sep 29, 2017

jnfoster commented Oct 4, 2017

cc10512 commented Oct 13, 2017 • edited by jafingerhut Loading

jafingerhut Oct 13, 2017

Choose a reason for hiding this comment

jafingerhut Oct 13, 2017

Choose a reason for hiding this comment

jafingerhut Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

vgurevich Oct 14, 2017

Choose a reason for hiding this comment

jafingerhut Oct 18, 2017

Choose a reason for hiding this comment

vgurevich Oct 18, 2017

Choose a reason for hiding this comment

jafingerhut left a comment

Choose a reason for hiding this comment

jnfoster commented Oct 14, 2017 via email

vgurevich Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

jafingerhut Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

jnfoster commented Sep 18, 2017 •

edited

Loading

cc10512 commented Oct 13, 2017 •

edited by jafingerhut

Loading

jafingerhut Oct 13, 2017 •

edited

Loading

vgurevich Oct 15, 2017 •

edited

Loading

jafingerhut Oct 16, 2017 •

edited

Loading