On Fri, May 12, 2017 at 7:13 PM, David Miller <da...@davemloft.net> wrote:
> From: Saeed Mahameed <sae...@mellanox.com>
> Date: Fri, 12 May 2017 14:56:45 +0300
>
>> From: Gal Pressman <g...@mellanox.com>
>>
>> Add a spinlock to prevent races when querying statistics, for example
>> querying counters in the middle of a non atomic memcpy() operation in
>> mlx5e_update_stats().
>>
>> This RW lock should be held when accessing priv->stats, to prevent other
>> reads/writes.
>>
>> Fixes: 9218b44dcc05 ("net/mlx5e: Statistics handling refactoring")
>> Signed-off-by: Gal Pressman <g...@mellanox.com>
>> Suggested-by: Eric Dumazet <eric.duma...@gmail.com>
>> Cc: kernel-t...@fb.com
>> Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
>
> This is overkill, and that rwlock is going to show up in perf for some
> workloads.
>
> Furthermore, two kzalloc()'s for a single state update operation?
> That's not reasonable either.
>Hi Dave, Well, the idea of the patch is to minimize the stats update to a single safe copy operation under stats_lock for that we need a temp buffer to store stats FW commands output, and then safely -under stats_lock- copy it to the buffer ethtool is going to report. I agree, it is really ridiculous that we allocate/free a couple of buffers on each update_stats operations, regardless of this patch. Is it ok if we use a temp buffer under netdev_priv for such usages or even use kmemcache ? > Use a seqlock, which is the primitive for handling this kind of > situation cheaply, and adds no atomics to the read path. > Will change this. Thanks for the tip. I will drop this patch for now and I will resend it later once Gal addresses all of the above comments. Thanks, Saeed.
