Re: git: a730d82378d3 - main - tcp: fix RACK and BBR when using VIMAGE enabled kernel

From: Daniel O'Connor via freebsd-hackers <freebsd-hackers_at_freebsd.org>
Date: Fri, 23 Jul 2021 15:19:45 +0930
> On 20 Jul 2021, at 18:22, Marko Zec <zec_at_fer.hr> wrote:
> 
> On Tue, 20 Jul 2021 10:29:30 +0930
> Daniel O'Connor via freebsd-hackers <freebsd-hackers_at_freebsd.org> wrote:
>> 
>> 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...

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.
> 
> 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 ==
> 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()?

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
Received on Fri Jul 23 2021 - 05:49:45 UTC

Original text of this message