Thread (13 messages) 13 messages, 3 authors, 2022-01-03

Re: [PATCH net-next v2 6/8] net/funeth: add the data path

From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-01-01 17:57:41

quoted hunk ↗ jump to hunk
+++ b/drivers/net/ethernet/fungible/funeth/funeth_rx.c
+/* See if a page is running low on refs we are holding and if so take more. */
+static inline void refresh_refs(struct funeth_rxbuf *buf)
+{
+	if (unlikely(buf->pg_refs < MIN_PAGE_REFS)) {
+		buf->pg_refs += EXTRA_PAGE_REFS;
+		page_ref_add(buf->page, EXTRA_PAGE_REFS);
+	}
+}
No inline functions in C files please. Let the compiler decide. Please
check for this in the whole driver.
+/* A CQE contains a fixed completion structure along with optional metadata and
+ * even packet data. Given the start address of a CQE return the start of the
+ * contained fixed structure, which lies at the end.
+ */
+static inline const void *cqe_to_info(const void *cqe)
+{
+	return cqe + FUNETH_CQE_INFO_OFFSET;
+}
+
+/* The inverse of cqe_to_info(). */
+static inline const void *info_to_cqe(const void *cqe_info)
+{
+	return cqe_info - FUNETH_CQE_INFO_OFFSET;
+}
This looks pretty brittle. Can you define a struct cqe, so avoid all
this arithmetic on void * pointers?
+static unsigned int write_pkt_desc(struct sk_buff *skb, struct funeth_txq *q,
+				   unsigned int tls_len)
+{
+	unsigned int idx = q->prod_cnt & q->mask;
+	struct fun_eth_tx_req *req = fun_tx_desc_addr(q, idx);
You need to move the assignment into the body to keep with reverse
Christmas tree.
+	if (txq_to_end(q, gle) == 0) {
+		gle = (struct fun_dataop_gl *)q->desc;
+		for ( ; i < ngle; i++, gle++)
+			fun_dataop_gl_init(gle, 0, 0, lens[i], addrs[i]);
+	}
+
+#ifdef CONFIG_TLS_DEVICE
If possible, use if (IS_ENABLED(TLS_DEVICE)), not #ifdef. It will give
you better compile testing, if it works.
+	if (unlikely(tls_len)) {
+		struct fun_eth_tls *tls = (struct fun_eth_tls *)gle;
+		struct fun_ktls_tx_ctx *tls_ctx;
+
+		req->len8 += FUNETH_TLS_SZ / 8;
+		req->flags = cpu_to_be16(FUN_ETH_TX_TLS);
+
+		tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX);
+		tls->tlsid = tls_ctx->tlsid;
+		tls_ctx->next_seq += tls_len;
+
+		u64_stats_update_begin(&q->syncp);
+		q->stats.tx_tls_bytes += tls_len;
+		q->stats.tx_tls_pkts += 1 + extra_pkts;
+		u64_stats_update_end(&q->syncp);
+	}
+#endif
+/* Create a Tx queue, allocating all the host and device resources needed. */
+struct funeth_txq *funeth_txq_create(struct net_device *dev, unsigned int qidx,
+				     unsigned int ndesc, struct fun_irq *irq)
+{
+	struct funeth_priv *fp = netdev_priv(dev);
+	unsigned int ethid = fp->ethid_start + qidx;
+	int numa_node, err = -ENOMEM;
+	struct funeth_txq *q;
+	const char *qtype;
...
+	netif_info(fp, ifup, dev,
+		   "%s queue %u, depth %u, HW qid %u, IRQ idx %u, node %d\n",
+		   qtype, qidx, ndesc, q->hw_qid, q->irq_idx, numa_node);
Probably should be _dbg(). We try not to spam the kernel log.
+struct funeth_txq_stats {  /* per Tx queue SW counters */
+	u64 tx_pkts;       /* # of Tx packets */
+	u64 tx_bytes;      /* total bytes of Tx packets */
+	u64 tx_cso;        /* # of packets with checksum offload */
+	u64 tx_tso;        /* # of TSO super-packets */
+	u64 tx_more;       /* # of DBs elided due to xmit_more */
+	u64 tx_nstops;     /* # of times the queue has stopped */
+	u64 tx_nrestarts;  /* # of times the queue has restarted */
+	u64 tx_map_err;    /* # of packets dropped due to DMA mapping errors */
+	u64 tx_len_err;    /* # of packets dropped due to unsupported length */
+	u64 tx_xdp_full;   /* # of XDP packets that could not be enqueued */
+#ifdef CONFIG_TLS_DEVICE
+	u64 tx_tls_pkts;   /* # of Tx TLS packets offloaded to HW */
+	u64 tx_tls_bytes;  /* Tx bytes of HW-handled TLS payload */
+	u64 tx_tls_fallback; /* attempted Tx TLS offloads punted to SW */
+	u64 tx_tls_drops;  /* attempted Tx TLS offloads dropped */
+#endif
You might want to think about this, and kAPI stability. I assume it
implies a different number of statistics are returned by ethtool -S,
depending on how the driver is built. That could be surprising for
user space. It might be better to always have the statistics, and just
return 0 when TLS is not available.

       Andrew
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help