[PATCH V2 net-next 0/4] Enhance current features in ena driver

[PATCH V2 net-next 0/4] Enhance current features in ena driver

[sameehj]

From: Sameeh Jubran <sameehj@amazon.com>

This series adds the following:
* Exposes new device stats using ethtool.
* Adds and exposes the stats of xdp TX queues through ethtool.

V1: Use unsigned long for pointer math instead of uintptr_t

Sameeh Jubran (4):
  net: ena: ethtool: use unsigned long for pointer arithmetics
  net: ena: ethtool: Add new device statistics
  net: ena: ethtool: add stats printing to XDP queues
  net: ena: xdp: add queue counters for xdp actions

 drivers/net/ethernet/amazon/ena/ena_admin_defs.h |  37 ++++-
 drivers/net/ethernet/amazon/ena/ena_com.c        |  19 ++-
 drivers/net/ethernet/amazon/ena/ena_com.h        |   9 ++
 drivers/net/ethernet/amazon/ena/ena_ethtool.c    | 170 +++++++++++++++++------
 drivers/net/ethernet/amazon/ena/ena_netdev.c     |  45 +++++-
 drivers/net/ethernet/amazon/ena/ena_netdev.h     |   9 ++
 6 files changed, 236 insertions(+), 53 deletions(-)

-- 
2.16.6

[PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[sameehj]

From: Sameeh Jubran <sameehj@amazon.com>

unsigned long is the type for doing maths on pointers.

Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 430275bc0..897beddc5 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -141,8 +141,8 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
 			ena_stats = &ena_stats_tx_strings[j];
 
-			ptr = (u64 *)((uintptr_t)&ring->tx_stats +
-				(uintptr_t)ena_stats->stat_offset);
+			ptr = (u64 *)((unsigned long)&ring->tx_stats +
+				ena_stats->stat_offset);
 
 			ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
 		}
@@ -153,8 +153,8 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 		for (j = 0; j < ENA_STATS_ARRAY_RX; j++) {
 			ena_stats = &ena_stats_rx_strings[j];
 
-			ptr = (u64 *)((uintptr_t)&ring->rx_stats +
-				(uintptr_t)ena_stats->stat_offset);
+			ptr = (u64 *)((unsigned long)&ring->rx_stats +
+				ena_stats->stat_offset);
 
 			ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
 		}
@@ -170,8 +170,8 @@ static void ena_dev_admin_queue_stats(struct ena_adapter *adapter, u64 **data)
 	for (i = 0; i < ENA_STATS_ARRAY_ENA_COM; i++) {
 		ena_stats = &ena_stats_ena_com_strings[i];
 
-		ptr = (u64 *)((uintptr_t)&adapter->ena_dev->admin_queue.stats +
-			(uintptr_t)ena_stats->stat_offset);
+		ptr = (u64 *)((unsigned long)&adapter->ena_dev->admin_queue.stats +
+			ena_stats->stat_offset);
 
 		*(*data)++ = *ptr;
 	}
@@ -189,8 +189,8 @@ static void ena_get_ethtool_stats(struct net_device *netdev,
 	for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) {
 		ena_stats = &ena_stats_global_strings[i];
 
-		ptr = (u64 *)((uintptr_t)&adapter->dev_stats +
-			(uintptr_t)ena_stats->stat_offset);
+		ptr = (u64 *)((unsigned long)&adapter->dev_stats +
+			ena_stats->stat_offset);
 
 		ena_safe_update_stat(ptr, data++, &adapter->syncp);
 	}
-- 
2.16.6

[PATCH V2 net-next 2/4] net: ena: ethtool: Add new device statistics

[sameehj]

From: Sameeh Jubran <sameehj@amazon.com>

The new metrics provide granular visibility along multiple network
dimensions and enable troubleshooting and remediation of issues caused
by instances exceeding network performance allowances.

The new statistics can be queried using ethtool command.

