From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [PATCH net-next] net: sched: change tcf_del_walker() to use concurrent-safe delete Date: Wed, 05 Sep 2018 10:03:03 +0300 Message-ID: References: <1535958361-6778-1-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Linux Kernel Network Developers , Jamal Hadi Salim , Jiri Pirko , David Miller To: Cong Wang Return-path: Received: from mail-eopbgr30043.outbound.protection.outlook.com ([40.107.3.43]:45705 "EHLO EUR03-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726242AbeIELhE (ORCPT ); Wed, 5 Sep 2018 07:37:04 -0400 In-reply-to: Sender: netdev-owner@vger.kernel.org List-ID: On Tue 04 Sep 2018 at 22:41, Cong Wang wrote: > On Mon, Sep 3, 2018 at 1:33 PM Vlad Buslov wrote: >> >> >> On Mon 03 Sep 2018 at 18:50, Cong Wang wrote: >> > On Mon, Sep 3, 2018 at 12:06 AM Vlad Buslov wrote: >> >> >> >> Action API was changed to work with actions and action_idr in concurrency >> >> safe manner, however tcf_del_walker() still uses actions without taking >> >> reference to them first and deletes them directly, disregarding possible >> >> concurrent delete. >> >> >> >> Change tcf_del_walker() to use tcf_idr_delete_index() that doesn't require >> >> caller to hold reference to action and accepts action id as argument, >> >> instead of direct action pointer. >> > >> > Hmm, why doesn't tcf_del_walker() just take idrinfo->lock? At least >> > tcf_dump_walker() already does. >> >> Because tcf_del_walker() calls __tcf_idr_release(), which take >> idrinfo->lock itself (deadlock). It also calls sleeping functions like > > Deadlock can be easily resolved by moving the lock out. > > >> tcf_action_goto_chain_fini(), so just implementing function that >> releases action without taking idrinfo->lock is not enough. > > Sleeping can be resolved either by making it atomic or > deferring it to a work queue. > > None of your arguments here is a blocker to locking > idrinfo->lock. You really should focus on if it is really > necessary to lock idrinfo->lock in tcf_del_walker(), rather > than these details. > > For me, if you need idrinfo->lock for dump walker, you must > need it for delete walker too, because deletion is a writer > which should require stronger protection than the dumper, > which merely a reader. I don't get how it is necessary. Dump walker uses pointers to actions directly, and in order to be concurrency-safe it must either hold the lock or obtain reference to action. Note that del walker doesn't use the action pointer, it only passed action id to tcf_idr_delete_index() function, which does all the necessary locking and can deal with potential concurrency issues (concurrent delete, etc.). This approach also benefits from code reuse from other code paths that delete actions, instead of implementing its own.