From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next 17/17] net: sched: unlock rules update API Date: Thu, 15 Nov 2018 11:20:24 +0100 Message-ID: <20181115102024.GA2253@nanopsycho.orion> References: <1542009346-23780-1-git-send-email-vladbu@mellanox.com> <1542009346-23780-18-git-send-email-vladbu@mellanox.com> <20181112.093014.891880255643252936.davem@davemloft.net> <20181114064453.GA2235@nanopsycho.orion> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: David Miller , "netdev@vger.kernel.org" , "jhs@mojatatu.com" , "xiyou.wangcong@gmail.com" , "ast@kernel.org" , "daniel@iogearbox.net" To: Vlad Buslov Return-path: Received: from mail-wm1-f42.google.com ([209.85.128.42]:52714 "EHLO mail-wm1-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728609AbeKOUeQ (ORCPT ); Thu, 15 Nov 2018 15:34:16 -0500 Received: by mail-wm1-f42.google.com with SMTP id r11-v6so18239134wmb.2 for ; Thu, 15 Nov 2018 02:27:00 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Wed, Nov 14, 2018 at 05:45:34PM CET, vladbu@mellanox.com wrote: > >On Wed 14 Nov 2018 at 06:44, Jiri Pirko wrote: >> Tue, Nov 13, 2018 at 02:46:54PM CET, vladbu@mellanox.com wrote: >>>On Mon 12 Nov 2018 at 17:30, David Miller wrote: >>>> From: Vlad Buslov >>>> Date: Mon, 12 Nov 2018 09:55:46 +0200 >>>> >>>>> Register netlink protocol handlers for message types RTM_NEWTFILTER, >>>>> RTM_DELTFILTER, RTM_GETTFILTER as unlocked. Set rtnl_held variable that >>>>> tracks rtnl mutex state to be false by default. >>>> >>>> This whole conditional locking mechanism is really not clean and makes >>>> this code so much harder to understand and audit. >>>> >>>> Please improve the code so that this kind of construct is not needed. >>>> >>>> Thank you. >>> >>>Hi David, >>> >>>I considered several approaches to this problem and decided that this >>>one is most straightforward to implement. I understand your concern and >>>agree that this code is not easiest to understand and can suggest >>>several possible solutions that do not require this kind of elaborate >>>locking mechanism in cls API, but have their own drawbacks: >>> >>>1. Convert all qdiscs and classifiers to support unlocked execution, >>>like we did for actions. However, according to my experience with >>>converting flower classifier, these require much more code than actions. >>>I would estimate it to be more work than whole current unlocking effort >>>(hundred+ patches). Also, authors of some of them might be unhappy with >>>such intrusive changes. I don't think this approach is realistic. >>> >>>2. Somehow determine if rtnl is needed at the beginning of cls API rule >>>update functions. Currently, this is not possible because locking >>>requirements are determined by qdisc_class_ops and tcf_proto_ops 'flags' >>>field, which requires code to first do whole ops lookup sequence. >>>However, instead of class field I can put 'flags' in some kind of hash >>>table or array that will map qdisc/classifier type string to flags, so >>>it will be possible to determine locking requirements by just parsing >>>netlink message and obtaining flags by qdisc/classifier type. I do not >>>consider it pretty solution either, but maybe you have different >>>opinion. >> >> I think you will have to do 2. or some modification. Can't you just >> check for cls ability to run unlocked early on in tc_new_tfilter()? >> You would call tcf_proto_locking_check(nla_data(tca[TCA_KIND]), ...), >> which would do tcf_proto_lookup_ops() for ops and check the flags? > >I guess that would work. However, such solution requires calling >tcf_proto_lookup_ops(), which iterates over tcf_proto_base list and >calls strcmp() for each proto, for every rule update call. That is why I >suggested to use some kind of optimized data structure for that purpose >in my first reply. Dunno if such solution will significantly impact rule >update performance. We don't have that many classifiers and their names >are short, so I guess not? Let's do it like this for unlocked first, then we can optimize if necessary. > >> >> >>> >>>3. Anything you can suggest? I might be missing something simple that >>>you would consider more elegant solution to this problem. >>> >>>Thanks, >>>Vlad >>> >