Fix rxcsum issue introduced in r338838
ClosedPublic
Actions

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

Details

Reviewers

lev
erj
sbruno

Group Reviewers

Intel Networking

Commits

rS340365: MFC r340236:
rS340236: Fix rxcsum issue introduced in r338838

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 - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 20673
Build 20088: arc lint + arc unit

Event Timeline

shurd created this revision.Nov 7 2018, 5:05 AM

Herald added subscribers: ae, imp. · View Herald TranscriptNov 7 2018, 5:05 AM

Harbormaster completed remote builds in B20673: Diff 50101.Nov 7 2018, 5:05 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

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.

Closed by commit rS340236: Fix rxcsum issue introduced in r338838 (authored by shurd). · Explain WhyNov 7 2018, 7:31 PM

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 ;)

In D17881#382258, @krzysztof.galazka_intel.com wrote:
@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.

jacob.e.keller_intel.com added inline comments.Nov 7 2018, 9:31 PM

head/sys/net/iflib.c
4260 ↗	(On Diff #50134)	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...

@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.

Revision Contents
Changeset List

Path

Size

sys/

net/

iflib.c

17 lines

Diff 50101

View Options

Fix rxcsum issue introduced in r338838ClosedPublicActions