From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3578AC43381 for ; Fri, 15 Feb 2019 11:22:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D2A0921900 for ; Fri, 15 Feb 2019 11:22:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="f8oPppbs" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388246AbfBOLWx (ORCPT ); Fri, 15 Feb 2019 06:22:53 -0500 Received: from mail-eopbgr40071.outbound.protection.outlook.com ([40.107.4.71]:29383 "EHLO EUR03-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726321AbfBOLWx (ORCPT ); Fri, 15 Feb 2019 06:22:53 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=sSHyzZvjW30VgPWlo5MADjdxblk4gCvavae1FwD6Guk=; b=f8oPppbsUhD685FID4qlTEc4L2vsU4utEhtM20TqFV2ylqgqtss8ixDnr8N1U1dH66nT9ONj3zrrAXy2pQKz1IzBDizodC2sJgCWb7pHbgkvc9G4dPCNkx3yDjaXuZDsjaSLbh/PjR3fDtykRR5WdHiLEKHejIuE27/SocPmaBE= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB3115.eurprd05.prod.outlook.com (10.175.29.149) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1622.16; Fri, 15 Feb 2019 11:22:45 +0000 Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::4041:bb68:2af3:eab8]) by HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::4041:bb68:2af3:eab8%5]) with mapi id 15.20.1622.018; Fri, 15 Feb 2019 11:22:45 +0000 From: Vlad Buslov To: Stefano Brivio CC: "netdev@vger.kernel.org" , "jhs@mojatatu.com" , "xiyou.wangcong@gmail.com" , "jiri@resnulli.us" , "davem@davemloft.net" Subject: Re: [PATCH net-next 03/12] net: sched: flower: introduce reference counting for filters Thread-Topic: [PATCH net-next 03/12] net: sched: flower: introduce reference counting for filters Thread-Index: AQHUxDmfZE1aHr/BOEiTBA49yHLJeaXfwOOAgAD4MIA= Date: Fri, 15 Feb 2019 11:22:45 +0000 Message-ID: References: <20190214074712.17846-1-vladbu@mellanox.com> <20190214074712.17846-4-vladbu@mellanox.com> <20190214213423.2260f5b9@redhat.com> In-Reply-To: <20190214213423.2260f5b9@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0188.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a::32) To HE1PR0502MB3641.eurprd05.prod.outlook.com (2603:10a6:7:85::11) authentication-results: spf=none (sender IP is ) smtp.mailfrom=vladbu@mellanox.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [37.142.13.130] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1f30b0a1-c0c3-4904-cb96-08d69337e878 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600110)(711020)(4605077)(4618075)(2017052603328)(7153060)(7193020);SRVR:HE1PR0502MB3115; x-ms-traffictypediagnostic: HE1PR0502MB3115: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB3115;23:fggUCG23tx7+vEH/zYqxIJRnSG5s38XpFJS7t?= =?iso-8859-1?Q?hcUme+dmn3e6d5q7LMgl3/0SbPsHSz0zVdDGi1HX10lZtXpAEAl/etAXJ0?= =?iso-8859-1?Q?jNnDpqtEV9wLehBjd+cWHo4HxtH2dSGX2iVH2MSq+Fv2ROyOmxG+pQh3On?= =?iso-8859-1?Q?F2m8oZURYaak3H57A5nUTKQOS/Xz1bXTWVOxAgjcXYcZ63U24Xh35Wdyul?= =?iso-8859-1?Q?bqPoaHd8QlfKfWQiHuusjuKX8ej/Kpp4AceHTqG5MsHCrK/LSokA8T9JnT?= =?iso-8859-1?Q?vk7V0wqeBSUZIqgkZcc4EJe/Z5SayJ9P9tbLEF949Iuon4OVAz/ogmspDD?= =?iso-8859-1?Q?SLtel05JLWe2KcDBLBJAJIZRu5ACMjjAs32sh+87ETwNn5zDT1kZ9ACgxY?= =?iso-8859-1?Q?2T3+ZdI31RkrphUFj/EV08o4r7vHTNbRB+817KT4TGt85Gk2lsvde+IaMK?= =?iso-8859-1?Q?50IJnD/yFGQVWrIHZSZBDRfTEzd98GaxHnLdcUAkWhdeKJn9EhJM9LOsQU?= =?iso-8859-1?Q?IEvcy1iZPRbRHojhjsreT/Y+AsxOwxdtzbDZQ7tnbSA6Nj3KnCsx1H7lAL?= =?iso-8859-1?Q?2yfUd2OBLWnlP8N0+E26NvtzGXicuUka1Zb9hvdpGo+7T7WiXKEABXU+Q4?= =?iso-8859-1?Q?4StgWnkVIU9ntVrpNrB7p0VTt7G72nZ7zmxYJsFsKDquUi21oxayhhqkLB?= =?iso-8859-1?Q?E8b8uL9ZDJYrL/Hq9/lPmm51fYcSLtc6Lyx99/r+BZDhyAL5EVLSOuJ5c8?= =?iso-8859-1?Q?sXiJGwzwE77qqpQu7VGRtRRCDm0GLNvPAWcnZCpkIU6vL6lboqzXXAsQ7j?= =?iso-8859-1?Q?7KdSxyFOyLmV3hzNJ/QXbolAxloKL1zsJibwEIqJkgzV7r3HSfZzvDeLBB?= =?iso-8859-1?Q?S8jc1YYDg5tA2i4GOQ7hWqy27kZuK9dQsdsa0hckEerezFWA6PKxKfgOlp?= =?iso-8859-1?Q?zvJAQ9ZbjEG/lLWKNYwqDav69NRDPGxyLl0CjpseBVm34C7/2eNq33yOlr?= =?iso-8859-1?Q?y9FrLzHbtzhTUnGT69qxo2qBDz+sUAVVPukYrEwKek+OVzfbf4NMqHd6Tn?= =?iso-8859-1?Q?FOfEqZ1npCM14Q7xz9D48bxu8qv4+N5vYGUQW+OkH9to3qzij+C2Pt/bbj?= =?iso-8859-1?Q?nMWs/M0frqk6NjArtR2/mVkD9YneLxx3QB25rBogPqXwP4R2dJpbSJy/G2?= =?iso-8859-1?Q?s+tzqx1uUlvEVRuq8ErwNnjAjhSsT1HRjzte/JuWD5mFnTIKdjSoviRaJ3?= =?iso-8859-1?Q?j4AL7s8LSMuVTYHByg4?= x-microsoft-antispam-prvs: x-forefront-prvs: 09497C15EB x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(376002)(39860400002)(136003)(366004)(396003)(346002)(189003)(199004)(8936002)(86362001)(6506007)(99286004)(81166006)(6436002)(81156014)(8676002)(229853002)(14454004)(2616005)(256004)(14444005)(186003)(486006)(386003)(478600001)(476003)(6246003)(26005)(11346002)(6486002)(54906003)(76176011)(66066001)(316002)(305945005)(7736002)(4326008)(25786009)(6916009)(71200400001)(3846002)(446003)(6116002)(97736004)(53936002)(102836004)(71190400001)(2906002)(6512007)(36756003)(52116002)(68736007)(106356001)(105586002);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB3115;H:HE1PR0502MB3641.eurprd05.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: YujoEx7LD3SMfJMtFHSoRbeSZs/DSiyT+Xi9iWpzpGsMuymeLYsn7H57W3r48JwY0OAdmD9B3iy1C7KgSHLhxGZY3o0Ra6QhVT5m6TZcs9AzrgVZWi8wcdrcaSFQO3XKNFuasZQeedoCt3z+olNLISVY/PEBSEZoTewGXPjTqxHNPJvjPThz87VIJpwx2JJtnAcubFgKelmMz/JqOFRr7cbJMu2vKpNX4zJYvHSBOdZIktSIl+i4oPlte0ZNauWwUxvcb/lxP4AfUZCAqJSKo/FwkPUXsyVP8tJow0DAUVEIkq2MnxKktyw0CNtLELgTwMHkN7CCzB0TS2+9g04G+TrH92pf3I0jAOnXjDCr/MR1IKzC0ZbxAuKsJUVvfOYOanHF0+Ft4g1pmID3Sh3C5lUnCbocvNjgiIzwrhPnFsY= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1f30b0a1-c0c3-4904-cb96-08d69337e878 X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2019 11:22:44.3302 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0502MB3115 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu 14 Feb 2019 at 20:34, Stefano Brivio wrote: > On Thu, 14 Feb 2019 09:47:03 +0200 > Vlad Buslov wrote: > >> +static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp, >> + unsigned long *handle) >> +{ >> + struct cls_fl_head *head =3D fl_head_dereference(tp); >> + struct cls_fl_filter *f; >> + >> + rcu_read_lock(); >> + /* don't return filters that are being deleted */ >> + while ((f =3D idr_get_next_ul(&head->handle_idr, >> + handle)) !=3D NULL && >> + !refcount_inc_not_zero(&f->refcnt)) >> + ++(*handle); > > This... hurts :) What about: > > while ((f =3D idr_get_next_ul(&head->handle_idr, &handle))) { > if (refcount_inc_not_zero(&f->refcnt)) > break; > ++(*handle); > } > > ? I prefer to avoid using value of assignment as boolean and non-structured jumps, when possible. In this case it seems OK either way, but how about: for (f =3D idr_get_next_ul(&head->handle_idr, handle); f && !refcount_inc_not_zero(&f->refcnt); f =3D idr_get_next_ul(&head->handle_idr, handle)) ++(*handle); > >> + rcu_read_unlock(); >> + >> + return f; >> +} >> + >> static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f, >> struct netlink_ext_ack *extack) >> { >> @@ -456,10 +503,7 @@ static bool __fl_delete(struct tcf_proto *tp, struc= t cls_fl_filter *f, >> if (!tc_skip_hw(f->flags)) >> fl_hw_destroy_filter(tp, f, extack); >> tcf_unbind_filter(tp, &f->res); >> - if (async) >> - tcf_queue_work(&f->rwork, fl_destroy_filter_work); >> - else >> - __fl_destroy_filter(f); >> + __fl_put(f); >> >> return last; >> } >> @@ -494,11 +538,18 @@ static void fl_destroy(struct tcf_proto *tp, bool = rtnl_held, >> tcf_queue_work(&head->rwork, fl_destroy_sleepable); >> } >> >> +static void fl_put(struct tcf_proto *tp, void *arg) >> +{ >> + struct cls_fl_filter *f =3D arg; >> + >> + __fl_put(f); >> +} >> + >> static void *fl_get(struct tcf_proto *tp, u32 handle) >> { >> struct cls_fl_head *head =3D fl_head_dereference(tp); >> >> - return idr_find(&head->handle_idr, handle); >> + return __fl_get(head, handle); >> } >> >> static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] =3D { >> @@ -1321,12 +1372,16 @@ static int fl_change(struct net *net, struct sk_= buff *in_skb, >> struct nlattr **tb; >> int err; >> >> - if (!tca[TCA_OPTIONS]) >> - return -EINVAL; >> + if (!tca[TCA_OPTIONS]) { >> + err =3D -EINVAL; >> + goto errout_fold; >> + } >> >> mask =3D kzalloc(sizeof(struct fl_flow_mask), GFP_KERNEL); >> - if (!mask) >> - return -ENOBUFS; >> + if (!mask) { >> + err =3D -ENOBUFS; >> + goto errout_fold; >> + } >> >> tb =3D kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL= ); >> if (!tb) { >> @@ -1349,6 +1404,7 @@ static int fl_change(struct net *net, struct sk_bu= ff *in_skb, >> err =3D -ENOBUFS; >> goto errout_tb; >> } >> + refcount_set(&fnew->refcnt, 1); >> >> err =3D tcf_exts_init(&fnew->exts, TCA_FLOWER_ACT, 0); >> if (err < 0) >> @@ -1381,6 +1437,7 @@ static int fl_change(struct net *net, struct sk_bu= ff *in_skb, >> if (!tc_in_hw(fnew->flags)) >> fnew->flags |=3D TCA_CLS_FLAGS_NOT_IN_HW; >> >> + refcount_inc(&fnew->refcnt); > > I guess I'm not getting the semantics but... why is it 2 now? As soon as fnew is inserted into head->handle_idr (one reference), it becomes accessible to concurrent users, which means that it can be deleted at any time. However, tp->change() returns a reference to newly created filter to cls_api by assigning "arg" parameter to it (second reference). After tp->change() returns, cls API continues to use fnew and releases it with tfilter_put() when finished.