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?