Signed-off-by: Guy Tzalik <gtzalik@amazon.com>
Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_admin_defs.h |  37 +++++++-
 drivers/net/ethernet/amazon/ena/ena_com.c        |  19 +++-
 drivers/net/ethernet/amazon/ena/ena_com.h        |   9 ++
 drivers/net/ethernet/amazon/ena/ena_ethtool.c    | 106 ++++++++++++++++++-----
 drivers/net/ethernet/amazon/ena/ena_netdev.c     |  18 ++++
 drivers/net/ethernet/amazon/ena/ena_netdev.h     |   4 +
 6 files changed, 170 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index b818a169c..86869baa7 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -117,6 +117,8 @@ enum ena_admin_completion_policy_type {
 enum ena_admin_get_stats_type {
 	ENA_ADMIN_GET_STATS_TYPE_BASIC              = 0,
 	ENA_ADMIN_GET_STATS_TYPE_EXTENDED           = 1,
+	/* extra HW stats for specific network interface */
+	ENA_ADMIN_GET_STATS_TYPE_ENI                = 2,
 };
 
 enum ena_admin_get_stats_scope {
@@ -410,10 +412,43 @@ struct ena_admin_basic_stats {
 	u32 tx_drops_high;
 };
 
+/* ENI Statistics Command. */
+struct ena_admin_eni_stats {
+	/* The number of packets shaped due to inbound aggregate BW
+	 * allowance being exceeded
+	 */
+	u64 bw_in_allowance_exceeded;
+
+	/* The number of packets shaped due to outbound aggregate BW
+	 * allowance being exceeded
+	 */
+	u64 bw_out_allowance_exceeded;
+
+	/* The number of packets shaped due to PPS allowance being exceeded */
+	u64 pps_allowance_exceeded;
+
+	/* The number of packets shaped due to connection tracking
+	 * allowance being exceeded and leading to failure in establishment
+	 * of new connections
+	 */
+	u64 conntrack_allowance_exceeded;
+
+	/* The number of packets shaped due to linklocal packet rate
+	 * allowance being exceeded
+	 */
+	u64 linklocal_allowance_exceeded;
+};
+
 struct ena_admin_acq_get_stats_resp {
 	struct ena_admin_acq_common_desc acq_common_desc;
 
-	struct ena_admin_basic_stats basic_stats;
+	union {
+		u64 raw[7];
+
+		struct ena_admin_basic_stats basic_stats;
+
+		struct ena_admin_eni_stats eni_stats;
+	} u;
 };
 
 struct ena_admin_get_set_feature_common_desc {
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 435bf05a8..452e66b39 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -2167,6 +2167,21 @@ static int ena_get_dev_stats(struct ena_com_dev *ena_dev,
 	return ret;
 }
 
+int ena_com_get_eni_stats(struct ena_com_dev *ena_dev,
+			  struct ena_admin_eni_stats *stats)
+{
+	struct ena_com_stats_ctx ctx;
+	int ret;
+
+	memset(&ctx, 0x0, sizeof(ctx));
+	ret = ena_get_dev_stats(ena_dev, &ctx, ENA_ADMIN_GET_STATS_TYPE_ENI);
+	if (likely(ret == 0))
+		memcpy(stats, &ctx.get_resp.u.eni_stats,
+		       sizeof(ctx.get_resp.u.eni_stats));
+
+	return ret;
+}
+
 int ena_com_get_dev_basic_stats(struct ena_com_dev *ena_dev,
 				struct ena_admin_basic_stats *stats)
 {
@@ -2176,8 +2191,8 @@ int ena_com_get_dev_basic_stats(struct ena_com_dev *ena_dev,
 	memset(&ctx, 0x0, sizeof(ctx));
 	ret = ena_get_dev_stats(ena_dev, &ctx, ENA_ADMIN_GET_STATS_TYPE_BASIC);
 	if (likely(ret == 0))
-		memcpy(stats, &ctx.get_resp.basic_stats,
-		       sizeof(ctx.get_resp.basic_stats));
+		memcpy(stats, &ctx.get_resp.u.basic_stats,
+		       sizeof(ctx.get_resp.u.basic_stats));
 
 	return ret;
 }
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 4287d47b2..e4aafeda0 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -616,6 +616,15 @@ int ena_com_get_dev_attr_feat(struct ena_com_dev *ena_dev,
 int ena_com_get_dev_basic_stats(struct ena_com_dev *ena_dev,
 				struct ena_admin_basic_stats *stats);
 
+/* ena_com_get_eni_stats - Get extended network interface statistics
+ * @ena_dev: ENA communication layer struct
+ * @stats: stats return value
+ *
+ * @return: 0 on Success and negative value otherwise.
+ */
+int ena_com_get_eni_stats(struct ena_com_dev *ena_dev,
+			  struct ena_admin_eni_stats *stats);
+
 /* ena_com_set_dev_mtu - Configure the device mtu.
  * @ena_dev: ENA communication layer struct
  * @mtu: mtu value
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 897beddc5..f0cc10aa1 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -49,6 +49,11 @@ struct ena_stats {
 	.stat_offset = offsetof(struct ena_stats_##stat_type, stat) \
 }
 
+#define ENA_STAT_HW_ENTRY(stat, stat_type) { \
+	.name = #stat, \
+	.stat_offset = offsetof(struct ena_admin_##stat_type, stat) \
+}
+
 #define ENA_STAT_RX_ENTRY(stat) \
 	ENA_STAT_ENTRY(stat, rx)
 
@@ -58,6 +63,9 @@ struct ena_stats {
 #define ENA_STAT_GLOBAL_ENTRY(stat) \
 	ENA_STAT_ENTRY(stat, dev)
 
+#define ENA_STAT_ENI_ENTRY(stat) \
+	ENA_STAT_HW_ENTRY(stat, eni_stats)
+
 static const struct ena_stats ena_stats_global_strings[] = {
 	ENA_STAT_GLOBAL_ENTRY(tx_timeout),
 	ENA_STAT_GLOBAL_ENTRY(suspend),
@@ -68,6 +76,14 @@ static const struct ena_stats ena_stats_global_strings[] = {
 	ENA_STAT_GLOBAL_ENTRY(admin_q_pause),
 };
 
+static const struct ena_stats ena_stats_eni_strings[] = {
+	ENA_STAT_ENI_ENTRY(bw_in_allowance_exceeded),
+	ENA_STAT_ENI_ENTRY(bw_out_allowance_exceeded),
+	ENA_STAT_ENI_ENTRY(pps_allowance_exceeded),
+	ENA_STAT_ENI_ENTRY(conntrack_allowance_exceeded),
+	ENA_STAT_ENI_ENTRY(linklocal_allowance_exceeded),
+};
+
 static const struct ena_stats ena_stats_tx_strings[] = {
 	ENA_STAT_TX_ENTRY(cnt),
 	ENA_STAT_TX_ENTRY(bytes),
@@ -110,10 +126,12 @@ static const struct ena_stats ena_stats_ena_com_strings[] = {
 	ENA_STAT_ENA_COM_ENTRY(no_completion),
 };
 
-#define ENA_STATS_ARRAY_GLOBAL	ARRAY_SIZE(ena_stats_global_strings)
-#define ENA_STATS_ARRAY_TX	ARRAY_SIZE(ena_stats_tx_strings)
-#define ENA_STATS_ARRAY_RX	ARRAY_SIZE(ena_stats_rx_strings)
-#define ENA_STATS_ARRAY_ENA_COM	ARRAY_SIZE(ena_stats_ena_com_strings)
+#define ENA_STATS_ARRAY_GLOBAL		ARRAY_SIZE(ena_stats_global_strings)
+#define ENA_STATS_ARRAY_TX		ARRAY_SIZE(ena_stats_tx_strings)
+#define ENA_STATS_ARRAY_RX		ARRAY_SIZE(ena_stats_rx_strings)
+#define ENA_STATS_ARRAY_ENA_COM		ARRAY_SIZE(ena_stats_ena_com_strings)
+#define ENA_STATS_ARRAY_ENI(adapter)	\
+	(ARRAY_SIZE(ena_stats_eni_strings) * (adapter)->eni_stats_supported)
 
 static void ena_safe_update_stat(u64 *src, u64 *dst,
 				 struct u64_stats_sync *syncp)
@@ -177,11 +195,10 @@ static void ena_dev_admin_queue_stats(struct ena_adapter *adapter, u64 **data)
 	}
 }
 
-static void ena_get_ethtool_stats(struct net_device *netdev,
-				  struct ethtool_stats *stats,
-				  u64 *data)
+static void ena_get_stats(struct ena_adapter *adapter,
+			  u64 *data,
+			  bool eni_stats_needed)
 {
-	struct ena_adapter *adapter = netdev_priv(netdev);
 	const struct ena_stats *ena_stats;
 	u64 *ptr;
 	int i;
@@ -195,10 +212,42 @@ static void ena_get_ethtool_stats(struct net_device *netdev,
 		ena_safe_update_stat(ptr, data++, &adapter->syncp);
 	}
 
+	if (eni_stats_needed) {
+		ena_update_hw_stats(adapter);
+		for (i = 0; i < ENA_STATS_ARRAY_ENI(adapter); i++) {
+			ena_stats = &ena_stats_eni_strings[i];
+
+			ptr = (u64 *)((unsigned long)&adapter->eni_stats +
+				ena_stats->stat_offset);
+
+			ena_safe_update_stat(ptr, data++, &adapter->syncp);
+		}
+	}
+
 	ena_queue_stats(adapter, &data);
 	ena_dev_admin_queue_stats(adapter, &data);
 }
 
+static void ena_get_ethtool_stats(struct net_device *netdev,
+				  struct ethtool_stats *stats,
+				  u64 *data)
+{
+	struct ena_adapter *adapter = netdev_priv(netdev);
+
+	ena_get_stats(adapter, data, adapter->eni_stats_supported);
+}
+
+static int ena_get_sw_stats_count(struct ena_adapter *adapter)
+{
+	return adapter->num_io_queues * (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX)
+		+ ENA_STATS_ARRAY_GLOBAL + ENA_STATS_ARRAY_ENA_COM;
+}
+
+static int ena_get_hw_stats_count(struct ena_adapter *adapter)
+{
+	return ENA_STATS_ARRAY_ENI(adapter);
+}
+
 int ena_get_sset_count(struct net_device *netdev, int sset)
 {
 	struct ena_adapter *adapter = netdev_priv(netdev);
@@ -206,8 +255,7 @@ int ena_get_sset_count(struct net_device *netdev, int sset)
 	if (sset != ETH_SS_STATS)
 		return -EOPNOTSUPP;
 
-	return adapter->num_io_queues * (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX)
-		+ ENA_STATS_ARRAY_GLOBAL + ENA_STATS_ARRAY_ENA_COM;
+	return ena_get_sw_stats_count(adapter) + ena_get_hw_stats_count(adapter);
 }
 
 static void ena_queue_strings(struct ena_adapter *adapter, u8 **data)
@@ -249,25 +297,43 @@ static void ena_com_dev_strings(u8 **data)
 	}
 }
 
-static void ena_get_strings(struct net_device *netdev, u32 sset, u8 *data)
+static void ena_get_strings(struct ena_adapter *adapter,
+			    u8 *data,
+			    bool eni_stats_needed)
 {
-	struct ena_adapter *adapter = netdev_priv(netdev);
 	const struct ena_stats *ena_stats;
 	int i;
 
-	if (sset != ETH_SS_STATS)
-		return;
-
 	for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) {
 		ena_stats = &ena_stats_global_strings[i];
 		memcpy(data, ena_stats->name, ETH_GSTRING_LEN);
 		data += ETH_GSTRING_LEN;
 	}
 
+	if (eni_stats_needed) {
+		for (i = 0; i < ENA_STATS_ARRAY_ENI(adapter); i++) {
+			ena_stats = &ena_stats_eni_strings[i];
+			memcpy(data, ena_stats->name, ETH_GSTRING_LEN);
+			data += ETH_GSTRING_LEN;
+		}
+	}
+
 	ena_queue_strings(adapter, &data);
 	ena_com_dev_strings(&data);
 }
 
+static void ena_get_ethtool_strings(struct net_device *netdev,
+				    u32 sset,
+				    u8 *data)
+{
+	struct ena_adapter *adapter = netdev_priv(netdev);
+
+	if (sset != ETH_SS_STATS)
+		return;
+
+	ena_get_strings(adapter, data, adapter->eni_stats_supported);
+}
+
 static int ena_get_link_ksettings(struct net_device *netdev,
 				  struct ethtool_link_ksettings *link_ksettings)
 {
@@ -847,7 +913,7 @@ static const struct ethtool_ops ena_ethtool_ops = {
 	.get_ringparam		= ena_get_ringparam,
 	.set_ringparam		= ena_set_ringparam,
 	.get_sset_count         = ena_get_sset_count,
-	.get_strings		= ena_get_strings,
+	.get_strings		= ena_get_ethtool_strings,
 	.get_ethtool_stats      = ena_get_ethtool_stats,
 	.get_rxnfc		= ena_get_rxnfc,
 	.set_rxnfc		= ena_set_rxnfc,
@@ -875,7 +941,7 @@ static void ena_dump_stats_ex(struct ena_adapter *adapter, u8 *buf)
 	int strings_num;
 	int i, rc;
 
-	strings_num = ena_get_sset_count(netdev, ETH_SS_STATS);
+	strings_num = ena_get_sw_stats_count(adapter);
 	if (strings_num <= 0) {
 		netif_err(adapter, drv, netdev, "Can't get stats num\n");
 		return;
@@ -895,13 +961,13 @@ static void ena_dump_stats_ex(struct ena_adapter *adapter, u8 *buf)
 				GFP_ATOMIC);
 	if (!data_buf) {
 		netif_err(adapter, drv, netdev,
-			  "failed to allocate data buf\n");
+			  "Failed to allocate data buf\n");
 		devm_kfree(&adapter->pdev->dev, strings_buf);
 		return;
 	}
 
-	ena_get_strings(netdev, ETH_SS_STATS, strings_buf);
-	ena_get_ethtool_stats(netdev, NULL, data_buf);
+	ena_get_strings(adapter, strings_buf, false);
+	ena_get_stats(adapter, data_buf, false);
 
 	/* If there is a buffer, dump stats, otherwise print them to dmesg */
 	if (buf)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 2a6c9725e..89c6e9ede 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -3181,6 +3181,19 @@ err:
 	ena_com_delete_debug_area(adapter->ena_dev);
 }
 
+int ena_update_hw_stats(struct ena_adapter *adapter)
+{
+	int rc = 0;
+
+	rc = ena_com_get_eni_stats(adapter->ena_dev, &adapter->eni_stats);
+	if (rc) {
+		dev_info_once(&adapter->pdev->dev, "Failed to get ENI stats\n");
+		return rc;
+	}
+
+	return 0;
+}
+
 static void ena_get_stats64(struct net_device *netdev,
 			    struct rtnl_link_stats64 *stats)
 {
@@ -4301,6 +4314,11 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ena_config_debug_area(adapter);
 
+	if (!ena_update_hw_stats(adapter))
+		adapter->eni_stats_supported = true;
+	else
+		adapter->eni_stats_supported = false;
+
 	memcpy(adapter->netdev->perm_addr, adapter->mac_addr, netdev->addr_len);
 
 	netif_carrier_off(netdev);
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 0c8504006..4c95a4d93 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -405,6 +405,8 @@ struct ena_adapter {
 
 	struct u64_stats_sync syncp;
 	struct ena_stats_dev dev_stats;
+	struct ena_admin_eni_stats eni_stats;
+	bool eni_stats_supported;
 
 	/* last queue index that was checked for uncompleted tx packets */
 	u32 last_monitored_tx_qid;
@@ -422,6 +424,8 @@ void ena_dump_stats_to_dmesg(struct ena_adapter *adapter);
 
 void ena_dump_stats_to_buf(struct ena_adapter *adapter, u8 *buf);
 
+int ena_update_hw_stats(struct ena_adapter *adapter);
+
 int ena_update_queue_sizes(struct ena_adapter *adapter,
 			   u32 new_tx_size,
 			   u32 new_rx_size);
-- 
2.16.6

[PATCH V2 net-next 3/4] net: ena: ethtool: add stats printing to XDP queues

[sameehj]

From: Sameeh Jubran <sameehj@amazon.com>

Added statistics for TX queues that are used for XDP TX. The statistics
are the same as the ones printed for regular non-XDP TX queues.

The XDP queue statistics can be queried using
`ethtool -S <ifname>`

Signed-off-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 47 +++++++++++++++++----------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index f0cc10aa1..bc12a0768 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -152,7 +152,7 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 	u64 *ptr;
 	int i, j;
 
-	for (i = 0; i < adapter->num_io_queues; i++) {
+	for (i = 0; i < adapter->num_io_queues + adapter->xdp_num_queues; i++) {
 		/* Tx stats */
 		ring = &adapter->tx_ring[i];
 
@@ -164,17 +164,19 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 
 			ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
 		}
+		/* XDP TX queues don't have a RX queue counterpart */
+		if (!ENA_IS_XDP_INDEX(adapter, i)) {
+			/* Rx stats */
+			ring = &adapter->rx_ring[i];
 
-		/* Rx stats */
-		ring = &adapter->rx_ring[i];
+			for (j = 0; j < ENA_STATS_ARRAY_RX; j++) {
+				ena_stats = &ena_stats_rx_strings[j];
 
-		for (j = 0; j < ENA_STATS_ARRAY_RX; j++) {
-			ena_stats = &ena_stats_rx_strings[j];
+				ptr = (u64 *)((unsigned long)&ring->rx_stats +
+					ena_stats->stat_offset);
 
-			ptr = (u64 *)((unsigned long)&ring->rx_stats +
-				ena_stats->stat_offset);
-
-			ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
+				ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
+			}
 		}
 	}
 }
@@ -240,6 +242,7 @@ static void ena_get_ethtool_stats(struct net_device *netdev,
 static int ena_get_sw_stats_count(struct ena_adapter *adapter)
 {
 	return adapter->num_io_queues * (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX)
+		+ adapter->xdp_num_queues * ENA_STATS_ARRAY_TX
 		+ ENA_STATS_ARRAY_GLOBAL + ENA_STATS_ARRAY_ENA_COM;
 }
 
@@ -261,24 +264,32 @@ int ena_get_sset_count(struct net_device *netdev, int sset)
 static void ena_queue_strings(struct ena_adapter *adapter, u8 **data)
 {
 	const struct ena_stats *ena_stats;
+	bool is_xdp;
 	int i, j;
 
-	for (i = 0; i < adapter->num_io_queues; i++) {
+	for (i = 0; i < adapter->num_io_queues + adapter->xdp_num_queues; i++) {
+		is_xdp = ENA_IS_XDP_INDEX(adapter, i);
 		/* Tx stats */
 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
 			ena_stats = &ena_stats_tx_strings[j];
 
 			snprintf(*data, ETH_GSTRING_LEN,
-				 "queue_%u_tx_%s", i, ena_stats->name);
-			(*data) += ETH_GSTRING_LEN;
+				 "queue_%u_%s_%s", i,
+				 is_xdp ? "xdp_tx" : "tx", ena_stats->name);
+			 (*data) += ETH_GSTRING_LEN;
 		}
-		/* Rx stats */
-		for (j = 0; j < ENA_STATS_ARRAY_RX; j++) {
-			ena_stats = &ena_stats_rx_strings[j];
 
-			snprintf(*data, ETH_GSTRING_LEN,
-				 "queue_%u_rx_%s", i, ena_stats->name);
-			(*data) += ETH_GSTRING_LEN;
+		if (!is_xdp) {
+			/* RX stats, in XDP there isn't a RX queue
+			 * counterpart
+			 */
+			for (j = 0; j < ENA_STATS_ARRAY_RX; j++) {
+				ena_stats = &ena_stats_rx_strings[j];
+
+				snprintf(*data, ETH_GSTRING_LEN,
+					 "queue_%u_rx_%s", i, ena_stats->name);
+				(*data) += ETH_GSTRING_LEN;
+			}
 		}
 	}
 }
-- 
2.16.6

[PATCH V2 net-next 4/4] net: ena: xdp: add queue counters for xdp actions

[sameehj]

From: Sameeh Jubran <sameehj@amazon.com>

When using XDP every ingress packet is passed to an eBPF (xdp) program
which returns an action for this packet.

This patch adds counters for the number of times each such action was
received. It also counts all the invalid actions received from the eBPF
program.

Signed-off-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c |  5 +++++
 drivers/net/ethernet/amazon/ena/ena_netdev.c  | 27 +++++++++++++++++++++------
 drivers/net/ethernet/amazon/ena/ena_netdev.h  |  5 +++++
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index bc12a0768..e558da909 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -116,6 +116,11 @@ static const struct ena_stats ena_stats_rx_strings[] = {
 	ENA_STAT_RX_ENTRY(bad_req_id),
 	ENA_STAT_RX_ENTRY(empty_rx_ring),
 	ENA_STAT_RX_ENTRY(csum_unchecked),
+	ENA_STAT_RX_ENTRY(xdp_aborted),
+	ENA_STAT_RX_ENTRY(xdp_drop),
+	ENA_STAT_RX_ENTRY(xdp_pass),
+	ENA_STAT_RX_ENTRY(xdp_tx),
+	ENA_STAT_RX_ENTRY(xdp_invalid),
 };
 
 static const struct ena_stats ena_stats_ena_com_strings[] = {
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 89c6e9ede..0f8692783 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -365,6 +365,7 @@ static int ena_xdp_execute(struct ena_ring *rx_ring,
 {
 	struct bpf_prog *xdp_prog;
 	u32 verdict = XDP_PASS;
+	u64 *xdp_stat;
 
 	rcu_read_lock();
 	xdp_prog = READ_ONCE(rx_ring->xdp_bpf_prog);
@@ -374,17 +375,31 @@ static int ena_xdp_execute(struct ena_ring *rx_ring,
 
 	verdict = bpf_prog_run_xdp(xdp_prog, xdp);
 
-	if (verdict == XDP_TX)
+	if (verdict == XDP_TX) {
 		ena_xdp_xmit_buff(rx_ring->netdev,
-				  xdp,
-				  rx_ring->qid + rx_ring->adapter->num_io_queues,
-				  rx_info);
-	else if (unlikely(verdict == XDP_ABORTED))
+				xdp,
+				rx_ring->qid + rx_ring->adapter->num_io_queues,
+				rx_info);
+
+		xdp_stat = &rx_ring->rx_stats.xdp_tx;
+	} else if (unlikely(verdict == XDP_ABORTED)) {
 		trace_xdp_exception(rx_ring->netdev, xdp_prog, verdict);
-	else if (unlikely(verdict > XDP_TX))
+		xdp_stat = &rx_ring->rx_stats.xdp_aborted;
+	} else if (unlikely(verdict == XDP_DROP)) {
+		xdp_stat = &rx_ring->rx_stats.xdp_drop;
+	} else if (unlikely(verdict == XDP_PASS)) {
+		xdp_stat = &rx_ring->rx_stats.xdp_pass;
+	} else {
 		bpf_warn_invalid_xdp_action(verdict);
+		xdp_stat = &rx_ring->rx_stats.xdp_invalid;
+	}
+
+	u64_stats_update_begin(&rx_ring->syncp);
+	(*xdp_stat)++;
+	u64_stats_update_end(&rx_ring->syncp);
 out:
 	rcu_read_unlock();
+
 	return verdict;
 }
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 4c95a4d93..52abb6a4f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -261,6 +261,11 @@ struct ena_stats_rx {
 	u64 bad_req_id;
 	u64 empty_rx_ring;
 	u64 csum_unchecked;
+	u64 xdp_aborted;
+	u64 xdp_drop;
+	u64 xdp_pass;
+	u64 xdp_tx;
+	u64 xdp_invalid;
 };
 
 struct ena_ring {
-- 
2.16.6

Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Andrew Lunn]

On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote:
> From: Sameeh Jubran <sameehj@amazon.com>
> 
> unsigned long is the type for doing maths on pointers.

Maths on pointers is perfectly valid. The real issue here is you have
all your types mixed up.

> -			ptr = (u64 *)((uintptr_t)&ring->tx_stats +
> -				(uintptr_t)ena_stats->stat_offset);
> +			ptr = (u64 *)((unsigned long)&ring->tx_stats +
> +				ena_stats->stat_offset);

struct ena_ring {
...
        union {
		struct ena_stats_tx tx_stats;
		struct ena_stats_rx rx_stats;
	};

struct ena_stats_tx {
	u64 cnt;
	u64 bytes;
	u64 queue_stop;
	u64 prepare_ctx_err;
	u64 queue_wakeup;
	...
}

&ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on
that, adding 1 for example, takes you forward a full ena_stats_tx
structure. Not what you want.

&ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that
will give you bytes, etc.

     Andrew

Re: [PATCH V2 net-next 2/4] net: ena: ethtool: Add new device statistics

[Andrew Lunn]

> +	if (eni_stats_needed) {
> +		ena_update_hw_stats(adapter);
> +		for (i = 0; i < ENA_STATS_ARRAY_ENI(adapter); i++) {
> +			ena_stats = &ena_stats_eni_strings[i];
> +
> +			ptr = (u64 *)((unsigned long)&adapter->eni_stats +
> +				ena_stats->stat_offset);

Yet more ugly casts. Please fix this. If done correctly, you should
not need any casts at all.

    Andrew

RE: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Jubran, Samih]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, August 19, 2020 5:17 PM
> To: Jubran, Samih <sameehj@amazon.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David
> <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>;
> Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed
> <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori,
> Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>;
> Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel
> <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt,
> Benjamin <benh@amazon.com>; Kiyanovski, Arthur
> <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com>
> Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use
> unsigned long for pointer arithmetics
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote:
> > From: Sameeh Jubran <sameehj@amazon.com>
> >
> > unsigned long is the type for doing maths on pointers.
> 
> Maths on pointers is perfectly valid. The real issue here is you have all your
> types mixed up.

The stat_offset field has the bytes from the start of the struct, the math is perfectly valid IMO¸
I have also went for the extra step and tested it using prints.

> 
> > -                     ptr = (u64 *)((uintptr_t)&ring->tx_stats +
> > -                             (uintptr_t)ena_stats->stat_offset);
> > +                     ptr = (u64 *)((unsigned long)&ring->tx_stats +
> > +                             ena_stats->stat_offset);
> 
> struct ena_ring {
> ...
>         union {
>                 struct ena_stats_tx tx_stats;
>                 struct ena_stats_rx rx_stats;
>         };
> 
> struct ena_stats_tx {
>         u64 cnt;
>         u64 bytes;
>         u64 queue_stop;
>         u64 prepare_ctx_err;
>         u64 queue_wakeup;
>         ...
> }
> 
> &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that,
> adding 1 for example, takes you forward a full ena_stats_tx structure. Not
> what you want.
> 
> &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give
> you bytes, etc.


If I understand you well, the alternative approach you are suggesting is:

ptr = &ring->tx_stats.cnt + ena_stats->stat_offset;

of course we need to convert the stat_offset field to be in 8 bytes resolution instead.

This approach has a potential bug hidden in it. If in the future
someone decides to expand the "ena_stats_tx" struct and add a field preceding cnt,
cnt will no longer be the beginning of the struct, which will cause a bug."

Therefore, if you have another way to do this, please share it. Otherwise I'd
rather leave this code as it is for the sake of robustness.

> 
>      Andrew

RE: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Jubran, Samih]

> -----Original Message-----
> From: Jubran, Samih
> Sent: Thursday, August 20, 2020 3:13 PM
> To: 'Andrew Lunn' <andrew@lunn.ch>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David
> <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>;
> Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed
> <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori,
> Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>;
> Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel
> <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt,
> Benjamin <benh@amazon.com>; Kiyanovski, Arthur
> <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com>
> Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use
> unsigned long for pointer arithmetics
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, August 19, 2020 5:17 PM
> > To: Jubran, Samih <sameehj@amazon.com>
> > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David
> > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>;
> Matushevsky,
> > Alexander <matua@amazon.com>; Bshara, Saeed
> <saeedb@amazon.com>;
> > Wilson, Matt <msw@amazon.com>; Liguori, Anthony
> <aliguori@amazon.com>;
> > Bshara, Nafea <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>;
> > Belgazal, Netanel <netanel@amazon.com>; Saidi, Ali
> > <alisaidi@amazon.com>; Herrenschmidt, Benjamin <benh@amazon.com>;
> > Kiyanovski, Arthur <akiyano@amazon.com>; Dagan, Noam
> > <ndagan@amazon.com>
> > Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use
> > unsigned long for pointer arithmetics
> >
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> >
> >
> >
> > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote:
> > > From: Sameeh Jubran <sameehj@amazon.com>
> > >
> > > unsigned long is the type for doing maths on pointers.
> >
> > Maths on pointers is perfectly valid. The real issue here is you have
> > all your types mixed up.
> 
> The stat_offset field has the bytes from the start of the struct, the math is
> perfectly valid IMO¸ I have also went for the extra step and tested it using
> prints.
> 
> >
> > > -                     ptr = (u64 *)((uintptr_t)&ring->tx_stats +
> > > -                             (uintptr_t)ena_stats->stat_offset);
> > > +                     ptr = (u64 *)((unsigned long)&ring->tx_stats +
> > > +                             ena_stats->stat_offset);
> >
> > struct ena_ring {
> > ...
> >         union {
> >                 struct ena_stats_tx tx_stats;
> >                 struct ena_stats_rx rx_stats;
> >         };
> >
> > struct ena_stats_tx {
> >         u64 cnt;
> >         u64 bytes;
> >         u64 queue_stop;
> >         u64 prepare_ctx_err;
> >         u64 queue_wakeup;
> >         ...
> > }
> >
> > &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on
> > that, adding 1 for example, takes you forward a full ena_stats_tx
> > structure. Not what you want.
> >
> > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that
> > will give you bytes, etc.
> 
> 
> If I understand you well, the alternative approach you are suggesting is:
> 
> ptr = &ring->tx_stats.cnt + ena_stats->stat_offset;
> 
> of course we need to convert the stat_offset field to be in 8 bytes resolution
> instead.
> 
> This approach has a potential bug hidden in it. If in the future someone
> decides to expand the "ena_stats_tx" struct and add a field preceding cnt,
> cnt will no longer be the beginning of the struct, which will cause a bug."
> 
> Therefore, if you have another way to do this, please share it. Otherwise I'd
> rather leave this code as it is for the sake of robustness.
> 
> >
> >      Andrew


Ping.

Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Maciej Fijalkowski]

On Thu, Aug 20, 2020 at 12:13:15PM +0000, Jubran, Samih wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, August 19, 2020 5:17 PM
> > To: Jubran, Samih <sameehj@amazon.com>
> > Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David
> > <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>;
> > Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed
> > <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori,
> > Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>;
> > Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel
> > <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt,
> > Benjamin <benh@amazon.com>; Kiyanovski, Arthur
> > <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com>
> > Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use
> > unsigned long for pointer arithmetics
> > 
> > CAUTION: This email originated from outside of the organization. Do not click
> > links or open attachments unless you can confirm the sender and know the
> > content is safe.
> > 
> > 
> > 
> > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote:
> > > From: Sameeh Jubran <sameehj@amazon.com>
> > >
> > > unsigned long is the type for doing maths on pointers.
> > 
> > Maths on pointers is perfectly valid. The real issue here is you have all your
> > types mixed up.
> 
> The stat_offset field has the bytes from the start of the struct, the math is perfectly valid IMO¸
> I have also went for the extra step and tested it using prints.
> 
> > 
> > > -                     ptr = (u64 *)((uintptr_t)&ring->tx_stats +
> > > -                             (uintptr_t)ena_stats->stat_offset);
> > > +                     ptr = (u64 *)((unsigned long)&ring->tx_stats +
> > > +                             ena_stats->stat_offset);
> > 
> > struct ena_ring {
> > ...
> >         union {
> >                 struct ena_stats_tx tx_stats;
> >                 struct ena_stats_rx rx_stats;
> >         };
> > 
> > struct ena_stats_tx {
> >         u64 cnt;
> >         u64 bytes;
> >         u64 queue_stop;
> >         u64 prepare_ctx_err;
> >         u64 queue_wakeup;
> >         ...
> > }
> > 
> > &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that,
> > adding 1 for example, takes you forward a full ena_stats_tx structure. Not
> > what you want.
> > 
> > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give
> > you bytes, etc.
> 
> 
> If I understand you well, the alternative approach you are suggesting is:
> 
> ptr = &ring->tx_stats.cnt + ena_stats->stat_offset;

I don't want to stir up the pot, but do you really need the offsetof() of
each member in the stats struct? Couldn't you piggyback on assumption that
these stats need to be u64 and just walk the struct with pointer?

	struct ena_ring *ring;
	int offset;
	int i, j;
	u8 *ptr;

	for (i = 0; i < adapter->num_io_queues; i++) {
		/* Tx stats */
		ring = &adapter->tx_ring[i];
		ptr = (u8 *)&ring->tx_stats;

		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
			ena_safe_update_stat((u64 *)ptr, (*data)++, &ring->syncp);
			ptr += sizeof(u64);
		}
	}

I find this as a simpler and lighter solution. There might be issues with
code typed in email client, but you get the idea.

> 
> of course we need to convert the stat_offset field to be in 8 bytes resolution instead.
> 
> This approach has a potential bug hidden in it. If in the future
> someone decides to expand the "ena_stats_tx" struct and add a field preceding cnt,
> cnt will no longer be the beginning of the struct, which will cause a bug."
> 
> Therefore, if you have another way to do this, please share it. Otherwise I'd
> rather leave this code as it is for the sake of robustness.
> 
> > 
> >      Andrew

Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Shay Agroskin]

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

> On Thu, Aug 20, 2020 at 12:13:15PM +0000, Jubran, Samih wrote:
>> 
>> > ...
>> > 
>> > On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com 
>> > wrote:
>> > > From: Sameeh Jubran <sameehj@amazon.com>
>> > >
>> > > unsigned long is the type for doing maths on pointers.
>> > 
>> > Maths on pointers is perfectly valid. The real issue here is 
>> > you have all your
>> > types mixed up.
>> 
>> The stat_offset field has the bytes from the start of the 
>> struct, the math is perfectly valid IMO¸
>> I have also went for the extra step and tested it using prints.
>> 
>> > 
>> > > -                     ptr = (u64 
>> > > *)((uintptr_t)&ring->tx_stats +
>> > > - 
>> > > (uintptr_t)ena_stats->stat_offset);
>> > > +                     ptr = (u64 *)((unsigned 
>> > > long)&ring->tx_stats +
>> > > +                             ena_stats->stat_offset);
>> > 
>> > struct ena_ring {
>> > ...
>> >         union {
>> >                 struct ena_stats_tx tx_stats;
>> >                 struct ena_stats_rx rx_stats;
>> >         };
>> > 
>> > struct ena_stats_tx {
>> >         u64 cnt;
>> >         u64 bytes;
>> >         u64 queue_stop;
>> >         u64 prepare_ctx_err;
>> >         u64 queue_wakeup;
>> >         ...
>> > }
>> > 
>> > &ring->tx_stats will give you a struct 
>> > *ena_stats_tx. Arithmetic on that,
>> > adding 1 for example, takes you forward a full ena_stats_tx 
>> > structure. Not
>> > what you want.
>> > 
>> > &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 
>> > to that will give
>> > you bytes, etc.
>> 
>> 
>> If I understand you well, the alternative approach you are 
>> suggesting is:
>> 
>> ptr = &ring->tx_stats.cnt + ena_stats->stat_offset;
>
> I don't want to stir up the pot, but do you really need the 
> offsetof() of
> each member in the stats struct? Couldn't you piggyback on 
> assumption that
> these stats need to be u64 and just walk the struct with 
> pointer?
>
> 	struct ena_ring *ring;
> 	int offset;
> 	int i, j;
> 	u8 *ptr;
>
> 	for (i = 0; i < adapter->num_io_queues; i++) {
> 		/* Tx stats */
> 		ring = &adapter->tx_ring[i];
> 		ptr = (u8 *)&ring->tx_stats;
>
> 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
> 			ena_safe_update_stat((u64 *)ptr, 
> (*data)++, &ring->syncp);
> 			ptr += sizeof(u64);
> 		}
> 	}
>
> I find this as a simpler and lighter solution. There might be 
> issues with
> code typed in email client, but you get the idea.
>
>> 
>> of course we need to convert the stat_offset field to be in 8 
>> bytes resolution instead.
>> 
>> This approach has a potential bug hidden in it. If in the 
>> future
>> someone decides to expand the "ena_stats_tx" struct and add a 
>> field preceding cnt,
>> cnt will no longer be the beginning of the struct, which will 
>> cause a bug."
>> 
>> Therefore, if you have another way to do this, please share 
>> it. Otherwise I'd
>> rather leave this code as it is for the sake of robustness.
>> 
>> > 
>> >      Andrew

Hi all,

We tried to implement your suggestion, and found that removing the 
stat_offset
field causes problems that are challenging to solve.
Removing stat_offset introduces a requirement that the statistics 
in a stat
strings array (check [1] for example) and stat variables struct 
(check [2] for
example) must be in the same order.
This requirement is prone to future bugs that might be challenging 
to locate.
We also tried to unify the array and struct creation by
using X macros. At the moment this change requires more time and 
effort by us
and our customers need this code merged asap.

[1] https://elixir.bootlin.com/linux/v5.9-
rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71
[2] https://elixir.bootlin.com/linux/v5.9-
rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232

(This message was sent before but didn't seem to get into the 
mailing list. Apologies if you got it twice)

Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

[Jakub Kicinski]

On Sun, 6 Sep 2020 13:47:13 +0300 Shay Agroskin wrote:
> Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:
> > I don't want to stir up the pot, but do you really need the 
> > offsetof() of
> > each member in the stats struct? Couldn't you piggyback on 
> > assumption that
> > these stats need to be u64 and just walk the struct with 
> > pointer?
> >
> > 	struct ena_ring *ring;
> > 	int offset;
> > 	int i, j;
> > 	u8 *ptr;
> >
> > 	for (i = 0; i < adapter->num_io_queues; i++) {
> > 		/* Tx stats */
> > 		ring = &adapter->tx_ring[i];
> > 		ptr = (u8 *)&ring->tx_stats;
> >
> > 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
> > 			ena_safe_update_stat((u64 *)ptr, 
> > (*data)++, &ring->syncp);
> > 			ptr += sizeof(u64);
> > 		}
> > 	}
> >
> > I find this as a simpler and lighter solution. There might be 
> > issues with
> > code typed in email client, but you get the idea.
> >  
> >> 
> >> of course we need to convert the stat_offset field to be in 8 
> >> bytes resolution instead.
> >> 
> >> This approach has a potential bug hidden in it. If in the 
> >> future
> >> someone decides to expand the "ena_stats_tx" struct and add a 
> >> field preceding cnt,
> >> cnt will no longer be the beginning of the struct, which will 
> >> cause a bug."
> >> 
> >> Therefore, if you have another way to do this, please share 
> >> it. Otherwise I'd
> >> rather leave this code as it is for the sake of robustness.
> >>   
> >> > 
> >> >      Andrew  
> 
> Hi all,
> 
> We tried to implement your suggestion, and found that removing the 
> stat_offset
> field causes problems that are challenging to solve.
> Removing stat_offset introduces a requirement that the statistics 
> in a stat
> strings array (check [1] for example) and stat variables struct 
> (check [2] for
> example) must be in the same order.
> This requirement is prone to future bugs that might be challenging 
> to locate.
> We also tried to unify the array and struct creation by
> using X macros. At the moment this change requires more time and 
> effort by us
> and our customers need this code merged asap.
> 
> [1] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71
> [2] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232
> 
> (This message was sent before but didn't seem to get into the 
> mailing list. Apologies if you got it twice)

Divide the offset by 8, cast &ring->tx_stats to u64 * (without
referencing cnt). That should be fine.