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?

Reply via email to