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: Wed, 14 Nov 2018 07:44:53 +0100 Message-ID: <20181114064453.GA2235@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> 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-f65.google.com ([209.85.128.65]:53768 "EHLO mail-wm1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727375AbeKNQx1 (ORCPT ); Wed, 14 Nov 2018 11:53:27 -0500 Received: by mail-wm1-f65.google.com with SMTP id f10-v6so14237078wme.3 for ; Tue, 13 Nov 2018 22:51:32 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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? > >3. Anything you can suggest? I might be missing something simple that >you would consider more elegant solution to this problem. > >Thanks, >Vlad >