| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1330875881.2730.15.camel@edumazet-laptop>
Date: Sun, 04 Mar 2012 07:44:41 -0800
From: Eric Dumazet <eric.dumazet@...il.com>
To: Junchang Wang <junchangwang@...il.com>
Cc: romieu@...zoreil.com, davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] r8169: Add 64bit statistics
Le dimanche 04 mars 2012 à 17:37 +0800, Junchang Wang a écrit :
> I submitted this two months ago. This is a resubmission. Thanks.
>
> Switch to use ndo_get_stats64 to get 64bit statistics.
> Two sync entries are used (one for Rx and one for Tx).
>
> Signed-off-by: Junchang Wang <junchangwang@...il.com>
> ---
> drivers/net/ethernet/realtek/r8169.c | 61 +++++++++++++++++++++++++++++-----
> 1 files changed, 52 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index fbd855b..c1e2421 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -675,6 +675,12 @@ enum rtl_flag {
> RTL_FLAG_MAX
> };
>
> +struct rtl8169_stats {
> + struct u64_stats_sync syncp;
small point : Using this means you have a 32bit hole here (on 32bit
build). Its minor, you dont need to change.
> + u64 packets;
> + u64 bytes;
> +};
> +
> struct rtl8169_private {
> void __iomem *mmio_addr; /* memory map physical address */
> struct pci_dev *pci_dev;
> @@ -689,6 +695,8 @@ struct rtl8169_private {
> u32 dirty_tx;
> struct TxDesc *TxDescArray; /* 256-aligned Tx descriptor ring */
> struct RxDesc *RxDescArray; /* 256-aligned Rx descriptor ring */
> + struct rtl8169_stats rx_stats;
> + struct rtl8169_stats tx_stats;
You could try to put these somewhere else, to try to keep this portion
as read only memory, to be more SMP friendly.
(Some loaded server could have one CPU serving RX stuff, and other cpus
doing TX stuff)
> dma_addr_t TxPhyAddr;
> dma_addr_t RxPhyAddr;
> void *Rx_databuff[NUM_RX_DESC]; /* Rx data buffers */
> @@ -775,7 +783,9 @@ static void rtl_hw_start(struct net_device *dev);
> static int rtl8169_close(struct net_device *dev);
> static void rtl_set_rx_mode(struct net_device *dev);
> static void rtl8169_tx_timeout(struct net_device *dev);
> -static struct net_device_stats *rtl8169_get_stats(struct net_device *dev);
> +static struct rtnl_link_stats64 *rtl8169_get_stats64(struct net_device *dev,
> + struct rtnl_link_stats64
> + *stats);
> static int rtl8169_change_mtu(struct net_device *dev, int new_mtu);
> static void rtl8169_rx_clear(struct rtl8169_private *tp);
> static int rtl8169_poll(struct napi_struct *napi, int budget);
> @@ -3521,7 +3531,7 @@ static void rtl_disable_msi(struct pci_dev *pdev, struct rtl8169_private *tp)
> static const struct net_device_ops rtl8169_netdev_ops = {
> .ndo_open = rtl8169_open,
> .ndo_stop = rtl8169_close,
> - .ndo_get_stats = rtl8169_get_stats,
> + .ndo_get_stats64 = rtl8169_get_stats64,
> .ndo_start_xmit = rtl8169_start_xmit,
> .ndo_tx_timeout = rtl8169_tx_timeout,
> .ndo_validate_addr = eth_validate_addr,
> @@ -5658,8 +5668,10 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
> rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
> tp->TxDescArray + entry);
> if (status & LastFrag) {
> - dev->stats.tx_packets++;
> - dev->stats.tx_bytes += tx_skb->skb->len;
> + u64_stats_update_begin(&tp->tx_stats.syncp);
> + tp->tx_stats.packets++;
> + tp->tx_stats.bytes += tx_skb->skb->len;
> + u64_stats_update_end(&tp->tx_stats.syncp);
> dev_kfree_skb(tx_skb->skb);
> tx_skb->skb = NULL;
> }
> @@ -5807,8 +5819,10 @@ process_pkt:
>
> napi_gro_receive(&tp->napi, skb);
>
> - dev->stats.rx_bytes += pkt_size;
> - dev->stats.rx_packets++;
> + u64_stats_update_begin(&tp->rx_stats.syncp);
> + tp->rx_stats.packets++;
> + tp->rx_stats.bytes += pkt_size;
> + u64_stats_update_end(&tp->rx_stats.syncp);
> }
>
> /* Work around for AMD plateform. */
> @@ -6070,20 +6084,49 @@ static void rtl_set_rx_mode(struct net_device *dev)
> }
>
> /**
> - * rtl8169_get_stats - Get rtl8169 read/write statistics
> + * rtl8169_get_stats64 - Get rtl8169 read/write statistics
> * @dev: The Ethernet Device to get statistics for
missing @stats
> *
> * Get TX/RX statistics for rtl8169
> */
> -static struct net_device_stats *rtl8169_get_stats(struct net_device *dev)
> +static struct rtnl_link_stats64 *
> +rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> {
> struct rtl8169_private *tp = netdev_priv(dev);
> void __iomem *ioaddr = tp->mmio_addr;
> + u64 _packets, _bytes;
> + unsigned int start;
>
> if (netif_running(dev))
> rtl8169_rx_missed(dev, ioaddr);
>
> - return &dev->stats;
> + do {
> + start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
> + _packets = tp->rx_stats.packets;
> + _bytes = tp->rx_stats.bytes;
> + } while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));
> +
> + stats->rx_packets = _packets;
> + stats->rx_bytes = _bytes;
You dont need _bytes and _packets temp variables, as @stats points to a
private memory, we can use it as working storage, just do :
do {
start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
stats->rx_packets = tp->rx_stats.packets;
stats->rx_bytes = tp->rx_stats.bytes;
} while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));
> +
> + do {
> + start = u64_stats_fetch_begin_bh(&tp->tx_stats.syncp);
> + _packets = tp->tx_stats.packets;
> + _bytes = tp->tx_stats.bytes;
> + } while (u64_stats_fetch_retry_bh(&tp->tx_stats.syncp, start));
same here.
> +
> + stats->tx_packets = _packets;
> + stats->tx_bytes = _bytes;
> +
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists