Re: git: 8062f37d1c99 - stable/14 - vtnet: set VNET context in RX handler

  Marko,

On Wed, Apr 17, 2024 at 10:22:52AM +0200, Marko Zec wrote:
M> > https://cgit.FreeBSD.org/src/commit/?id=8062f37d1c99ded250b36ad90fb32bb5f77ee600
M> ...
M> >     vtnet: set VNET context in RX handler
M> 
M> Two questions:
M> 
M> 1) why is vnet not resolved directly via ifp->if_vnet, i.e. why do we
M> need to go through if_getvnet() here?  Not only is if_getvnet()
M> unnecessarily making a function call to return a value of a struct
M> member already at hand, but is doing this on a hot path.  if_getvnet()
M> arrived in 0d2684e15e415cef652e0f75e88962a674bffe04 / D38202 as an
M> "accessor" function with a declared intent to help a vendor maintaining
M> their private out-of-tree drivers independent of changes in struct ifnet
M> layout.  While this indeed may make sense for certain vendor-specific
M> use cases, why do we need to take a penalty of calling if_getvnet() in
M> internal code, here and elsewhere?  if_getvnet() use is starting to get
M> widespread, and it would be nice if we could hear the reasoning behind
M> this school of thought, and perhaps document when the call should be
M> used, and when prefereably not?

The intent to have struct ifnet opaque to drivers is already over 10 years old.
Initially I had quite a radical branch in subversion, which was not finished.
Later Juniper came with less radical changes, where they just hide every field
under accessor function and we agreed it is all right as an intermediate step
(which potentally can last very long).  For vendor maintained drivers it is a
clear win, for in house drivers not so clear.  If the KPI fully hides ifnet,
then resturcturings in sys/net can happen without touching a single driver in
the tree.  What you suggest is allowing two KPIs to exist in parallel
officially - one for in house drivers and the other for vendor maintained.  And
allow that  for an immesurable performance gain.  I'd argue it is totally
immesurable as we are talking about VirtIO driver - a driver that runs in a
virtual machine, where no one can make a reproducible benchmark that would be
able to detect such a micro optimization. I'd also argue that if somebody cares
about performance at a level of optimizing out function calls, they'd probably
be running without VIMAGE already and unlikely running in the virtual
environment, as well.

M> 2) why is CURVNET_SET_QUIET() used instead of CURVNET_SET()?  Which VNET
M> recursion are you trying to quiesce with the _QUIET() version?

That's a good point, thanks! I will correct this.

P.S. I'd really appreciate commits to main being reviewed before merge to
stable/14.

-- 
Gleb Smirnoff