Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
References: <20180523073239.GC3155@nanopsycho> <1527065574-11299-1-git-send-email-vladbu@mellanox.com> <CAM_iQpWNRRND4+uav4Sqwe1ctsb_J61eueShaSkRmYeW1=KMJw@mail.gmail.com>
User-agent: mu4e 0.9.16; emacs 25.3.50.2
From:   Vlad Buslov <vladbu@mellanox.com>
To:     Cong Wang <xiyou.wangcong@gmail.com>
Cc:     Jiri Pirko <jiri@resnulli.us>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        David Miller <davem@davemloft.net>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next v3] net: sched: don't disable bh when accessing action idr
In-reply-to: <CAM_iQpWNRRND4+uav4Sqwe1ctsb_J61eueShaSkRmYeW1=KMJw@mail.gmail.com>
Date:   Thu, 24 May 2018 19:54:26 +0300
Message-ID: <vbfsh6hhu4d.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
MIME-Version: 1.0
Content-Type: text/plain
Received-SPF: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2018 16:54:32.5842 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 244cd951-c3a5-4568-7a24-08d5c19706a7
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR05MB4696
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On Wed 23 May 2018 at 23:14, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, May 23, 2018 at 1:52 AM, Vlad Buslov <vladbu@mellanox.com> wrote:
>> Initial net_device implementation used ingress_lock spinlock to synchronize
>> ingress path of device. This lock was used in both process and bh context.
>> In some code paths action map lock was obtained while holding ingress_lock.
>> Commit e1e992e52faa ("[NET_SCHED] protect action config/dump from irqs")
>> modified actions to always disable bh, while using action map lock, in
>> order to prevent deadlock on ingress_lock in softirq. This lock was removed
>> in commit 555353cfa1ae ("netdev: The ingress_lock member is no longer
>> needed.").
>>
>> Another reason to disable bh was filters delete code, that released actions
>> in rcu callback. This code was changed to release actions from workqueue
>> context in patch set "net_sched: close the race between call_rcu() and
>> cleanup_net()".
>>
>> With these changes it is no longer necessary to continue disable bh while
>> accessing action map.
>>
>> Replace all action idr spinlock usage with regular calls that do not
>> disable bh.
>
> Looks much better now!
>
> I _guess_ we perhaps can even get rid of this spinlock since most of
> the callers hold RTNL lock, not sure about the dump() path where
> RTNL might be removed recently.

Actually, this change is a result of discussion in code review of my
patch set that removes RTNL dependency from TC rules update path.

>
> Anyway,
>
> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Thank you for reviewing my code!