Add support for Amazon Elastic Network Adapter (ENA) NIC
ClosedPublic
Actions

Authored by mk_semihalf.com on Apr 19 2017, 11:35 AM.

Details

Reviewers

cperciva
zbb
adrian
mw_semihalf.com

Group Reviewers

manpages

Commits

rS318647: Add support for Amazon Elastic Network Adapter (ENA) NIC

Summary

ENA is a networking interface designed to make good use of modern CPU
features and system architectures.

The ENA device exposes a lightweight management interface with a
minimal set of memory mapped registers and extendable command set
through an Admin Queue.

The driver supports a range of ENA devices, is link-speed independent
(i.e., the same driver is used for 10GbE, 25GbE, 40GbE, etc.), and has
a negotiated and extendable feature set.

Some ENA devices support SR-IOV. This driver is used for both the
SR-IOV Physical Function (PF) and Virtual Function (VF) devices.

ENA devices enable high speed and low overhead network traffic
processing by providing multiple Tx/Rx queue pairs (the maximum number
is advertised by the device via the Admin Queue), a dedicated MSI-X
interrupt vector per Tx/Rx queue pair, and CPU cacheline optimized
data placement.

The ENA driver supports industry standard TCP/IP offload features such
as checksum offload and TCP transmit segmentation offload (TSO).
Receive-side scaling (RSS) is supported for multi-core scaling.

The ENA driver and its corresponding devices implement health
monitoring mechanisms such as watchdog, enabling the device and driver
to recover in a manner transparent to the application, as well as
debug logs.

Some of the ENA devices support a working mode called Low-latency
Queue (LLQ), which saves several more microseconds. This feature will
be implemented for driver in future releases.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mw_semihalf.com created this revision.Apr 19 2017, 11:35 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 19 2017, 11:35 AM

A few cursory comments from a brief look.

This will need a man page
Should be hooked up in sys/conf/files (or files.amd64) so that it can be compiled into the kernel as well.

