libct/seccomp: enable seccomp binary tree optimization

[kolyshkin]

This makes libseccomp produce a BPF which uses a binary tree for
syscalls (instead of linear set of if statements).

This should speed up doing syscalls from containers that were created
from OCI spec containing lots of seccomp rules.

[kolyshkin]

Tests hung at

runc run [seccomp] (empty listener path) most probably due to newer libseccomp-golang.

Investigating.

[kolyshkin]

OK, this is not caused by the libseccomp-golang bump (as I have suspected), but by SetOptimize(2) call.

[kolyshkin]

The only test that hangs is runc run [seccomp] (empty listener path), others run fine.

This test does not set any seccomp rules; it also seems it is not named correctly.

[kolyshkin]

OK, so the key is to not enable binary tree optimization when there are no rules.

Also, it makes little sense to enable binary tree optimization when there are just a few rules. I arbitrarily chose 8 as the value of "a few" here.

[kolyshkin]

OK, so the key is to not enable binary tree optimization when there are no rules.

This is because of seccomp/libseccomp#370

[kolyshkin]

The only test that hangs is runc run [seccomp] (empty listener path), others run fine.

This test does not set any seccomp rules; it also seems it is not named correctly.

Opened #3415 wrt test name and purpose.

[AkihiroSuda]

This should speed up doing syscalls from containers that were created
from OCI spec containing lots of seccomp rules.

Any benchmark?

[kolyshkin]

Any benchmark?

Some are available from seccomp/libseccomp@c9df088

[kolyshkin]

Rebased.

[kolyshkin]

Any benchmark?

Some are available from seccomp/libseccomp@c9df088

@AkihiroSuda the above links has a short test result. If that is not enough, please see seccomp/libseccomp#116 and seccomp/libseccomp#152

I tried to do my own tests but apparently measuring syscall latency is some sort of dark art I am not very good at, and later I got carried away with the other stuff.

[kolyshkin]

Rebased to resolve a conflict due to #3441. @AkihiroSuda @cyphar PTAL

[kolyshkin]

@opencontainers/runc-maintainers PTAL

[kolyshkin]

Rebased on top of merged #3465

[thaJeztah]

SGTM

would be nice indeed if we had benchmarks on our side as well to allow keeping track of performance regressions

[cyphar]

Needs a small comment explaining why > 8 is the threshold.

[thaJeztah]

Needs a small comment explaining why > 8 is the threshold.

perhaps defining a const for it (where the const is documented) would be nice (just a comment would of course work).

[kolyshkin]

• Rebased to resolve conflicts.
• Added a (somewhat weak) explanation of why 8 is used as a threshold (copy/pasting it here):

       // Enable libseccomp binary tree optimization. The number 8 is chosen
       // semi-arbitrarily, considering the following:
       // 1. libseccomp <= 2.5.4 misbehaves when binary tree optimization
       // is enabled and there are 0 rules.
       // 2. All known libseccomp versions (2.5.0 to 2.5.4) generate a binary
       // tree with 4 syscalls per node.
       if len(config.Syscalls) > 8 {
               err = filter.SetOptimize(2)
               ....

@drakenclimber perhaps you have a better idea of when we should use bintree optimization (without introducing a knob for it, that it).

[thaJeztah]

still SGTM, thanks for updating!

[drakenclimber]

• Rebased to resolve conflicts.

  • Added a (somewhat weak) explanation of why 8 is used as a threshold (copy/pasting it here):

       // Enable libseccomp binary tree optimization. The number 8 is chosen
       // semi-arbitrarily, considering the following:
       // 1. libseccomp <= 2.5.4 misbehaves when binary tree optimization
       // is enabled and there are 0 rules.
       // 2. All known libseccomp versions (2.5.0 to 2.5.4) generate a binary
       // tree with 4 syscalls per node.
       if len(config.Syscalls) > 8 {
               err = filter.SetOptimize(2)
               ....

@drakenclimber perhaps you have a better idea of when we should use bintree optimization (without introducing a knob for it, that it).

The binary tree should help improve performance when the filter is large. Originally I had envisioned automatically turning it on when there were greater than X syscalls in the seccomp filter, but some users preferred to control optimizations themselves.

If you want to avoid a knob, then I would pick a somewhat arbitrary number, say 32, and enable the binary tree when the filter is larger than that.

[drakenclimber]

@drakenclimber perhaps you have a better idea of when we should use bintree optimization (without introducing a knob for it, that it).

So to actually answer your question :), no I don't have a better answer than what you've done.

[kolyshkin]

I would pick a somewhat arbitrary number, say 32, and enable the binary tree when the filter is larger than that.

Changed 8 to 32, slightly improved the comment.

[thaJeztah]

LGTM

[thaJeztah]

LGTM

[AkihiroSuda]

@cyphar LGTY?

addressed

[kolyshkin]

@cyphar LGTY?

Assuming @cyphar is good with this change, as his only request was t add an explanation for the number of rules threshold, which was added.

Let's merge it and face the consequences.