Page MenuHomeFreeBSD

Try to fix vmxnet3 when RSS option is enabled

Authored by avg on Jan 12 2020, 7:49 PM.
Referenced Files
Unknown Object (File)
Tue, Feb 7, 9:09 PM
Unknown Object (File)
Dec 16 2022, 2:18 PM
Unknown Object (File)
Dec 12 2022, 12:17 PM
Unknown Object (File)
Nov 28 2022, 1:46 PM



We observe at least one problem: if a UDP socket is connect-ed, then a
received packet that matches the connection cannot be matched to the
corresponding PCB because of an incorrect flow ID. That was oberved for DNS
requests from the libc resolver. We got this problem because FreeBSD
r343291 enabled code that can set rsstype of received packets to values
other than M_HASHTYPE_OPAQUE_HASH. Earlier that code was under 'ifdef
notyet'. The commit message did not provide any rationale.

The essence of this change is to use the system-wide RSS key instead of some
historic hardcoded key.

Sponsored by Panzura.

Diff Detail

rS FreeBSD src repository - subversion
Lint Not Applicable
Tests Not Applicable

Event Timeline

I have updated bug 242890 to include the rationale for enabling (and also modifying) the #ifdef notyet RSS code when converting the driver to iflib, and also raised the question of whether this same issue exists for the bnxt driver.

1164–1167 ↗(On Diff #66657)

I think the correct way to do this is to modify vmxnet3_attach_post() such that when RSS is defined, if rss_gethashalgo() is not TOEPLITZ, the VMXNET3_FLAGS_RSS flag will not get set in sc->vmx_flags. When that flag is not set, vmxnet3_reinit_rss_shared_data() will never be called, and thus can be written assuming that rss->hash_func should always be set to TOEPLITZ.

The way it is written now, we have to wonder whether the vmxnet3 virtual device implementation will tolerate RSS enabled with hash_func set to NONE. We have no documentation for this virtual device implementation and no reference code that tries this mode. We do however know (via the successful operation of both the FreeBSD and Linux drivers) that the virtual device works when RSS is disabled in the virtual device requested features, and also when it is enabled in the requested features with the hash_func sent to TOEPLITZ.

1164–1167 ↗(On Diff #66657)

Another idea that I've just got is to regress to using M_HASHTYPE_OPAQUE_HASH for all received packets when rss_gethashalgo() is not TOEPLITZ while still configuring the "hardware" to use TOEPLITZ. With that, we still get the benefit of distributing incoming packets between queues / CPUs, but the software RSS will not expect that hardware hashes are the same as software hashes.

1164–1167 ↗(On Diff #66657)

My understanding is that with that approach, the benefit would be that the work of the hashes in software would then be distributed across multiple cores (it would be done on the core handling the given receive queue prior to dispatching the packet to the core indicated by the computed hash). If that's the case, seems reasonable to me.

Always configure the virtual NIC to use Toeplitz algorithm.

If the software RSS is enabled and it is configured with the same algorithm,
then use the same hash key and report flow hash types used by the hardware.
In this configuration, the software and the "hardware" should be in
agreement about hash types and values.

If the software RSS is not enabled or if it uses a different algorithm, then
use the internal hash key and report the flow hash type as opaque regardless
of the hardware determined type. The hardware and the software would not be
in agreement about hash values anyway. And if the software RSS is not
enabled then there is nothing at all to agree about.

1179 ↗(On Diff #66744)

I think you mean for this to be #endif, and for the #endif below to go away.

avg marked an inline comment as done.Jan 15 2020, 7:23 AM
avg added inline comments.
1179 ↗(On Diff #66744)


Looks good to me.

1538–1560 ↗(On Diff #66772)

This could be placed under #ifdef RSS to leave the code out as VMXNET_FLAG_SOFT_RSS can't be set when RSS is not defined. Probably does not make much of a practical difference given this driver is not run on very constrained platforms. I'll leave that up to you as to whether to do it.

This revision is now accepted and ready to land.Jan 15 2020, 3:36 PM
This revision was automatically updated to reflect the committed changes.
avg marked an inline comment as done.