Support "default-default" entry for tables with ability to reset #551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces the following: * support for compile-time default entry for indirect tables; until now only direct tables could have compile-time default entries. Unlike a control-plane provided default entry, the compile-time default entry for indirect tables uses action + action data (not member / group handle) * ability to reset default entry to initial compile-time default entry (aka default-default entry). The Thrift IDL has been updated as well as the runtime_CLI, with support in P4 Runtime to follow. These changes should be able to accomodate both P4_14 & P4_16 and both bmv2 compilers. I believe I also found a possible race condition when indirect match tables share the same action profile and it should be fixed now.
While implementing support for compile-time default entries for indirect tables, I realized that the locking for action profiles was completely wrong. I think that it has been broken since action profile sharing support was introduced, so it must have been broken for a year... In particular, the action profile mutex wasn't held during the action entry execution, which means that modifying the action profile while applying an action from the action profile on a packet could result in a race condition and the corruption of the action data used by the action. I found & implemented a temporary solution by adding virtual functions to MatchTableAbstract to acquire the action profile mutex (for MatchTable, the virtual functions return an empty lock). I'm confident I can find a more elegant solution if I have time...
The ref count was being incremented, which means we could end up with dangling default entries.
Instead just log an error and treat it as an error.
Codecov Report
@@ Coverage Diff @@
## master #551 +/- ##
==========================================
+ Coverage 76.15% 76.37% +0.21%
==========================================
Files 109 109
Lines 9106 9181 +75
==========================================
+ Hits 6935 7012 +77
+ Misses 2171 2169 -2
Continue to review full report at Codecov.
|
|
I found more race conditions in the action profile implementation which for some reason I have completely missed in the last year. So this PR ended-up being much more than supporting default-default entry... |
antoninbas
deleted the
antonin/support-default-default-entry-for-match-tables-with-reset-operation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces the following:
now only direct tables could have compile-time default entries. Unlike
a control-plane provided default entry, the compile-time default entry
for indirect tables uses action + action data (not member / group
handle)
(aka default-default entry). The Thrift IDL has been updated as well
as the runtime_CLI, with support in P4 Runtime to follow.
These changes should be able to accomodate both P4_14 & P4_16 and both
bmv2 compilers.
I believe I also found a possible race condition when indirect match
tables share the same action profile and it should be fixed now.