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? > >3. Anything you can suggest? I might be missing something simple that >you would consider more elegant solution to this problem. > >Thanks, >Vlad >
