Date: Tue, 13 Dec 2022 10:00:24 -0800 From: James Gritton <jamie@freebsd.org> To: jail@freebsd.org Cc: bz@freebsd.org, "glebius@FreeBSD.org" <glebius@freebsd.org>, Andrew Gallatin <gallatin@gmail.com> Subject: Re: prison_flag() check in hot path of in_pcblookup() Message-ID: <e5ef5a4dfae8f7723c10dfb8db9b7d9a@freebsd.org> In-Reply-To: <CADwhF6VuoPCNEqyBmt%2BdZgDwHdaGty2%2BsYU4eYg0_62CMHq-BA@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
On 2022-12-13 09:18, Andrew Gallatin wrote:
> I was trying to improve the performance of in_pcblookup(), as it is a
> very hot path for us (Netflix). One thing I noticed was the
> prison_flag() check in in_pcblookup_hash_locked() can cause a cache
> miss just by deref'ing the cred pointer, and it can also cause multiple
> misses in tables with collisions by causing us to walk the entire chain
> even after finding a perfect match.
>
> I'm curious why this check is needed. Can you explain it to me? It
> originated in this commit:
>
> commit 413628a7e3d23a897cd959638d325395e4c9691b
> Author: Bjoern A. Zeeb <bz@FreeBSD.org>
> Date: Sat Nov 29 14:32:14 2008 +0000
>
> MFp4:
> Bring in updated jail support from bz_jail branch.
>
> This enhances the current jail implementation to permit multiple
> addresses per jail. In addtion to IPv4, IPv6 is supported as well.
>
> My thinking is that a jail will either use the host IP, and share its
> port space, or it will have its own IP entirely (but I know nothing
> about jails). In either case, a perfect 4-tuple match should be enough
> to uniquely identify the connection.
>
> Even if this somehow is not the case and we have multiple connections
> somehow sharing the same 4-tuple, how does checking the prison flag
> help us? It would prefer the jailed connection over the non jailed,
> but that would shadow a host connection. And if we had 2 jails sharing
> the same 4-tuple, the first jail would win.
>
> I can't see how this check is doing anything useful, so I'd very much
> like to remove this check if possible. Untested patch attached.
For a complete 4-tuple, it should indeed be the case that a match would
only ever identify a single prison. The later part of the function that
examines wildcards definitely needs the check. I don't get the XXX
comment about both being bound with SO_REUSEPORT, because I would only
expect that to apply to listening, not to full connections. But I also
expect Bjoern to know more than I do here...
- Jamie
[-- Attachment #2 --]
<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /></head><body style='font-size: 10pt; font-family: Verdana,Geneva,sans-serif'>
<p id="reply-intro">On 2022-12-13 09:18, Andrew Gallatin wrote:</p>
<blockquote type="cite" style="padding: 0 0.4em; border-left: #1010ff 2px solid; margin: 0">
<div id="replybody1">
<div dir="ltr">
<div>I was trying to improve the performance of in_pcblookup(), as it is a very hot path for us (Netflix). One thing I noticed was the prison_flag() check in in_pcblookup_hash_locked() can cause a cache miss just by <span class="v1gmail-LI v1gmail-ng">deref'ing</span> the cred pointer, and it can also cause multiple misses in tables with collisions by causing us to walk the entire chain even after finding a perfect match.</div>
<div> </div>
<div>I'm curious why this check is needed. Can you explain it to me? It originated in this commit:</div>
<div> </div>
<div>
<pre class="v1gmail-c-mrkdwn__pre">commit 413628a7e3d23a897cd959638d325395e4c9691b
Author: Bjoern A. Zeeb <bz@FreeBSD.org>
Date: Sat Nov 29 14:32:14 2008 +0000
MFp4:
Bring in updated jail support from bz_jail branch.
This enhances the current jail implementation to permit multiple
addresses per jail. In <span class="v1gmail-LI v1gmail-ng">addtion</span> to IPv4, IPv6 is supported as well.</pre>
</div>
<div> </div>
<div>My thinking is that a jail will either use the host IP, and share its port space, or it will have its own IP entirely (but I know nothing about jails). In either case, a perfect 4-tuple match should be enough to uniquely identify the connection. </div>
<div> </div>
<div>Even if this somehow is not the case and we have multiple connections somehow sharing the same 4-tuple, how does checking the prison flag help us? It would prefer the jailed connection over the non jailed, but that would shadow a host connection. And if we had 2 jails sharing the same 4-tuple, the first jail would win.</div>
<div> </div>
<div>I can't see how this check is doing anything useful, so I'd very much like to remove this check if possible. Untested patch attached.</div>
</div>
</div>
</blockquote>
<div id="replybody1">
<div dir="ltr">
<div> </div>
<div>For a complete 4-tuple, it should indeed be the case that a match would only ever identify a single prison. The later part of the function that examines wildcards definitely needs the check. I don't get the XXX comment about both being bound with SO_REUSEPORT, because I would only expect that to apply to listening, not to full connections. But I also expect Bjoern to know more than I do here...</div>
<div> </div>
<div>- Jamie</div>
</div>
</div>
</body></html>
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?e5ef5a4dfae8f7723c10dfb8db9b7d9a>