RFE: improve rule add performance through shadow transactions #180
Conversation
Pay back some of the technical debt in db_col_rule_add(), no logic changes in this patch, just removing some code duplication. Signed-off-by: Paul Moore <paul@paul-moore.com>
|
FYI, I added a new patch to the stack in this PR to fix some issues. I'll squash this patch into the main shadow transaction patchset later, but I wanted to keep it separate for the moment in case others were testing the earlier, two-patch, version of the this PR. |
Creating a transaction can be very time consuming on large filters since we create a duplicate filter tree iteratively using the rules supplied by the caller. In an effort to speed this up we introduce the idea of shadow transactions where on a successful transaction commit we preserve the old transaction checkpoint and bring it up to date with the current filter and save it for future use. The next time we start a new transaction we check to see if a shadow transaction exists, if it does we use that instead of creating a new transaction checkpoint from scratch. Signed-off-by: Paul Moore <paul@paul-moore.com>
|
Refreshed the PR with the last two patches squashed as discussed above. |
| * isn't generally useful. Returns zero on success, negative values on error. | ||
| * | ||
| */ | ||
| static int _db_col_rule_add(struct db_filter *filter, |
There was a problem hiding this comment.
I really like this change. db_col_rule_add() was getting pretty big, and splitting this out really helps.
Aside - I am wondering if we want to introduce unit testing for some of these complex internal functions. We may be able to catch some difficult corner cases and improve code coverage.
There was a problem hiding this comment.
I really like this change. db_col_rule_add() was getting pretty big, and splitting this out really helps.
Yeah, this had been marked as a TODO for some time, instead of making yet another copy-n-paste of the same code it seemed like it was time to pay down this debt :)
Aside - I am wondering if we want to introduce unit testing for some of these complex internal functions. We may be able to catch some difficult corner cases and improve code coverage.
Unless there is no way to avoid it, I would prefer the testing boundary to stay with the published libseccomp API; that's what matters to callers, so that's what we want to test/guarantee. A lot of code coverage misses fall into two categories: transactions are not available in the API, we don't measure seccomp_load() operations. I just created a issue for the former (this will be nice to have in the API, and we need it to something pledge-esque), and I've got a PR queued for the later (along with a number of other code coverage improvements) but PR #182 needs to land first.
| struct db_filter **tmp_f; | ||
|
|
||
| /* add filters */ | ||
| tmp_f = realloc(snap->filters, |
There was a problem hiding this comment.
I noticed the code in this if block wasn't hit with code coverage. Looks like this could be a place we could hit with a surgical unit test.
https://coveralls.io/builds/26761036/source?filename=src%2Fdb.c#L2497
There was a problem hiding this comment.
If we expose the transactions to the public API it should be easy enough to test this.
| continue; | ||
| if (rule_s != NULL) { | ||
| do { | ||
| rule_o = rule_o->next; |
There was a problem hiding this comment.
Could there be a case where we dereference a NULL pointer here? We're advancing rule_o to rule_o->next but never checking if rule_o has become NULL.
There was a problem hiding this comment.
Since we don't support removing rules, the col->filters[iter] list is always going to have at least as many entries as the snap->filters[iter] list.
|
FYI @drakenclimber I'm going to take your approval of this PR as an ACK and mark the commits accordingly, if that's not the case let me know and we can revert the commits. |
libseccomp 2.4 introduced a serious performance regression. Backport the fix. seccomp/libseccomp#153 seccomp/libseccomp#180
See the commit descriptions and GH issue #153 for more information.