This structure is used for KPI between TCP and the TCP hostcache module. An API structure for userland tools to fetch (possibly inject?) TCP hostcache entries should not be mixed with this one.

In D58356#1339476, @rscheff wrote:

• provide a way to check for the mere existance, and contents of an entry for a given IP address.

Sorry for being slow. This review is on my TODO list.

Fixup comments:

• Remove comment about alignment. inpcb doesn't have IPv6 addresses aligned and the RFC talks about alignment of API structures, specifically sockaddr_in6.
• Remove old mentioning of old pad
• Fixup mentioning of a structure that is just deleted

That sounds like pretty serious compiler bug. What is the compiler and what is the machine arch?

Thanks! While I'm 99% sure it is going to fix my panic, I can't test it right away. Traveling right now and machine that exhibits it is not with me. I'll be able to test next week. If you want you can delay the push until I test. I will report here, in this revision.

The raw and netinet6 versions look correct, but IPv4 is slightly weird. Comments above.

This revision is good one independently of how the API to delete entries is going to look like. To be fair I'm not very happy with new function names, but can't quickly come with a good alternatives.

You may add the patch as is under some non-default ifdef.

This is definitely a useful functionality but the implementation will raise concerns from developers outside of network stack area. We don't want to add string parsing procedures to the kernel.

• 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.