Wed, Nov 14, 2018 at 05:45:34PM CET, vla...@mellanox.com wrote: > >On Wed 14 Nov 2018 at 06:44, Jiri Pirko <j...@resnulli.us> wrote: >> Tue, Nov 13, 2018 at 02:46:54PM CET, vla...@mellanox.com wrote: >>>On Mon 12 Nov 2018 at 17:30, David Miller <da...@davemloft.net> wrote: >>>> From: Vlad Buslov <vla...@mellanox.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 >>> >
