From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [PATCH net-next v6 06/11] net: sched: add 'delete' function to action ops Date: Fri, 10 Aug 2018 12:41:18 +0300 Message-ID: References: <1530800673-12280-1-git-send-email-vladbu@mellanox.com> <1530800673-12280-7-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Linux Kernel Network Developers , David Miller , Jamal Hadi Salim , Jiri Pirko , Alexei Starovoitov , Daniel Borkmann , Yevgeny Kliteynik , Jiri Pirko To: Cong Wang Return-path: Received: from mail-eopbgr70040.outbound.protection.outlook.com ([40.107.7.40]:8283 "EHLO EUR04-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727117AbeHJMMt (ORCPT ); Fri, 10 Aug 2018 08:12:49 -0400 In-reply-to: Sender: netdev-owner@vger.kernel.org List-ID: On Thu 09 Aug 2018 at 19:38, Cong Wang wrote: > On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov wrote: >> >> Extend action ops with 'delete' function. Each action type to implements >> its own delete function that doesn't depend on rtnl lock. >> >> Implement delete function that is required to delete actions without >> holding rtnl lock. Use action API function that atomically deletes action >> only if it is still in action idr. This implementation prevents concurrent >> threads from deleting same action twice. > > I fail to understand why you introduce ops->delete(), it seems all > you want is getting the tn->idrinfo, but you already have tc_action > before calling ops->delete(), and tc_action has ->idrinfo... > > Each type of action does the same too, that is, just calling > tcf_idr_delete_index()... I agree with your assessment. Should have implemented it by just calling tcf_idr_delete_index() directly. > > This changelog sucks again, it claims for skipping rtnl lock, > but you can skip rtnl lock by just calling tcf_idr_delete_index() > directly too, it is not the reason for adding ops->delete(). My intention was to implement some generic and parallel-safe API that could be used to implement delete for all actions. It turns out that, as you noted, just calling tcf_idr_delete_index() is enough because any special per-action delete code is already implemented by ops->cleanup().