From: Stephen Hemminger <stephen@networkplumber.org>
To: Jan Medala <jan@semihalf.com>
Cc: dev@dpdk.org, matua@amazon.com
Subject: Re: [PATCH v3 0/4] DPDK polling-mode driver for Amazon Elastic Network Adapters (ENA)
Date: Mon, 22 Feb 2016 13:07:47 -0800 [thread overview]
Message-ID: <20160222130747.4843fc2f@xeon-e3> (raw)
In-Reply-To: <1456169211-18867-1-git-send-email-jan@semihalf.com>
On Mon, 22 Feb 2016 20:26:47 +0100
Jan Medala <jan@semihalf.com> wrote:
> This drop includes additional features for Amazon ENA:
> * Low Latenycy Queue (LLQ) for Tx
> * RSS
>
> All previous comments are resolved.
>
> Jan Medala (4):
> ena: Amazon ENA documentation
> ena: Amazon ENA communication layer
> ena: Amazon ENA communication layer for DPDK platform
> ena: DPDK polling-mode driver for Amazon Elastic Network Adapters
> (ENA)
>
> config/common_linuxapp | 11 +
> doc/guides/nics/ena.rst | 238 ++
> drivers/net/Makefile | 1 +
> drivers/net/ena/Makefile | 62 +
> drivers/net/ena/base/ena_com.c | 2750 ++++++++++++++++++++
> drivers/net/ena/base/ena_com.h | 1038 ++++++++
> drivers/net/ena/base/ena_defs/ena_admin_defs.h | 1714 ++++++++++++
> .../net/ena/base/ena_defs/ena_admin_defs_custom.h | 40 +
> drivers/net/ena/base/ena_defs/ena_common_defs.h | 54 +
> drivers/net/ena/base/ena_defs/ena_eth_io_defs.h | 1143 ++++++++
> drivers/net/ena/base/ena_defs/ena_gen_info.h | 35 +
> drivers/net/ena/base/ena_defs/ena_includes.h | 39 +
> drivers/net/ena/base/ena_defs/ena_regs_defs.h | 326 +++
> drivers/net/ena/base/ena_eth_com.c | 506 ++++
> drivers/net/ena/base/ena_eth_com.h | 154 ++
> drivers/net/ena/base/ena_plat.h | 51 +
> drivers/net/ena/base/ena_plat_dpdk.h | 212 ++
> drivers/net/ena/ena_ethdev.c | 1327 ++++++++++
> drivers/net/ena/ena_ethdev.h | 155 ++
> drivers/net/ena/ena_logs.h | 76 +
> drivers/net/ena/ena_platform.h | 58 +
> mk/rte.app.mk | 1 +
> 22 files changed, 9991 insertions(+)
> create mode 100644 doc/guides/nics/ena.rst
> create mode 100755 drivers/net/ena/Makefile
> create mode 100644 drivers/net/ena/base/ena_com.c
> create mode 100644 drivers/net/ena/base/ena_com.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_admin_defs.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_admin_defs_custom.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_common_defs.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_eth_io_defs.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_gen_info.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_includes.h
> create mode 100644 drivers/net/ena/base/ena_defs/ena_regs_defs.h
> create mode 100644 drivers/net/ena/base/ena_eth_com.c
> create mode 100644 drivers/net/ena/base/ena_eth_com.h
> create mode 100644 drivers/net/ena/base/ena_plat.h
> create mode 100644 drivers/net/ena/base/ena_plat_dpdk.h
> create mode 100644 drivers/net/ena/ena_ethdev.c
> create mode 100755 drivers/net/ena/ena_ethdev.h
> create mode 100644 drivers/net/ena/ena_logs.h
> create mode 100644 drivers/net/ena/ena_platform.h
>
I still see lots of style issues reported by running checkpatch (from Linux kernel)
with some of the standard DPDKism's suppressed.
CHECK: spaces preferred around that '/' (ctx:VxV)
#54: FILE: drivers/net/ena/ena_ethdev.c:54:
+#define ENA_IO_RXQ_IDX_REV(q) ((q - 1)/2) /*reverse version of ENA_IO_RXQ_IDX*/
^
CHECK: 'desciptors' may be misspelled - perhaps 'descriptors'?
#59: FILE: drivers/net/ena/ena_ethdev.c:59:
+/* While processing submitted and completed desciptors (rx and tx path
ERROR: space prohibited before that close parenthesis ')'
#67: FILE: drivers/net/ena/ena_ethdev.c:67:
+#define __MERGE_64B_H_L(h, l) (((long)h << 32) | l )
WARNING: space prohibited before semicolon
#115: FILE: drivers/net/ena/ena_ethdev.c:115:
+static int ena_queue_restart(struct ena_ring *ring) ;
WARNING: braces {} are not necessary for any arm of this statement
#149: FILE: drivers/net/ena/ena_ethdev.c:149:
+ if (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP) {
[...]
+ } else if (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP) {
[...]
WARNING: braces {} are not necessary for any arm of this statement
#155: FILE: drivers/net/ena/ena_ethdev.c:155:
+ if (ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV4) {
[...]
+ } else if (ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV6) {
[...]
WARNING: void function return statements are not generally useful
#169: FILE: drivers/net/ena/ena_ethdev.c:169:
+ return;
+}
CHECK: Alignment should match open parenthesis
#177: FILE: drivers/net/ena/ena_ethdev.c:177:
+ if (mbuf->ol_flags & (PKT_TX_L4_MASK || PKT_TX_IP_CKSUM ||
+ PKT_TX_TCP_SEG)) {
CHECK: Blank lines aren't necessary after an open brace '{'
#178: FILE: drivers/net/ena/ena_ethdev.c:178:
+ PKT_TX_TCP_SEG)) {
+
WARNING: line over 100 characters
#184: FILE: drivers/net/ena/ena_ethdev.c:184:
+ mbuf->l3_len + mbuf->l2_len)->data_off) >> 4);
CHECK: Alignment should match open parenthesis
#198: FILE: drivers/net/ena/ena_ethdev.c:198:
+ if (mbuf->packet_type & (RTE_PTYPE_L4_NONFRAG ||
+ RTE_PTYPE_INNER_L4_NONFRAG))
WARNING: void function return statements are not generally useful
#231: FILE: drivers/net/ena/ena_ethdev.c:231:
+ return;
+}
ERROR: "(foo*)" should be "(foo *)"
#235: FILE: drivers/net/ena/ena_ethdev.c:235:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
WARNING: void function return statements are not generally useful
#243: FILE: drivers/net/ena/ena_ethdev.c:243:
+ return;
+}
ERROR: "(foo*)" should be "(foo *)"
#249: FILE: drivers/net/ena/ena_ethdev.c:249:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
WARNING: braces {} are not necessary for single statement blocks
#256: FILE: drivers/net/ena/ena_ethdev.c:256:
+ if ((reta_size == 0) || (reta_conf == NULL)) {
+ return -EINVAL;
+ }
WARNING: line over 100 characters
#261: FILE: drivers/net/ena/ena_ethdev.c:261:
+ RTE_LOG(WARNING, PMD, "indirection table supplied (%d) is bigger than supported (%d)\n",
CHECK: Alignment should match open parenthesis
#262: FILE: drivers/net/ena/ena_ethdev.c:262:
+ RTE_LOG(WARNING, PMD, "indirection table supplied (%d) is bigger than supported (%d)\n",
+ reta_size, ENA_RX_RSS_TABLE_SIZE);
ERROR: "(foo*)" should be "(foo *)"
#300: FILE: drivers/net/ena/ena_ethdev.c:300:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
CHECK: Alignment should match open parenthesis
#309: FILE: drivers/net/ena/ena_ethdev.c:309:
+ if (reta_size == 0 || reta_conf == NULL ||
+ (reta_size > RTE_RETA_GROUP_SIZE && ((reta_conf + 1) == NULL))) {
WARNING: braces {} are not necessary for single statement blocks
#323: FILE: drivers/net/ena/ena_ethdev.c:323:
+ if (TEST_BIT(reta_conf[reta_conf_idx].mask, reta_idx)) {
+ reta_conf[reta_conf_idx].reta[reta_idx] = ENA_IO_RXQ_IDX_REV(indirect_table[i]);
+ }
WARNING: line over 100 characters
#324: FILE: drivers/net/ena/ena_ethdev.c:324:
+ reta_conf[reta_conf_idx].reta[reta_idx] = ENA_IO_RXQ_IDX_REV(indirect_table[i]);
CHECK: Alignment should match open parenthesis
#347: FILE: drivers/net/ena/ena_ethdev.c:347:
+ rc = ena_com_indirect_table_fill_entry(ena_dev, i,
+ ENA_IO_RXQ_IDX(val));
ERROR: "(foo**)" should be "(foo **)"
#386: FILE: drivers/net/ena/ena_ethdev.c:386:
+ struct ena_ring **queues = (struct ena_ring**)dev->data->rx_queues;
WARNING: void function return statements are not generally useful
#394: FILE: drivers/net/ena/ena_ethdev.c:394:
+ return;
+}
ERROR: "(foo**)" should be "(foo **)"
#398: FILE: drivers/net/ena/ena_ethdev.c:398:
+ struct ena_ring **queues = (struct ena_ring**)dev->data->tx_queues;
WARNING: void function return statements are not generally useful
#406: FILE: drivers/net/ena/ena_ethdev.c:406:
+ return;
+}
ERROR: "(foo*)" should be "(foo *)"
#410: FILE: drivers/net/ena/ena_ethdev.c:410:
+ struct ena_ring *ring = (struct ena_ring*)queue;
WARNING: quoted string split across lines
#415: FILE: drivers/net/ena/ena_ethdev.c:415:
+ ena_assert_msg(ring->configured, "API violation. "
+ "Trying to release not configured queue");
CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#416: FILE: drivers/net/ena/ena_ethdev.c:416:
+ ena_assert_msg(ring->adapter->state != ENA_ADAPTER_STATE_RUNING,
WARNING: unnecessary whitespace before a quoted newline
#433: FILE: drivers/net/ena/ena_ethdev.c:433:
+ RTE_LOG(NOTICE, PMD, "RX Queue %d:%d released \n", ring->port_id, ring->id);
WARNING: void function return statements are not generally useful
#435: FILE: drivers/net/ena/ena_ethdev.c:435:
+ return;
+}
CHECK: No space is necessary after a cast
#439: FILE: drivers/net/ena/ena_ethdev.c:439:
+ struct ena_ring *ring = (struct ena_ring*) queue;
ERROR: "(foo*)" should be "(foo *)"
#439: FILE: drivers/net/ena/ena_ethdev.c:439:
+ struct ena_ring *ring = (struct ena_ring*) queue;
WARNING: quoted string split across lines
#444: FILE: drivers/net/ena/ena_ethdev.c:444:
+ ena_assert_msg(ring->configured, "API violation. Trying to release "
+ "not configured queue");
CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#445: FILE: drivers/net/ena/ena_ethdev.c:445:
+ ena_assert_msg(ring->adapter->state != ENA_ADAPTER_STATE_RUNING,
WARNING: unnecessary whitespace before a quoted newline
#467: FILE: drivers/net/ena/ena_ethdev.c:467:
+ RTE_LOG(NOTICE, PMD, "TX Queue %d:%d released \n", ring->port_id, ring->id);
WARNING: void function return statements are not generally useful
#469: FILE: drivers/net/ena/ena_ethdev.c:469:
+ return;
+}
WARNING: void function return statements are not generally useful
#485: FILE: drivers/net/ena/ena_ethdev.c:485:
+ return;
+}
ERROR: "foo* bar" should be "foo *bar"
#492: FILE: drivers/net/ena/ena_ethdev.c:492:
+ struct ena_tx_buffer* tx_buf =
WARNING: void function return statements are not generally useful
#502: FILE: drivers/net/ena/ena_ethdev.c:502:
+ return;
+}
ERROR: "(foo*)" should be "(foo *)"
#518: FILE: drivers/net/ena/ena_ethdev.c:518:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
CHECK: Blank lines aren't necessary after an open brace '{'
#550: FILE: drivers/net/ena/ena_ethdev.c:550:
+{
+
WARNING: Missing a blank line after declarations
#552: FILE: drivers/net/ena/ena_ethdev.c:552:
+ uint32_t max_frame_len = adapter->max_mtu;
+ if (adapter->rte_eth_dev_data->dev_conf.rxmode.jumbo_frame == 1)
CHECK: Blank lines aren't necessary before a close brace '}'
#569: FILE: drivers/net/ena/ena_ethdev.c:569:
+
+}
ERROR: "(foo*)" should be "(foo *)"
#573: FILE: drivers/net/ena/ena_ethdev.c:573:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
WARNING: Missing a blank line after declarations
#574: FILE: drivers/net/ena/ena_ethdev.c:574:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
+ rte_atomic64_init(&adapter->drv_stats->ierrors);
ERROR: "(foo*)" should be "(foo *)"
#583: FILE: drivers/net/ena/ena_ethdev.c:583:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
ERROR: "(foo*)" should be "(foo *)"
#619: FILE: drivers/net/ena/ena_ethdev.c:619:
+ adapter = (struct ena_adapter*)(dev->data->dev_private);
ERROR: "(foo*)" should be "(foo *)"
#643: FILE: drivers/net/ena/ena_ethdev.c:643:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
ERROR: space required before the open parenthesis '('
#646: FILE: drivers/net/ena/ena_ethdev.c:646:
+ if(!(adapter->state == ENA_ADAPTER_STATE_CONFIG ||
CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#672: FILE: drivers/net/ena/ena_ethdev.c:672:
+ adapter->state = ENA_ADAPTER_STATE_RUNING;
ERROR: "(foo*)" should be "(foo *)"
#706: FILE: drivers/net/ena/ena_ethdev.c:706:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
WARNING: quoted string split across lines
#728: FILE: drivers/net/ena/ena_ethdev.c:728:
+ RTE_LOG(ERR, PMD, "failed to create io TX queue #%d (ena_qid: %d)"
+ "rc: %d\n", queue_idx, ena_qid, rc);
ERROR: "(foo*)" should be "(foo *)"
#770: FILE: drivers/net/ena/ena_ethdev.c:770:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
ERROR: "(foo*)" should be "(foo *)"
#802: FILE: drivers/net/ena/ena_ethdev.c:802:
+ sizeof(struct rte_mbuf*) * nb_desc, RTE_CACHE_LINE_SIZE);
ERROR: "foo ** bar" should be "foo **bar"
#822: FILE: drivers/net/ena/ena_ethdev.c:822:
+ struct rte_mbuf ** mbufs = &rxq->rx_buffer_info[0];
ERROR: trailing statements should be on next line
#824: FILE: drivers/net/ena/ena_ethdev.c:824:
+ if (unlikely(!count)) return 0;
ERROR: space required after that ',' (ctx:VxV)
#836: FILE: drivers/net/ena/ena_ethdev.c:836:
+ PMD_RX_LOG(DEBUG,"there are no enough free buffers");
^
WARNING: Block comments use a trailing */ on a separate line
#902: FILE: drivers/net/ena/ena_ethdev.c:902:
+ * information */
ERROR: "(foo*)" should be "(foo *)"
#926: FILE: drivers/net/ena/ena_ethdev.c:926:
+ struct ena_adapter *adapter = (struct ena_adapter*)(eth_dev->data->dev_private);
ERROR: do not initialise statics to 0
#931: FILE: drivers/net/ena/ena_ethdev.c:931:
+ static int adapters_found = 0;
WARNING: braces {} are not necessary for single statement blocks
#942: FILE: drivers/net/ena/ena_ethdev.c:942:
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ return 0;
+ }
CHECK: Alignment should match open parenthesis
#980: FILE: drivers/net/ena/ena_ethdev.c:980:
+ PMD_INIT_LOG(ERR,
+ "Trying to use LLQ but llq_num is 0. Fall back into regular queues\n");
ERROR: "(foo*)" should be "(foo *)"
#997: FILE: drivers/net/ena/ena_ethdev.c:997:
+ eth_dev->data->mac_addrs = (struct ether_addr*)adapter->mac_addr;
CHECK: Blank lines aren't necessary after an open brace '{'
#1017: FILE: drivers/net/ena/ena_ethdev.c:1017:
+{
+
ERROR: "(foo*)" should be "(foo *)"
#1018: FILE: drivers/net/ena/ena_ethdev.c:1018:
+ struct ena_adapter *adapter = (struct ena_adapter*)(dev->data->dev_private);
WARNING: Missing a blank line after declarations
#1046: FILE: drivers/net/ena/ena_ethdev.c:1046:
+ struct ena_ring *ring = &adapter->tx_ring[i];
+ ring->configured = 0;
CHECK: Blank lines aren't necessary before a close brace '}'
#1052: FILE: drivers/net/ena/ena_ethdev.c:1052:
+
+ }
WARNING: Missing a blank line after declarations
#1056: FILE: drivers/net/ena/ena_ethdev.c:1056:
+ struct ena_ring *ring = &adapter->rx_ring[i];
+ ring->configured = 0;
ERROR: "(foo*)" should be "(foo *)"
#1074: FILE: drivers/net/ena/ena_ethdev.c:1074:
+ adapter = (struct ena_adapter*)(dev->data->dev_private);
ERROR: "(foo*)" should be "(foo *)"
#1114: FILE: drivers/net/ena/ena_ethdev.c:1114:
+ struct ena_ring *rx_ring = (struct ena_ring*)(rx_queue);
CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#1130: FILE: drivers/net/ena/ena_ethdev.c:1130:
+ if (unlikely(rx_ring->adapter->state != ENA_ADAPTER_STATE_RUNING)) {
ERROR: trailing statements should be on next line
#1137: FILE: drivers/net/ena/ena_ethdev.c:1137:
+ if (unlikely(nb_pkts > desc_in_use)) nb_pkts = desc_in_use;
WARNING: unnecessary whitespace before a quoted newline
#1147: FILE: drivers/net/ena/ena_ethdev.c:1147:
+ RTE_LOG(ERR, PMD, "ena_com_rx_pkt error %d \n", rc);
WARNING: Missing a blank line after declarations
#1155: FILE: drivers/net/ena/ena_ethdev.c:1155:
+ int segments = 0;
+ while (segments < ena_rx_ctx.descs) {
ERROR: "(foo*)" should be "(foo *)"
#1198: FILE: drivers/net/ena/ena_ethdev.c:1198:
+ struct ena_ring *tx_ring = (struct ena_ring*)(tx_queue);
CHECK: 'RUNING' may be misspelled - perhaps 'RUNNING'?
#1211: FILE: drivers/net/ena/ena_ethdev.c:1211:
+ if (unlikely(tx_ring->adapter->state != ENA_ADAPTER_STATE_RUNING)) {
CHECK: Blank lines aren't necessary after an open brace '{'
#1218: FILE: drivers/net/ena/ena_ethdev.c:1218:
+ for (sent_idx = 0; sent_idx < nb_pkts; sent_idx++) {
+
ERROR: "(foo*)" should be "(foo *)"
#1235: FILE: drivers/net/ena/ena_ethdev.c:1235:
+ ena_tx_ctx.push_header = (void *)((char*)(mbuf->buf_addr) + mbuf->data_off);
CHECK: spaces preferred around that '|' (ctx:VxV)
#1241: FILE: drivers/net/ena/ena_ethdev.c:1241:
+ if (unlikely(mbuf->ol_flags & (PKT_RX_L4_CKSUM_BAD|PKT_RX_IP_CKSUM_BAD))) {
^
WARNING: braces {} are not necessary for single statement blocks
#1241: FILE: drivers/net/ena/ena_ethdev.c:1241:
+ if (unlikely(mbuf->ol_flags & (PKT_RX_L4_CKSUM_BAD|PKT_RX_IP_CKSUM_BAD))) {
+ rte_atomic64_inc(&tx_ring->adapter->drv_stats->ierrors);
+ }
ERROR: space required before the open parenthesis '('
#1255: FILE: drivers/net/ena/ena_ethdev.c:1255:
+ while((mbuf = mbuf->next) != NULL) {
total: 31 errors, 31 warnings, 21 checks, 1327 lines checked
drivers/net/ena/ena_ethdev.c has style problems, please review.
NOTE: Ignored message types: BIT_MACRO COMPARISON_TO_NULL PREFER_KERNEL_TYPES
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
prev parent reply other threads:[~2016-02-22 21:08 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-22 19:26 [PATCH v3 0/4] DPDK polling-mode driver for Amazon Elastic Network Adapters (ENA) Jan Medala
2016-02-22 19:26 ` [PATCH v3 1/4] ena: Amazon ENA documentation Jan Medala
2016-02-23 10:19 ` Mcnamara, John
2016-02-22 19:26 ` [PATCH v3 2/4] ena: Amazon ENA communication layer Jan Medala
2016-02-22 19:26 ` [PATCH v3 3/4] ena: Amazon ENA communication layer for DPDK platform Jan Medala
2016-02-22 19:26 ` [PATCH v3 4/4] ena: DPDK polling-mode driver for Amazon Elastic Network Adapters (ENA) Jan Medala
2016-02-22 21:07 ` Stephen Hemminger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160222130747.4843fc2f@xeon-e3 \
--to=stephen@networkplumber.org \
--cc=dev@dpdk.org \
--cc=jan@semihalf.com \
--cc=matua@amazon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.