From: Stephen Hemminger <stephen@networkplumber.org>
To: Jan Medala <jan@semihalf.com>
Cc: dev@dpdk.org, matua@amazon.com
Subject: Re: [dpdk-dev] [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 Jan Medala
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 1/4] ena: Amazon ENA documentation Jan Medala
2016-02-23 10:19 ` Mcnamara, John
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 2/4] ena: Amazon ENA communication layer Jan Medala
2016-02-22 19:26 ` [dpdk-dev] [PATCH v3 3/4] ena: Amazon ENA communication layer for DPDK platform Jan Medala
2016-02-22 19:26 ` [dpdk-dev] [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).