Date: Fri, 23 Jul 2021 15:19:45 +0930 From: Daniel O'Connor via freebsd-hackers <freebsd-hackers@freebsd.org> To: Marko Zec <zec@fer.hr> Cc: "Daniel O'Connor via freebsd-hackers" <freebsd-hackers@freebsd.org>, Michael Tuexen <tuexen@freebsd.org> Subject: Re: git: a730d82378d3 - main - tcp: fix RACK and BBR when using VIMAGE enabled kernel Message-ID: <4ED18100-AE72-409E-B5AA-C839B920804F@dons.net.au> In-Reply-To: <20210720105238.1341472c@x23> References: <202107192233.16JMX0k4044018@gitrepo.freebsd.org> <CE6960A4-B1C0-49DE-BF69-57EDF55C8696@dons.net.au> <20210720105238.1341472c@x23>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 20 Jul 2021, at 18:22, Marko Zec <zec@fer.hr> wrote:
>=20
> On Tue, 20 Jul 2021 10:29:30 +0930
> Daniel O'Connor via freebsd-hackers <freebsd-hackers@freebsd.org> =
wrote:
>>=20
>> Not to pick on this particular commit, but does anyone know why
>> CURVNET_RESTORE os not defined such that it doesn't require wrapping
>> with {}?
>=20
> True, the body of CURVNET_RESTORE() macro could be redefined so that =
it
> gets enclosed in a do {...} while (0) block, most probably without any
> ill side-effects.
>=20
> One reason it was never defined in such manner is that CURVNET_ macros
> were never intended to be invoked conditionally, which is also clearly
> documented in VNET(9) . The other reason is my clumsiness...
OK understood. I didn't even think to look for a man page, sorry!
One nit pick about the man page is that 'man 9 VNET' doesn't work, you =
need 'man 9 vnet'.
>> Looking through vnet.h I see that VNET_ASSERT,
>> VNET_GLOBAL_EVENTHANDLER_REGISTER_TAG, and
>> VNET_GLOBAL_EVENTHANDLER_REGISTER have a do { } while(0) wrapper but
>> CURVNET_SET_QUIET, CURVNET_SET_VERBOSE, CURVNET_RESTORE,
>> VNET_SYSINIT, and VNET_SYSUNINIT don't.
>=20
> CURVNET_SET macros cannot be wrapped in a separate block because they
> declare a local variable which must be visible / available to the
> corresponding CURVNET_RESTORE(s).
Ahh right, obviously I didn't read the code very deeply :)
> In this particular case, i.e. inside ctf_process_inbound_raw(), the
> rule that CURVNET_SET() must not be conditionally called seems to be
> circumvented by a local hack, in an attempt to handle a case when
> VNET cannot be inferred from m->m_pkthdr->rcvif->if_vnet, when m =3D=3D
> NULL.
>=20
> Given that a non-NULL struct socket *so seem to be available as an
> argument to ctf_process_inbound_raw(), perhaps it could be possible to
> remove the if (no_vn =3D=3D 0) hack, and instead to unconditionally =
invoke
> CURVNET_SET(so->so_vnet) on entry to ctf_process_inbound_raw()?
Thanks for the detailed explanation, much appreciated.
--
Daniel O'Connor
"The nice thing about standards is that there
are so many of them to choose from."
-- Andrew Tanenbaum
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4ED18100-AE72-409E-B5AA-C839B920804F>