-
Notifications
You must be signed in to change notification settings - Fork 444
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
ebpf/PSA: Checksum support for fields wider than 64 bits #3801
Conversation
This commit adds support for fields wider than 64 bits in implemented checksum algorithms.
@fruffy do you know why the test case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve, but you may want to at least document the code better.
" bpf_trace_message(\"CRC%w%: data byte: %x\\n\", data[i-1]);\n" | ||
" *reg ^= (u16) data[i-1];\n" | ||
"void crc16_update(u16 * reg, const u8 * data, " | ||
"u16 data_size, const u16 poly) {\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by this code, some documentation would help.
if (addData) { | ||
builder->appendFormat("%s = csum16_add(%s, %s)", stateVar.c_str(), | ||
stateVar.c_str(), tmpVar.c_str()); | ||
builder->appendFormat("csum16_add(%s, %s)", stateVar.c_str(), tmpVar.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the result of csum16_add is ignored here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the result is not ignored. Assignment of the state variable is done 2 lines above, just before if
statement. I moved this as a common expression for csum16_add
and csum16_sub
but I see it can be little confusing.
This is a flaky test. It can be fixed by rerunning. I do not know whether the fault is in bmv2's or testgen's checksum calculation yet. |
This PR adds support for fields wider than 64 bits in implemented checksum algorithms.
CC: @osinstom