On 5/2/2024 10:31 PM, Stephen Hemminger wrote:
> The names of Linux network devices are IFNAMSIZ(16) not the
> same as DPDK which has up to 64 characters. Don't need to
> hold onto the whole ifreq to save the remote interface flags.
>
> Make sure packet and byte counters are read once, so that global
> and per-queue values add up. No need for separate rx_nombuf counter
> since there is an alloc_failed value in ethdev.
>
> Keep only the statistics that are used. I.e no ipackets on
> tx queues etc.
>
This patch does multiple things, although each not very complex, I think
splitting the patch makes it simpler to review.
> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 138 ++++++++++++++++++----------------
> drivers/net/tap/rte_eth_tap.h | 22 +++---
> 2 files changed, 83 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index d847565073..3614aaf1dc 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -46,6 +46,11 @@
> #include <tap_netlink.h>
> #include <tap_tcmsgs.h>
>
> +/* Used to snapshot statistics */
> +#ifndef READ_ONCE
> +#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
> +#endif
> +
>
Why 'READ_ONCE' is required?
As it is a function that provides pointer as parameter, won't stat
values will be read from memory anyway?
<...>
> @@ -748,9 +754,8 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> }
> }
>
> - txq->stats.opackets += num_packets;
> - txq->stats.errs += nb_pkts - num_tx;
>
As I commented before, we can't just drop this.
Please check code in the function that has comment:
"/* stats.errs will be incremented */"
That code relies on this update, if we remove here something should
replace it.
<...>
> struct pmd_internals {
> struct rte_eth_dev *dev; /* Ethernet device. */
> - char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
> - char name[RTE_ETH_NAME_MAX_LEN]; /* Internal Tap device name */
> + char remote_iface[IFNAMSIZ]; /* Remote netdevice name */
> + char name[IFNAMSIZ]; /* Internal Tap device name */
> int type; /* Type field - TUN|TAP */
> int persist; /* 1 if keep link up, else 0 */
> struct rte_ether_addr eth_addr; /* Mac address of the device port */
> - struct ifreq remote_initial_flags;/* Remote netdevice flags on init */
> + uint16_t remote_flags; /* Remote netdevice flags on init */
> int remote_if_index; /* remote netdevice IF_INDEX */
> int if_index; /* IF_INDEX for the port */
> int ioctl_sock; /* socket for ioctl calls */
> + int ka_fd; /* keep-alive file descriptor */
>
> #ifdef HAVE_TCA_FLOWER
> int nlsk_fd; /* Netlink socket fd */
> @@ -88,12 +85,11 @@ struct pmd_internals {
> /* implicit rte_flow rules set when a remote device is active */
> LIST_HEAD(tap_implicit_flows, rte_flow) implicit_flows;
> #endif
> + struct rte_intr_handle *intr_handle; /* LSC interrupt handle. */
> + struct rte_mempool *gso_ctx_mp; /* Mempool for GSO packets */
>
> struct rx_queue rxq[RTE_PMD_TAP_MAX_QUEUES]; /* List of RX queues */
> struct tx_queue txq[RTE_PMD_TAP_MAX_QUEUES]; /* List of TX queues */
> - struct rte_intr_handle *intr_handle; /* LSC interrupt handle. */
> - int ka_fd; /* keep-alive file descriptor */
> - struct rte_mempool *gso_ctx_mp; /* Mempool for GSO packets */
> };
>
>
Are the order of fields changed intentionally, for a specific reason?
