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=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 6AB3BC43381 for ; Fri, 15 Feb 2019 10:38:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2E06C21924 for ; Fri, 15 Feb 2019 10:38:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="msUCkkKd" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2405667AbfBOKiN (ORCPT ); Fri, 15 Feb 2019 05:38:13 -0500 Received: from mail-eopbgr10048.outbound.protection.outlook.com ([40.107.1.48]:7776 "EHLO EUR02-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726952AbfBOKiM (ORCPT ); Fri, 15 Feb 2019 05:38:12 -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=tUoroFN8Z/+eiasgyCXCke2BvDHk7rcRr0cBrqjsWio=; b=msUCkkKd7Y1jrOpLrmcdfRL7nWuxnLHUJyUD0lvDFFjcOWwiMxPo+qREqdY2I0y01qIWvcp1IdRUGiyjGYkkVP56jr+60CL4fY6b4EV7OsUjz/2/u3LE3QUmrf3kX6uNv/jA1Q8TMoTQZgdkFxJT3+JsPGCE3MvZ+Y+cML/3iso= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB2892.eurprd05.prod.outlook.com (10.175.34.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1622.18; Fri, 15 Feb 2019 10:38:04 +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 10:38:04 +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 02/12] net: sched: flower: refactor fl_change Thread-Topic: [PATCH net-next 02/12] net: sched: flower: refactor fl_change Thread-Index: AQHUxDmXK4xMRl3b/k6q0KMty4qoGqXfwMoAgADrNIA= Date: Fri, 15 Feb 2019 10:38:04 +0000 Message-ID: References: <20190214074712.17846-1-vladbu@mellanox.com> <20190214074712.17846-3-vladbu@mellanox.com> <20190214213402.67919dea@redhat.com> In-Reply-To: <20190214213402.67919dea@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0373.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a3::25) 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: c2d76838-052d-4eb0-9324-08d69331aa3c x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(5600110)(711020)(4605077)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020);SRVR:HE1PR0502MB2892; x-ms-traffictypediagnostic: HE1PR0502MB2892: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB2892;23:E4TpKXPGQEPNPLvGNLFskBWTMaRejfSjH2nmq?= =?iso-8859-1?Q?HNcpVcOHLAYPs9W2mkgP8g8aQfuBvnYsxyxDFwHo/k5v1OxL/LqSsnqzQm?= =?iso-8859-1?Q?xQDZiat2yMJSLV5VfAKfRltOlUAdsVkabw5mX8khoanj60/q/WkzulvTS/?= =?iso-8859-1?Q?hJCUETnlRfqjUCY/X5/Z+CGT5HK+lsDnyl6NPQdwsNKG2u9E7PIBkMBdF8?= =?iso-8859-1?Q?be1qe+MenCum2/Nn9uzvNZpms1/kncx7buJg+hDwMVZJyNEY5HETBO/Gtj?= =?iso-8859-1?Q?yi75UEXj8kYx2i44xEd84q6sa2JTeumjwP+GoMgGdQDAhlhaMj+58otuMe?= =?iso-8859-1?Q?Jf3irpM+ThOfNGBuliQ8rCd+YHVw5X8VWEC7S6DBsHyz9IjsngF7mBTRXV?= =?iso-8859-1?Q?5bJKvmjTh9tCC2lUQWdPVMOTKeNN2g4wcSZUki5pw/o1ze66Y+OnQ67tX0?= =?iso-8859-1?Q?WhVqKVGZM43AAaabN1s6IuT7RBuRe4hlHJr6Z6IfXaMiWZUWhEzsVmYZGU?= =?iso-8859-1?Q?aGK+jmjIYfzEGkRzXRCZd9LvohURGHSkNci18zPDjMY4LNEaSsExXuYEXY?= =?iso-8859-1?Q?v/FJOU5ndkoXEjwFGPZyRvvMXtp40UFtuIaTw4PVgUEFJ0a9FkCyU1wZq9?= =?iso-8859-1?Q?XFQm/u9n2dZqh4YGIqVWGEYKESNapQGg5QElGd0MhpxtqiWP6ybP+ICqLt?= =?iso-8859-1?Q?UrMPKnqNC1UBPfNhGupTm8K9+S3LnXtLTv/2J4jaiR5ybN/YJFnD4eOkhM?= =?iso-8859-1?Q?1QznjsBLyrSf9pX8HuAbPPRJaCH8Q4NwqjIlUjtLw2fE19+OYcAFYG1El0?= =?iso-8859-1?Q?aMBXuyIHhIOWyG5ubniErV1rPPUI+ID4VyMgodX/7G/ZlNo3Vp6x1jE5b0?= =?iso-8859-1?Q?XMAhQJpXqc8A0lrAPPbnZRRjuP43Yyo4ue/s5LPUGstRvo3Qmwvowiqhxj?= =?iso-8859-1?Q?1BYyYkMspf5HqPFqcloQTl6fYCN8BAEppmQOG17h+Ddn/F9Js0t7y+lBmo?= =?iso-8859-1?Q?8/ye/LUZ8TAw+MSBS6G3Ksjw7/AVd1Zf6anrpZe75EYqvmKVOvm5YedEKx?= =?iso-8859-1?Q?kzl+X2Yzzf6y4p+SYc5JldSjeLPQQk/5EQG4MKesoA4s6g99LVrZkQIqT7?= =?iso-8859-1?Q?2k1BhU8ZhfcvJiUVYM3cNqSHvtHpFhs1UaCjf9Pk/nChmdouvkT24ODKKF?= =?iso-8859-1?Q?/PhhliCJighdShCrxmWMJcupe9sND+ROxNqTNyl74wNxlnM+Mc+XBPFe1m?= =?iso-8859-1?Q?NouyHeSVW/LCmcRTtmf?= x-microsoft-antispam-prvs: x-forefront-prvs: 09497C15EB x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(396003)(346002)(376002)(39860400002)(136003)(366004)(189003)(199004)(446003)(8936002)(2616005)(3846002)(6116002)(26005)(11346002)(486006)(8676002)(6512007)(53936002)(229853002)(6436002)(6506007)(54906003)(4326008)(386003)(476003)(81156014)(6486002)(68736007)(102836004)(81166006)(316002)(36756003)(7736002)(6916009)(71200400001)(76176011)(14444005)(71190400001)(256004)(86362001)(186003)(99286004)(66066001)(52116002)(478600001)(106356001)(97736004)(25786009)(6246003)(14454004)(305945005)(105586002)(2906002);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB2892;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: XyDTrBQqnTCc0Dfz9he7oJwUniJwD+wpn9xEol4HDFOf34EUmSr95I0hfKX5dNmZCpzH3NRAKsllcP4bVTxQUHi9UMeYzq5KqbpYnrOYc1aktUcEqYmMLhIG2CpoJhHSjWSqEJbF92933mquqrZl7Xlbh5JMqgb+VmYlXGH4wFXTguuDILLKvcNjOria1jjD1UI+IsQ11OY2N2ACeqcfXCKW1+CuUsZ1bBLJ1L2zsw/p/vt9VpeoDnaKNx/sNkQQDrvwt/0gvbZiYA5Mm8GOKvjWlJHQP/SEMC7qlWbSKAEnc5NZXlQ4nyRhRhXMsWlDSAk/wpKQ5+BtiSUuOYdaIiJFkCwMr5zK4eOGO8aLQj8vW4ZDxjVgjPgGx023je8ZosVU7Gk8+Ne4ewj1qpZz5I4jy1CS4ySidGff1WCSd/w= 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: c2d76838-052d-4eb0-9324-08d69331aa3c X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2019 10:38:02.9504 (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: HE1PR0502MB2892 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:02 +0200 > Vlad Buslov wrote: > >> As a preparation for using classifier spinlock instead of relying on >> external rtnl lock, rearrange code in fl_change. The goal is to group th= e >> code which changes classifier state in single block in order to allow >> following commits in this set to protect it from parallel modification w= ith >> tp->lock. Data structures that require tp->lock protection are mask >> hashtable and filters list, and classifier handle_idr. >> >> fl_hw_replace_filter() is a sleeping function and cannot be called while >> holding a spinlock. In order to execute all sequence of changes to share= d >> classifier data structures atomically, call fl_hw_replace_filter() befor= e >> modifying them. >> >> Signed-off-by: Vlad Buslov >> Acked-by: Jiri Pirko >> --- >> net/sched/cls_flower.c | 85 ++++++++++++++++++++++++++-----------------= ------- >> 1 file changed, 44 insertions(+), 41 deletions(-) >> >> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c >> index 88d7af78ba7e..91596a6271f8 100644 >> --- a/net/sched/cls_flower.c >> +++ b/net/sched/cls_flower.c >> @@ -1354,90 +1354,93 @@ static int fl_change(struct net *net, struct sk_= buff *in_skb, >> if (err < 0) >> goto errout; >> >> - if (!handle) { >> - handle =3D 1; >> - err =3D idr_alloc_u32(&head->handle_idr, fnew, &handle, >> - INT_MAX, GFP_KERNEL); >> - } else if (!fold) { >> - /* user specifies a handle and it doesn't exist */ >> - err =3D idr_alloc_u32(&head->handle_idr, fnew, &handle, >> - handle, GFP_KERNEL); >> - } >> - if (err) >> - goto errout; >> - fnew->handle =3D handle; >> - >> >> [...] >> >> if (fold) { >> + fnew->handle =3D handle; > > I'm probably missing something, but what if fold is passed and the > handle isn't specified? That can still happen, right? In that case we > wouldn't be allocating the handle. Hi Stefano, Thank you for reviewing my code. Cls API lookups fold by handle, so this pointer can only be not NULL when user specified a handle and filter with such handle exists on tp. > >> + >> + err =3D rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node, >> + fnew->mask->filter_ht_params); >> + if (err) >> + goto errout_hw; >> + >> rhashtable_remove_fast(&fold->mask->ht, >> &fold->ht_node, >> fold->mask->filter_ht_params); >> - if (!tc_skip_hw(fold->flags)) >> - fl_hw_destroy_filter(tp, fold, NULL); >> - } >> - >> - *arg =3D fnew; >> - >> - if (fold) { >> idr_replace(&head->handle_idr, fnew, fnew->handle); >> list_replace_rcu(&fold->list, &fnew->list); >> + >> + if (!tc_skip_hw(fold->flags)) >> + fl_hw_destroy_filter(tp, fold, NULL); >> tcf_unbind_filter(tp, &fold->res); >> tcf_exts_get_net(&fold->exts); >> tcf_queue_work(&fold->rwork, fl_destroy_filter_work); >> } else { >> + if (__fl_lookup(fnew->mask, &fnew->mkey)) { >> + err =3D -EEXIST; >> + goto errout_hw; >> + } >> + >> + if (handle) { >> + /* user specifies a handle and it doesn't exist */ >> + err =3D idr_alloc_u32(&head->handle_idr, fnew, &handle, >> + handle, GFP_ATOMIC); >> + } else { >> + handle =3D 1; >> + err =3D idr_alloc_u32(&head->handle_idr, fnew, &handle, >> + INT_MAX, GFP_ATOMIC); >> + } >> + if (err) >> + goto errout_hw; > > Just if you respin: a newline here would be nice to have. Agree. > >> + fnew->handle =3D handle; >> + >> + err =3D rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node, >> + fnew->mask->filter_ht_params); >> + if (err) >> + goto errout_idr; >> + >> list_add_tail_rcu(&fnew->list, &fnew->mask->filters); >> } >> >> + *arg =3D fnew; >> + >> kfree(tb); >> kfree(mask); >> return 0; >> >> -errout_mask_ht: >> - rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node, >> - fnew->mask->filter_ht_params); >> - >> -errout_mask: >> - fl_mask_put(head, fnew->mask, false); >> - >> errout_idr: >> if (!fold) > > This check could go away, I guess (not a strong preference though). Yes, it seems that after this change errout_idr lable is only accessed from else branch of if(fold) conditional so the check is redundant. > >> idr_remove(&head->handle_idr, fnew->handle); >> +errout_hw: >> + if (!tc_skip_hw(fnew->flags)) >> + fl_hw_destroy_filter(tp, fnew, NULL); >> +errout_mask: >> + fl_mask_put(head, fnew->mask, false); >> errout: >> tcf_exts_destroy(&fnew->exts); >> kfree(fnew);