netdev - Re: [Patch net-next] mlx5: use RCU lock in mlx5_eq_cq

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:   Wed, 6 Feb 2019 17:37:36 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "eric.dumazet@...il.com" <eric.dumazet@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...lanox.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: Re: [Patch net-next] mlx5: use RCU lock in mlx5_eq_cq_get()

On Wed, 2019-02-06 at 09:17 -0800, Eric Dumazet wrote:
> 
> On 02/06/2019 08:55 AM, Saeed Mahameed wrote:
> > On Wed, 2019-02-06 at 12:02 +0000, Tariq Toukan wrote:
> > > On 2/6/2019 2:35 AM, Cong Wang wrote:
> > > > mlx5_eq_cq_get() is called in IRQ handler, the spinlock inside
> > > > gets a lot of contentions when we test some heavy workload
> > > > with 60 RX queues and 80 CPU's, and it is clearly shown in the
> > > > flame graph.
> > > > 
> > 
> > Hi Cong,
> > 
> > The patch is ok to me, but i really doubt that you can hit a
> > contention
> > on latest upstream driver, since we already have spinlock per EQ,
> > which
> > means spinlock per core,  each EQ (core) msix handler can only
> > access
> > one spinlock (its own), so I am surprised how you got the
> > contention,
> > Maybe you are not running on latest upstream driver ?
> > 
> > what is the workload ? 
> 
> Surprisingly (or not), atomic operations, even on _not_ contended
> cache lines can
> stall the cpu enough for perf tools to notice...
> 
> If the atomic operation can be trivially replaced by RCU, then do it
> by any mean.
> 
> 

Totally agree, Thanks Eric.