introduce new 'ice' driver for Intel E800 Ethernet controllers

[erj]

Summary

The ice driver controls the Intel E800 Ethernet controllers. It is based
on the iflib framework. Initially, support for basic Tx/Rx functionality
is included.

Future work will enable virtual functions using the virtchnl interface and
the iavf virtual function driver.

[gallatin]

I just looked for a short time.. more comments later..

sys/dev/ice/ice_common_txrx.h

94

Can you remove or properly comment these lines (eg, // is frowned upon). When I sent you this for ixl, I never intended those lines be kept.

Also, very sad/disappointed that ice has this horrible restriction that makes iflib's transmit routine more complex.

176

Cool. Can you talk about what can be tunneled?

196

Why a counter and not just a uint64_t?

Note that access to the tx/rx rings should be serialized at the point your touching counters. Counters burn 16 bytes per element per cpu, due to how UMA works.

336

I hate this.

Maybe I'm just old fashioned, but I'd check for the 2 most common cases first (good checksum, or no checksum at all), first and likely not even increment counters for them. Eg, find all the status0 bits that would be set for a good packet, if they are all set, set the flags in one go and the data to 0xffff and be done. No counters.

Then I'd decode all the fiddly stuff for the error cases and increment counters there.

[jacob.e.keller_intel.com]

In D21959#479856, @gallatin wrote:

I just looked for a short time.. more comments later..

Replied to some comments. Hopefully Eric can give more feedback on the counters thing. I dont' remember if we had a problem with uint64 or not.

sys/dev/ice/ice_common_txrx.h

94

Yea, I had forgotten these were still here.

I really don't like this either, but there's unfortunately nothing we can do about it now :(

196

Eric, can you recall why these counters are used instead of just using a u64? I don't remember offhand.

336

Makes some sense.

[erj]

sys/dev/ice/ice_common_txrx.h

176

I'm not entirely sure, but the description for this field in the datasheet is almost exactly the same as it is in Fortville. The only difference in the field definition is that it looks like you can make Columbiaville calculate the tunneling UDP checksum, too.

196

From what I can remember, it was motivated by how we were storing these per-queue stats in the VSI struct instead of storing them in each queue struct. We're incrementing the stats in the hot path, so we didn't want to take some sort of per-VSI lock to protect the counters while they're being incremented. The counter(9) functionality looked like it would get around that requirement.

We could've stored each counter in each queue, but I think we were concerned about how much memory that could use and how it could be annoying to see what's incremented across many queues. Though, thinking about the per CPU memory usage, it might not be that bad to keep them in a per-queue struct. We also did have an implementation that actually periodically collected the stats from each queue (in the update_admin_status() function I think) and it added each queue counter to the VSI (so that we could just stick with uint64_t's), but I think that ran into some other problem. It might have been that we would've had to take a lock on each Tx/Rx ring to get the counter values, and that didn't seem like a good idea.

All that said, these were intended to help debug problems and validate that the hardware checksums were working; if it seems like there might be more harm than good or if they need to be reimplemented, it might be easier to just leave it out or have it compiled out upstream.

[erj]

sys/dev/ice/ice_common_txrx.h

336

It does seem like a good change to make, but I wonder if it'll have a positive performance impact.

[gallatin]

sys/dev/ice/ice_common_txrx.h

196

I really believe the counters should be stored per-ring, and summed without a lock.

[rpokala]

Don't forget a manpage. :-)

[jacob.e.keller_intel.com]

In D21959#480017, @rpokala wrote:

Don't forget a manpage. :-)

Good point, thanks! I think we have some stuff already prepared but not included yet. I'll look into that.

[jacob.e.keller_intel.com]

Update ice driver review to address feedback

The review now includes updated code, including changes to address the
review comments from Drew regarding the checksum calculations and statistics.

[jacob.e.keller_intel.com]

Update ice driver code

[erj]

• ice: Add missing includes to ice_iflib.h
• ice: Correct filename used in Makefile
• ice: Add files.amd64 entries for ice
• ice: Add "device ice" as a kernel config option; add it to GENERIC
• ice: Add ice module to sys/modules Makefile
• ice: Remove SR-IOV files/option from Makefile
• ice: Update ice driver code to a current version.

[erj]

• Remove extraneous line breaks from files.amd64
• ice: Fix spelling mistake caught by arcanist

[erj]

• Add ddp to kernel and build process
• ice: Use fancier ice module Makefile from OOT component
• Move ice files.amd64 entries to appropriate place
• ice: Update description in NOTES

[erj]

I'll add @adrian to see if I'm using the firmware(9) functionality correctly enough; it's similar to how it's used in the Wi-Fi drivers I think. Add other reviewers if you think it might be helpful.

[erj]

• ice-ddp: Use "SRCTOP" instead of "S" in module Makefile

[kbowling]

Can we enable this in an unsupported state for other 64-bit archs (arm64. ppc64)? Just would like to see the files entry correct for such usage.

[erj]

In D21959#546203, @kbowling wrote:

Can we enable this in an unsupported state for other 64-bit archs (arm64. ppc64)? Just would like to see the files entry correct for such usage.

Yeah, that's doable.

[erj]

In D21959#546203, @kbowling wrote:

Can we enable this in an unsupported state for other 64-bit archs (arm64. ppc64)? Just would like to see the files entry correct for such usage.

Making it compile on arm64 was straightforward, but powerpc64 is telling me that "atomic_testandset_32" and "atomic_testandclear_32" don't exist. Are there equivalent atomic instructions available?

[kbowling]

Having one other arch, arm64, compile seems like a reasonable litmus for the driver. I can look into the PPC stuff when AIC HW begins to appear on the market and it shouldn't block this.

[erj]

• ice: Add support for building on arm64 targets

[erj]

• ice: Add entries to arm64 NOTES file

[emaste]

sys/contrib/dev/ice/README

11 ↗

(On Diff #71804)

not sure what these special characters are (that Phab rendered as special colour !)

[erj]

• ice: Update MAINTAINERS file
• ice: Remove strange characters in ice_ddp README

[erj]

sys/contrib/dev/ice/README

11 ↗

(On Diff #71804)

I don't know what those are, but I removed them.

[jeffrey.e.pieper_intel.com]

We'll need to patch this to bring it in line with OOT, but is g2g for now.

[erj]

Any other comments before I commit this?

[erj]

This was committed in rS361541.