netdev - Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair

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:   Tue, 18 Dec 2018 20:27:48 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Vlad Buslov <vladbu@...lanox.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH net-next v2 01/17] net: sched: refactor
 mini_qdisc_pair_swap() to use workqueue

On Mon, Dec 17, 2018 at 2:30 AM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Sun, Dec 16, 2018 at 07:52:18PM CET, xiyou.wangcong@...il.com wrote:
> >On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov <vladbu@...lanox.com> wrote:
> >>
> >> On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@...il.com> wrote:
> >> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@...lanox.com> wrote:
> >> >>
> >> >> As a part of the effort to remove dependency on rtnl lock, cls API is being
> >> >> converted to use fine-grained locking mechanisms instead of global rtnl
> >> >> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
> >> >> function and cannot be executed while holding a spinlock.
> >> >
> >> >
> >> > Why does it have to be a spinlock not a mutex?
> >> >
> >> > I've read your cover letter and this changelog, I don't find any
> >> > answer.
> >>
> >> My initial implementation used mutex. However, it was changed to
> >> spinlock by Jiri's request during internal review.
> >>
> >
> >So what's the answer to my question? :)
>
> Yeah, my concern agains mutexes was that it would be needed to have one
> per every block and per every chain. I find it quite heavy and I believe
> it is better to use spinlock in those cases. This patch is a side effect
> of that. Do you think it would be better to have mutexes instead of
> spinlocks?

My only concern with spinlock is we have to go async as we
can't block. This is almost always error-prone especially
when dealing with tcf block. I had to give up with spinlock
for idrinfo->lock, please take a look at:

commit 95278ddaa15cfa23e4a06ee9ed7b6ee0197c500b
Author: Cong Wang <xiyou.wangcong@...il.com>
Date:   Tue Oct 2 12:50:19 2018 -0700

    net_sched: convert idrinfo->lock from spinlock to a mutex


There are indeed some cases in kernel we do take multiple
mutex'es, for example,

/*
 * bd_mutex locking:
 *
 *  mutex_lock(part->bd_mutex)
 *    mutex_lock_nested(whole->bd_mutex, 1)
 */

So, how heavy are they comparing with spinlocks?

Thanks.