Re: [PATCH] macvlan: lockless tx path

[Ben Greear <greearb@candelatech.com>]

On 11/10/2010 09:43 AM, Eric Dumazet wrote:
> Le mercredi 10 novembre 2010 à 09:39 -0800, Ben Greear a écrit :
>
>>>    /**
>>> - *	struct macvlan_rx_stats - MACVLAN percpu rx stats
>>> + *	struct macvlan_pcpu_stats - MACVLAN percpu stats
>>>     *	@rx_packets: number of received packets
>>>     *	@rx_bytes: number of received bytes
>>>     *	@rx_multicast: number of received multicast packets
>>> + *	@tx_
>>
>> Minor nit..seems you missed a few there?
>>
>
> Arg... you're right !
>
>>>     *	@syncp: synchronization point for 64bit counters
>>>     *	@rx_errors: number of errors
>>>     */
>>> -struct macvlan_rx_stats {
>>> +struct macvlan_pcpu_stats {
>>>    	u64			rx_packets;
>>>    	u64			rx_bytes;
>>>    	u64			rx_multicast;
>>> +	u64			tx_packets;
>>> +	u64			tx_bytes;
>>>    	struct u64_stats_sync	syncp;
>>>    	unsigned long		rx_errors;
>>> +	unsigned long		tx_dropped;
>>
>> Any reason to not also make those u64?
>>
>
> Well, they are supposed to be not incremented often, and they are packet
> counts only, so a wrap around in less than 5 seconds is very unlikely.

I agree, but if these can be read from user-space, it can be tricky to make
solid code to deal with wraps when the thing wrapping can be 32 or 64 bits,
depending on whether the kernel is compiled 32-bit or 64-bit.

So, my preference is to use u32 or u64 so there is no guesswork involved.

To be sure, this problem exists in lots of places already (/proc/net/dev comes to mind),
but the fewer places the better in my opinion.

That said, I don't feel too strongly about it, so if you want to keep these
stats as they are, I shall argue no more :)

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com