Message-ID: <vbf4lcjwqtx.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Date:   Wed, 14 Nov 2018 16:45:34 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Jiri Pirko <jiri@...nulli.us>
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


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?

>
>
>>
>>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