Re: [PATCH 02/33] netdev: introduce dev_get_stats()

[Eric Dumazet <dada1@cosmosbay.com>]

David Miller a écrit :
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Mon, 17 Nov 2008 15:42:09 -0800
> 
>> In order for the network device ops get_stats call to be immutable, the handling
>> of the default internal network device stats block has to be changed. Add a new
>> helper function which replaces the old use of internal_get_stats.
>>
>> Note: change return code to make it clear that the caller should not
>> go changing the returned statistics.
>>
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Applied.

I have one comment about netdev stats

on 32bit arches, SMP :

struct net_device = 0x480
offsetof(struct net_device, features)=0x44
offsetof(struct net_device, stats)=0x50
offsetof(struct net_device, stats.rx_packets)=0x50

So we trash features field, thats a problem...

I wonder if we could zap stats from netdev structure.
Some drivers already use external stats handling (like loopback)

get_dev_stats() would accept a second parameter : a pointer to a struct net_device_stats
provided by the reader, that the driver could use as a working zone, or not.

static struct net_device_stats *bond_get_stats(struct net_device *bond_dev,
	struct net_device_stats *temp)
{
        struct bonding *bond = netdev_priv(bond_dev);
        struct net_device_stats wrk;
        struct slave *slave;
        int i;

	/* use the caller provided zone */
        memset(temp, 0, sizeof(struct net_device_stats));

        read_lock_bh(&bond->lock);

        bond_for_each_slave(bond, slave, i) {
                const struct net_device_stats *sstats = dev_get_stats(slave->dev, &wrk);

                temp->rx_packets += sstats->rx_packets;
                temp->rx_bytes += sstats->rx_bytes;
                temp->rx_errors += sstats->rx_errors;
		...
	}


        read_unlock_bh(&bond->lock);

        return temp;
}

....

static struct net_device_stats *loopback_get_stats(struct net_device *dev,
			struct net_device_stats *temp)
{
        const struct pcpu_lstats *pcpu_lstats;
        unsigned long bytes = 0;
        unsigned long packets = 0;
        int i;

	memset(temp, 0, sizeof(*temp);
        pcpu_lstats = dev->ml_priv;
        for_each_possible_cpu(i) {
                const struct pcpu_lstats *lb_stats;

                lb_stats = per_cpu_ptr(pcpu_lstats, i);
                bytes   += lb_stats->bytes;
                packets += lb_stats->packets;
        }
        temp->rx_packets = packets;
        temp->tx_packets = packets;
        temp->rx_bytes = bytes;
        temp->tx_bytes = bytes;
        return temp;
}

...