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.

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.

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.

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

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

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

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