Message-ID: <20181115102024.GA2253@nanopsycho.orion>
Date:   Thu, 15 Nov 2018 11:20:24 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH net-next 17/17] net: sched: unlock rules update API

Wed, Nov 14, 2018 at 05:45:34PM CET, vladbu@...lanox.com wrote:
>
>On Wed 14 Nov 2018 at 06:44, Jiri Pirko <jiri@...nulli.us> wrote:
>> Tue, Nov 13, 2018 at 02:46:54PM CET, vladbu@...lanox.com wrote:
>>>On Mon 12 Nov 2018 at 17:30, David Miller <davem@...emloft.net> wrote:
>>>> From: Vlad Buslov <vladbu@...lanox.com>
>>>> 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
>>>
>

Powered by blists - more mailing lists