netdev - Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 26 Aug 2016 12:16:15 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Hadar Hen Zion <hadarh@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>, Jiri Benc <jbenc@...hat.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Tom Herbert <tom@...bertland.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Amir Vadai <amirva@...lanox.com>, Amir Vadai <amir@...ai.me>
Subject: Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
> On Thu, Aug 25, 2016 at 10:48 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> >
> > Please find a better way than using a spinlock in this hot path.
> >
> > Maybe looking at
> > 2ee22a90c7afac265bb6f7abea610b938195e2b8 net_sched: act_mirred: remove spinlock in fast path
> > 56e5d1ca183d8616fab377d7d466c244b4dbb3b9 net_sched: act_gact: remove spinlock in fast path
> 
> This is not necessary at the moment, because:
> 
> 1) Currently there are only a few actions using lockless, and they are
> questionable, as we already discussed before, there could be some
> race condition when you modify an existing action.

There is no fundamental issue with a race condition.

Sure, there are races, but they have no serious effect.

Feel free to send a fix if you really have time to spare.

> 
> 2) We need to change the tc action API in order to fully support RCU,
> which is what I have been working on these days. I should come up
> with something next Monday (if not this weekend).
> 
> So for this patchset, using spinlock is fine, just as many other actions.
> I will take care of it later.

This is _not_ fine.

We are in 2016, not in 1995 anymore.

We are not adding a spinlock in a hot path unless absolutely needed.

With multi queue NIC, this spinlock is going to hurt performance so much
that this action wont be used by any serious user.

Here, it is absolutely trivial to use RCU and/or percpu counters.