netdev - Re: [net 1/6] net/mlx5e: Use a spinlock to synchronize statistics

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 14 May 2017 11:52:13 +0300
From:   Saeed Mahameed <saeedm@....mellanox.co.il>
To:     David Miller <davem@...emloft.net>
Cc:     Saeed Mahameed <saeedm@...lanox.com>,
        Linux Netdev List <netdev@...r.kernel.org>,
        Gal Pressman <galp@...lanox.com>,
        Kernel Team <kernel-team@...com>
Subject: Re: [net 1/6] net/mlx5e: Use a spinlock to synchronize statistics

On Fri, May 12, 2017 at 7:13 PM, David Miller <davem@...emloft.net> wrote:
> From: Saeed Mahameed <saeedm@...lanox.com>
> Date: Fri, 12 May 2017 14:56:45 +0300
>
>> From: Gal Pressman <galp@...lanox.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 <galp@...lanox.com>
>> Suggested-by: Eric Dumazet <eric.dumazet@...il.com>
>> Cc: kernel-team@...com
>> Signed-off-by: Saeed Mahameed <saeedm@...lanox.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.