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> References: <CADwhF6VuoPCNEqyBmt%2BdZgDwHdaGty2%2BsYU4eYg0_62CMHq-BA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--=_3054962f98fc689e6f81a2c8ac68acda
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
format=flowed
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
--=_3054962f98fc689e6f81a2c8ac68acda
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html; charset=UTF-8
<html><head><meta http-equiv=3D"Content-Type" content=3D"text/html; charset=
=3DUTF-8" /></head><body style=3D'font-size: 10pt; font-family: Verdana,Gen=
eva,sans-serif'>
<p id=3D"reply-intro">On 2022-12-13 09:18, Andrew Gallatin wrote:</p>
<blockquote type=3D"cite" style=3D"padding: 0 0.4em; border-left: #1010ff 2=
px solid; margin: 0">
<div id=3D"replybody1">
<div dir=3D"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() c=
heck in in_pcblookup_hash_locked() can cause a cache miss just by <span cla=
ss=3D"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?&=
nbsp; It originated in this commit:</div>
<div> </div>
<div>
<pre class=3D"v1gmail-c-mrkdwn__pre">commit 413628a7e3d23a897cd959638d32539=
5e4c9691b
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.
=20
This enhances the current jail implementation to permit multiple
addresses per jail. In <span class=3D"v1gmail-LI v1gmail-ng">addtion</s=
pan> 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 j=
ails). In either case, a perfect 4-tuple match should be enough to un=
iquely 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 sa=
me 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=
=2E</div>
</div>
</div>
</blockquote>
<div id=3D"replybody1">
<div dir=3D"ltr">
<div> </div>
<div>For a complete 4-tuple, it should indeed be the case that a match woul=
d only ever identify a single prison. The later part of the function =
that examines wildcards definitely needs the check. I don't get the X=
XX comment about both being bound with SO_REUSEPORT, because I would only e=
xpect that to apply to listening, not to full connections. But I also expec=
t Bjoern to know more than I do here...</div>
<div> </div>
<div>- Jamie</div>
</div>
</div>
</body></html>
--=_3054962f98fc689e6f81a2c8ac68acda--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?e5ef5a4dfae8f7723c10dfb8db9b7d9a>