sys/dev/ena/ena.h
7–19 ↗	(On Diff #27547)	Could you check if it's acceptable to Amazon to number these as is conventionally done with BSD licenses, so that it fits a template already included in automated licence search tools? e.g. * 1. Redistributions of source code must retain the above copyright * notice, this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright * notice, this list of conditions and the following disclaimer in the * documentation and/or other materials provided with the distribution. * 3. Neither the name of the copyright holder nor the names of its contributors * may be used to endorse or promote products derived from this software * without specific prior written permission. It would be convenient also if they are willing to omit clause 3, in which case it becomes the "Simplified BSD license" or "FreeBSD license".
75–77 ↗	(On Diff #27547)	Can you rephrase this comment, it's somewhat difficult to understand. As I understand it a smaller buffer would just require too many buffers per packet with a higher MTU. I don't follow the "mtu will be changed" part.
sys/dev/ena/ena_com/ena_admin_defs.h
38–50 ↗	(On Diff #27547)	Double spacing seems a bit odd. I guess it's because other enums or structs have per-entry comments?
sys/modules/ena/Makefile
40 ↗	(On Diff #27547)	Forcing `-O2` seems odd here. Is there a reason you can't use regular build infrastructure to have `-O2` set when desired by the user?

A few comments on the user-facing sysctl description strings

sys/dev/ena/ena_sysctl.c
92 ↗	(On Diff #27547)	Maybe "watchdog expiry count" "expire" as a noun sounds a bit odd, and there's a straightforward way to rephrase. Perhaps same below, "interface up count", "interface down count" ? This is a bit subjective, for example "wakeups" below does not seem objectionable to me. Maybe it's just that "wakeups" has already been used extensively.
122 ↗	(On Diff #27547)	"buf" is more conventional short form if it's for buffer, but it probably doesn't need to be shortened at all. Perhaps: "TX buffer preparation failures" ? "TX buffer preparation fail count" ?
128 ↗	(On Diff #27547)	"DMA mapping failures"?
132 ↗	(On Diff #27547)	It looks like this happens when there are too many descriptors? It's better to provide a specific reason than just "wrong". Maybe something like "Excessive descriptor failures" or "Excessive descriptor packet discards"?
138 ↗	(On Diff #27547)	"TX polling" sounds like an enable knob. If it's a polling count, "TX poll count".
147 ↗	(On Diff #27547)	"Bad request id count"?
161 ↗	(On Diff #27547)	received
164 ↗	(On Diff #27547)	received
182 ↗	(On Diff #27547)	"Bad descriptor count"?
185 ↗	(On Diff #27547)	"Small copy packet count"?
202 ↗	(On Diff #27547)	If other descriptions are spelled out in full ought to do so here: "Receive packet drops"

Further to my comment above, I notice an earlier version of the driver posted to LKML has a dual GPL / 2-clause BSD license (https://lkml.org/lkml/2016/3/15/196) so I'm hopeful the standard 2-clause FreeBSD license can be used here.

cperciva added inline comments.Apr 20 2017, 1:12 AM

sys/dev/ena/ena.h
35 ↗	(On Diff #27547)	Looks like a whitespace typo?
49 ↗	(On Diff #27547)	These are `__STRING` and `__XSTRING`, defined in <sys/cdefs.h>
104 ↗	(On Diff #27547)	Is `_TRESHOLD` a typo for `_THRESHOLD`?

I think this needs a change to sys/modules/Makefile as well in order to connect it to the build?

mk_semihalf.com added inline comments.Apr 28 2017, 10:06 AM

sys/dev/ena/ena.h
7–19 ↗	(On Diff #27547)	We are still discussing this and need response from legal team. We will upload it with next patchset (3rd).
35 ↗	(On Diff #27547)	Ok
49 ↗	(On Diff #27547)	Ok
75–77 ↗	(On Diff #27547)	This value is no longer used. We decided to remove it.
104 ↗	(On Diff #27547)	Ok
sys/dev/ena/ena_com/ena_admin_defs.h
38–50 ↗	(On Diff #27547)	ena_com is HAL delivered to us by Amazon. We decided to move it to sys/contrib/ena-com
sys/dev/ena/ena_sysctl.c
92 ↗	(On Diff #27547)	Ok
122 ↗	(On Diff #27547)	Ok
128 ↗	(On Diff #27547)	Ok
132 ↗	(On Diff #27547)	Ok
138 ↗	(On Diff #27547)	Ok
147 ↗	(On Diff #27547)	Ok
161 ↗	(On Diff #27547)	Ok
164 ↗	(On Diff #27547)	Ok
182 ↗	(On Diff #27547)	Ok
185 ↗	(On Diff #27547)	Ok
202 ↗	(On Diff #27547)	Ok
sys/modules/ena/Makefile
40 ↗	(On Diff #27547)	Ok, all unnecessary flags will be deleted

mk_semihalf.com commandeered this revision.Apr 28 2017, 11:16 AM

mk_semihalf.com added a reviewer: mw_semihalf.com.

Add new patchset. Manual and legal issues will be added in patchset 3.

Changelog patchset1 <-> patchset2:

* Move ena_com (HAL) to sys/contrib/ena-com
  https://svnweb.freebsd.org/changeset/base/317516
  https://svnweb.freebsd.org/changeset/base/317518

* Update include path when including ena-com in driver

* Update Makefile PATH for ena_com and use SRCTOP instead of CURDIR

* Add ENA entries for sys/conf/files

* Remove CFLAGS from Makefile

* Fix panic when booting kernel with ENA module because driver was using
  timers when they are not yet available. To fix this RSS initialization
  was deferred.

* Add rss_support mode to handle failure of deferred RSS initialization.

* Fix compilation error if driver is compiled with flag -DENA_TRACE and
  -DENA_INFO. ena_trace macro was called with extra argument.

* Fix descriptions for sysctls

* Remove ENA_DEFAULT_MIN_RX_BUFF_ALLOC_SIZE which was no longer used

* Minor style fixes (header guardian typo)

* Use __XSTRING instead of stringify macro

* Fix typo in DB_THRESHOLD variable

* Fix memory leaks on detach:
  - release RSS resources
  - destroy mmio_reg_read_request

Add manual entry for the driver

Change license to BSD 2-clause

Add missing changes that were not uploaded with the driver
- allocate part of software queues resources during interface attachment
- there were missing error paths and drbr flush

Herald added a reviewer: manpages. · View Herald TranscriptMay 15 2017, 1:57 PM

Closed by commit rS318647: Add support for Amazon Elastic Network Adapter (ENA) NIC (authored by zbb). · Explain WhyMay 22 2017, 2:46 PM

This revision was automatically updated to reflect the committed changes.

wblock added a subscriber: wblock.Jul 7 2017, 6:53 PM

wblock added inline comments.

head/share/man/man4/ena.4
105	Please install textproc/igor and run `igor -R ena.4 \| less -RS` on this file to find this and other misspellings and problems. `igor -Ry ena.4 \| less -RS` will also show style suggestions. Please tell me which man page was used as a template for this.
108	Split the sentence instead of splicing with a semicolon.
112	s/low/old/
115	s/transcation/transaction/ Please start new sentences on new lines.
119	Split the sentence instead of splicing with a semicolon.
178	Split the sentence instead of splicing with a semicolon. Also, add articles and use active voice: Error occurred during initialization of one of RSS resources. The device will work with reduced performance because all RX packets will be
235	Use a separate sentence rather than splicing with a semicolon: IOCTL request for the device to work in promiscuous/allmulti mode. See
243	s/adapter/adapter,/
244	please email specific information related to the issue to

mk_semihalf.com added inline comments.Aug 14 2017, 11:25 AM

head/share/man/man4/ena.4
105	Thank you for your suggestions. We will upload updated manual together with bunch of patches that will be probably uploaded within few weeks. I will add you as a reviewer for the upcoming manual update. I was using ixgbe.4 and mlx.4 manuals as a template.