Date: Tue, 20 Jul 2021 10:52:38 +0200 From: Marko Zec <zec@fer.hr> To: Daniel O'Connor via freebsd-hackers <freebsd-hackers@freebsd.org> Cc: darius@dons.net.au, Michael Tuexen <tuexen@freebsd.org> Subject: Re: git: a730d82378d3 - main - tcp: fix RACK and BBR when using VIMAGE enabled kernel Message-ID: <20210720105238.1341472c@x23> In-Reply-To: <CE6960A4-B1C0-49DE-BF69-57EDF55C8696@dons.net.au> References: <202107192233.16JMX0k4044018@gitrepo.freebsd.org> <CE6960A4-B1C0-49DE-BF69-57EDF55C8696@dons.net.au>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 20 Jul 2021 10:29:30 +0930 Daniel O'Connor via freebsd-hackers <freebsd-hackers@freebsd.org> wrote: > > On 20 Jul 2021, at 08:03, Michael Tuexen <tuexen@freebsd.org> wrote: > > > > The branch main has been updated by tuexen: > > > > URL: > > https://cgit.FreeBSD.org/src/commit/?id=a730d82378d3cdf5356775ec0c23ad2ca40c5edb > > > > diff --git a/sys/netinet/tcp_stacks/rack_bbr_common.c > > b/sys/netinet/tcp_stacks/rack_bbr_common.c index > > baa267b43752..bf93359368f9 100644 --- > > a/sys/netinet/tcp_stacks/rack_bbr_common.c +++ > > b/sys/netinet/tcp_stacks/rack_bbr_common.c @@ -508,16 +508,18 @@ > > skip_vnet: m_freem(m); > > m = m_save; > > } > > - if (no_vn == 0) > > + if (no_vn == 0) { > > CURVNET_RESTORE(); > > + } > > INP_UNLOCK_ASSERT(inp); > > return(retval); > > } > > skipped_pkt: > > m = m_save; > > } > > - if (no_vn == 0) > > + if (no_vn == 0) { > > CURVNET_RESTORE(); > > + } > > return(retval); > > } > > 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 {}? 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. 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... > 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. 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). 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 == NULL. 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 == 0) hack, and instead to unconditionally invoke CURVNET_SET(so->so_vnet) on entry to ctf_process_inbound_raw()? Marko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20210720105238.1341472c>