Page MenuHomeFreeBSD

introduce new 'ice' driver for Intel E800 Ethernet controllers
Needs ReviewPublic

Authored by on Oct 9 2019, 7:06 PM.


Group Reviewers
Intel Networking

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.

Diff Detail

Lint OK
No Unit Test Coverage
Build Status
Buildable 28467
Build 26531: arc lint + arc unit

Event Timeline

erj added a reviewer: iflib.Oct 9 2019, 7:21 PM

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


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.


Cool. Can you talk about what can be tunneled?


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.


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.

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.


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 :(


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


Makes some sense.

erj added a subscriber: erj.Oct 10 2019, 1:25 AM
erj added inline comments.

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.


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 added inline comments.Oct 10 2019, 1:27 AM

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

gallatin added inline comments.Oct 10 2019, 12:07 PM

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

Don't forget a manpage. :-)

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.

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.

Update ice driver code