Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PSA] Hash function #306

Closed
hanw opened this issue Jun 25, 2017 · 8 comments
Closed

[PSA] Hash function #306

hanw opened this issue Jun 25, 2017 · 8 comments

Comments

@hanw
Copy link
Contributor

hanw commented Jun 25, 2017

Section on "Hash function". There is no description of what the 'max'
parameter of getHash does.

If the intent is for hashes and checksums to share the type
'HashAlgorithm', then HashAlgorithm should have the 16-bit 1's
complement checksum used by the IPv4 header to its choices.

The spec might want to note that 'remove' will only work for some hash
functions, and not others, e.g. it is easy to remove some data from a
1's complement checksum calculator, but much more complex, and
position-dependent, to remove it from a CRC calculation.

@jafingerhut
Copy link
Collaborator

Is it reasonable to proceed on issues like this one by opening a pull request with proposed changes to the text, and let the discussion be on the PR?

@cc10512
Copy link
Contributor

cc10512 commented Jul 7, 2017

@jafingerhut, yes, please!

@jafingerhut
Copy link
Collaborator

I will create such a PR, but another issue I found is that if you try code like this with the latest version of p4test from the p4lang/p4c repository, it gives an error.

Snippet of code:

enum HashAlgorithm {
  crc32,
  crc32_custom,
  crc16,
  crc16_custom,
  random,           /// are random hash algorithms useful?
  identity
}

extern Hash<HashAlgorithm, O> {
  Hash();
  O getHash<T, D, M>(in T base, in D data, in M max);
}

// ... later ...

parser P(packet_in pkt, out Parsed_packet hdr) {
  Hash<HashAlgorithm.crc16, bit<16>>() h;
  state start {
    bit<16> hash_value = h.getHash((bit<16>) 16, hdr.ethernet.srcAddr, (bit<16>) 100);
    transition accept;
  }
}

I get an error like the following from the latest 2017-Jul-05 version of p4test:

psa2.p4(396):syntax error, unexpected ., expecting > or ","
  Hash<HashAlgorithm.
                    ^
error: 1 errors encountered, aborting compilation

Is this something that you expect to be correct syntax in P4_16? The grammar seems to allow it, so not sure why the error message. I can open an issue on the p4lang/p4c repo if desired.

@cc10512
Copy link
Contributor

cc10512 commented Jul 7, 2017

@hanw noticed it as well and we believe this is a bug in the compiler. I recall some discussion between him and @mbudiu-vmw, but don't recall the conclusion. I should think that one of the enum members should be a valid type for the instantiation of the extern. So we''ll have to fix the compiler.

@hanw
Copy link
Contributor Author

hanw commented Jul 7, 2017

Issue #492 in p4lang/p4c. Solution is to pass constant value as a constructor parameter.
p4lang/p4c#492

@jafingerhut
Copy link
Collaborator

@hanw Thanks. Yes, that makes sense. My PR for the PSA document can include this change to the Hash extern as well.

@jafingerhut
Copy link
Collaborator

Created PR #346

@cc10512
Copy link
Contributor

cc10512 commented Jul 8, 2017

addressed by #346.

@cc10512 cc10512 closed this as completed Jul 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants