• Don't forget to clear the bit on inpcb disconnect/free. Thanks, Vova!

In D58281#1337102, @bnovkov wrote:

Thank you for the offer, but I was trying to avoid unnecessary churn if the same thing can be achieved with ifa_ifwithnet. If you have future uses for a fib-aware version of ifa_ifwithaddr() I can implement it as well.

Some performance data. The test that pushes local port allocation to the corner run on CURRENT as base, then D58131 as unlock and then on D58131 and this revision combined as cache. Using virtual machine with 8 CPUs.

x base
+ unlock
* cache
+------------------------------------------------------------------------------+
|*                                                                             |
|*                                                                             |
|*                                                                             |
|*                                                                             |
|*                                                                             |
|*                                                                             |
|*    +                                                                        |
|*    ++                                                                       |
|*    ++                                                                       |
|*    +++++                     xx x    x x       xx          x  x            x|
|A    |A|                        |____________M__A______________|              |
+------------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  10         14.44         36.24         21.28        22.525     7.2199696
+  10          2.39          4.14         2.835         3.021    0.59897041
Difference at 95.0% confidence
        -19.504 +/- 4.81339
        -86.5882% +/- 3.35841%
        (Student's t, pooled s = 5.12283)
*  10          0.14          0.21          0.14         0.147   0.022135944
Difference at 95.0% confidence
        -22.378 +/- 4.79693
        -99.3474% +/- 0.153552%
        (Student's t, pooled s = 5.10531)

IMHO, the check shall be located in in_pcbbind_avail() in the block that has the comment * Is the address a local IP address?. We need ifa_ifwithaddr() version that has fibnum argument. Many other functions in the ifa_if* family already have it. The IPv6 version is similar.

The struct in_endpoints can also be anonymized easily, but that would highly likely affect something outside of FreeBSD. Another level of #define to be removed. Let me know what you think about this and I will dig deeper into this if we all agree.

Note: union in_dependaddr can be anonymized after small cleanup of lbgroup code and siftr(4). This removes another level of of defines for both IPv6 and IPv4. This may need universe/exp-run, though.

• Rebase. Should now apply to main.

I got another naive benchmark. It exercises situation when we create as much as possible connections from us to a single peer:port eventually running out of ports. This exercises the worst case of the cycle in in_pcb_lport_dest(), which at the end of the test is executed for 10^4 times to find a port. While the cycle itself is not optimized at all, on a virtual machine with 8 CPUs the test passes 6-7 times faster than before the change.

Committed 57cc01057bb21fec37fa4fb7416b4a1f29a211c0

Thanks! Note that when doing such cleanups it is important to pay attention to the fact that bcopy(9) allows overlapping memory regions and memcpy(9) does not. In this particular patch everything looks fine.

I got naive microbenchmark. It allocates ncpu threads, and each threads creates n UDP sockets that are connect(2)-ed to a random valid address:port. In a test virtual machine with 8 CPUs and 10000 sockets per thread I got 6x-7x speedup. If the test is adjusted to close(2) sockets when 50% allocation is reached, so that both insertions and removals are exercised at the same time, the speedup is roughly the same. I didn't yet do any real tests on hardware, but benefits are clear.

No objections left from me.

In D57389#1333043, @zarychtam_plan-b.pwste.edu.pl wrote:

Unfortunately, I can't test this or the other optimisations in a production environment. There are simply too many commits that would need to be cherry-picked manually, and not all of them can be cleanly MFC'd.
@pouria, I think it would be worthwhile to merge at least the most important of your improvements and fixes into stable/15. I don't know whether there would be consensus for that, and this is probably not the best place for such a discussion, but there is still plenty of time before the 15.2-RELEASE release. Having those changes in stable/15 would allow them to receive much broader testing in real-world deployments.

Let me suggest a better KPI for wlock iterator. Instead of adding a wrapper function, just add const bool wlock member to struct nhop_iter. In that case all you need is to set it true in the initializer. Use same function to start and stop the iterator.

Didn't know about this one. Why didn't you push?

+ ObsoleteFiles.inc

Can you please make the pad variables unsigned? (and while here the associated length variables as well)

Thanks!

I decided to put this change on hold for a while.

I decided to put this change on hold for a while.

Duplicate of D57986

Yes, I was afraid of such problem when did this. Thanks for fixing!

I understand the change to pf_route(), but I do not understand the change to pf_test(). Why the second one is required? pf(4) definitely can test (read IP/TCP level headers) of mbuf chains that have M_EXTPG later in paylod.

I still can't understand :(

Thanks a lot! Didn't review the awk/sed code, since not an expert in it.

Cause we actually plan in the opposite direction: packets known to be valid (never went over a wire) shall not be checked.

Thanks! Stupid question: how did that happen that after Timo's change the loopback actually does check checksums?

• Add tunable to fallback to old behavior.
• Adjust tests.
• Document.

In D57865#1329350, @des wrote:

Most enums use names distinctive enough to handle with a regex just like we do for #defines. For the few cases where that doesn't work, we can use awk to limit the search to the lines between enum { and };. Give me a few days to come up with a poc, and please approve D57915 in the meantime.

EDIT: perhaps you forget that mktables doesn't need to know the value of each constant, it just needs the name.

I don't want this library to dictate naming of enums all around. The whole idea of using text parser to parse C files is fragile. Clang delivers a proper solution. Hopefully gcc will catch up.

Yes, patch as is doesn't improve anything. The comment above the function clearly marks the intent: the function checks if packet should be dropped and if so provides error code. It doesn't provide error code in case packet shouldn't be dropped.

Can you please review my version instead? :) https://reviews.freebsd.org/D57865

see ece716c5d34728a170f1dfe1b3389c267d6ddd1e

Hi Neel,

In D57851#1327098, @kp wrote:

Do you happen to know of a way to get an interface (struct ifnet) without AF_INET6? I used to use pflog0 for this, and now clearly can't any more. (The test for PR 288263 does that.)
Ideally I'd like to not remove the test, but without such an interface there's no point to it.

• Pushed one to final version. Fixup.

bpf_attach() uses M_WAITOK (that's why it doesn't fail). This lock should be sx(9). IMHO, it should be asserted both in call to pflog_create() and pflog_destroy(). The startup calls to pflog_create() should also acquire the lock, just for consistency.

Note that there is a parent revision!