Page MenuHomeFreeBSD

Fix rxcsum issue introduced in r338838
ClosedPublic

Authored by shurd on Nov 7 2018, 5:05 AM.

Details

Summary

r338838 attempted to fix issues with rxcsum and rxcsum6.
However, the rxcsum bits were set as though if_setcapenablebit() was
being called, not if_togglecapenable() which is in use. As a result,
it was not possible to disable rxcsum when rxcsum6 was supported.

See PR233004

Test Plan

Texted with my ix and em NICs. Have lev confirm the fix

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shurd created this revision.Nov 7 2018, 5:05 AM
lev added a comment.Nov 7 2018, 10:24 AM

It fix ix0 for me.

I concern about one thing (theoretical for me, though): iflib is (and will be) used not only by Intel's chips. Is this code right for all consumers of iflib?

lev accepted this revision.Nov 7 2018, 10:24 AM
This revision is now accepted and ready to land.Nov 7 2018, 10:24 AM
shurd added a comment.Nov 7 2018, 7:21 PM
In D17881#382102, @lev wrote:

It fix ix0 for me.

Great... I'll commit it then.

I concern about one thing (theoretical for me, though): iflib is (and will be) used not only by Intel's chips. Is this code right for all consumers of iflib?

Yes, while NICs won't be able to separately support RX checksum offload for IPv4 and IPv6, this seems to be true for almost all cards. In the future, we may add a capability flag to control this, but it's not needed yet.

This revision was automatically updated to reflect the committed changes.

@shurd - Turns out you've been faster. We've been working one this bug too. Our plan was to do it this way:

			if ((setmask & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)) !=
			    (oldmask & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)))
				setmask |= IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6;

But it's too late for me today to figure out which has more sense ;)

@shurd - Turns out you've been faster. We've been working one this bug too. Our plan was to do it this way:

			if ((setmask & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)) !=
			    (oldmask & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)))
				setmask |= IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6;

But it's too late for me today to figure out which has more sense ;)

I think the patch here in this review makes more obvious sense to me based on the problem description.

head/sys/net/iflib.c
4260

This removes the comment about the assumption that they are all enabled.

Can we add back a comment something like "This assumes that the supported flags are always set to the same state"?

I guess maybe it's not a big deal since we control the toggle code, and thus they should never get in this inconsistent state...

erj added a comment.Nov 7 2018, 10:16 PM

@krzysztof.galazka_intel.com I think @shurd's version here is better in that it masks isc_capabilities before togging RXCSUM/RXCSUM_IPV6. Though I think if you had added that, they might be roughly similar.