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,URIBL_BLOCKED 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 BBAA1C43381 for ; Fri, 15 Feb 2019 16:25:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7558A222D0 for ; Fri, 15 Feb 2019 16:25:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="WtNfo/PG" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726831AbfBOQZ6 (ORCPT ); Fri, 15 Feb 2019 11:25:58 -0500 Received: from mail-eopbgr00041.outbound.protection.outlook.com ([40.107.0.41]:14084 "EHLO EUR02-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725939AbfBOQZ5 (ORCPT ); Fri, 15 Feb 2019 11:25:57 -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=nko8WPEywtx8tZL/ny4rtmUoygLkf/S97KMgUBwSPU8=; b=WtNfo/PGYh4RUkesqWpx6pWChEeFPkMh5e1y7OUKzJE/GGQRrDE/vYY1ZBj8a3HBpQ0kfuOj9cMF29r87kbucaSGamwCbCO4rTjVwBV5D3Bt+OiBrTrliNVETRU+guCKNPpbrDReBp+0aGa3aN0nG7u0N2uGMo+YSSQpg+c7dJA= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB3738.eurprd05.prod.outlook.com (10.167.127.140) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1601.22; Fri, 15 Feb 2019 16:25:52 +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 16:25:52 +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/k6q0KMty4qoGqXfwMoAgADrNICAAAMkAIAAXgiA Date: Fri, 15 Feb 2019 16:25:52 +0000 Message-ID: References: <20190214074712.17846-1-vladbu@mellanox.com> <20190214074712.17846-3-vladbu@mellanox.com> <20190214213402.67919dea@redhat.com> <20190215114706.244fb7c2@redhat.com> In-Reply-To: <20190215114706.244fb7c2@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0394.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:f::22) 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: ec2c8440-fa4c-46b8-8120-08d6936240ed 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:HE1PR0502MB3738; x-ms-traffictypediagnostic: HE1PR0502MB3738: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB3738;23:gmqj2Z0EWS7k3VavMvDYmW2CBsrmLkbd8yFB+?= =?iso-8859-1?Q?l8LUkeX7ok5qk5Fx/mbvcd89R9+ls0gslXNqufHJ9JtcjZ7x6arsD/uYMC?= =?iso-8859-1?Q?SI0c1Vf24sX7cV+gwP26+IyBr1sy83kmiSf4V0MZB4KGIaVsNL5aSmpDi/?= =?iso-8859-1?Q?EC/sayvN4KItzuIGmkmRs17AtNaxrQbRd7wFfSuhWFZ7gNVpzHZ7Yk2/YE?= =?iso-8859-1?Q?wmb0Hj9+x4OPTovAC6odEsbpy/QasP/QtbCWuJvB286usfkuitwRU3l93o?= =?iso-8859-1?Q?qANJHKeNPP3r8ttBjmj3r3Nt27MFvOXN7eyh/f+dJnBCOd7kggAc/hh1zF?= =?iso-8859-1?Q?psPCUVKWGRlzHKPfNXZqR2tiqUiXWOJB2lpuH/jWb9iUZD93Ts0HnTBY9E?= =?iso-8859-1?Q?XbnFFBlFmjl48G8dxV6laJ/NnhYhLndIhQOt0m1AZfOb3l6gcVFHXMouhC?= =?iso-8859-1?Q?8hAbNOB2rPTFZmcieDoOlL4rg4TTmfbg3DMcgZ5yn1ZXHou4H10O3ZfXcF?= =?iso-8859-1?Q?ha+O7cA0fwYmBD3FjafSRzjdLNJXkl2liDFYjL+Zj8mOcELbdzkCt5iqXv?= =?iso-8859-1?Q?ZbtnsFxBzoZ+tZV0IB52uK8nXtk0Da9m5FN5tBpQsJ0QYKAB2MZJvRsFNY?= =?iso-8859-1?Q?UspiM2Np/gxPzSn8fDpzQc5kb5Bh++IuBeV5vHfOGvwPtXTaauoj6E6b/S?= =?iso-8859-1?Q?ROO0JOqe24TpmYFYgHxTqYxiSJDBgveywQNxVad+M9/kc/lzaBZYyJwUEh?= =?iso-8859-1?Q?gXTvrXENjkQ3X9t2Mn0wWaU74mjBlylPq9xQvYfVGrxSHsoipoeb3w80N3?= =?iso-8859-1?Q?m52fvMmxlUxjLKAtMCTeuoCYCRkKt0Pbu9uMZVW2iouuBKHFTW0oVe9rHO?= =?iso-8859-1?Q?UHENsLEO5KLsYlGS4KbkzCTHwokTZJBKf0lRTKFCz3YIN/QsN9xZNychnD?= =?iso-8859-1?Q?BQ1lz4YX4d8OSA9Ou7m4Fi1KLmgThogkysscaI+UuPE1Ez1BMk5JvXrCIf?= =?iso-8859-1?Q?kxWXOcdkNSyGlUgsZIJElpdmLy9am+vqUNXt0Hv7mNGw4EmcjPmeaa+oEv?= =?iso-8859-1?Q?9f9u/8bPAHyabsBmjBN6k6FFRviG2s2RG4UHUr2QIeaEmRigQVCKzZbDqn?= =?iso-8859-1?Q?4fEYZTbWEWXtmMhXfi+IYBHk/kY6cF76UQpHyk+G/4vwKk3Izi0n0KiLNz?= =?iso-8859-1?Q?2sLYj8w0Mb2tZzbXNoJW2qfhcuqwkmtToKdbbUyz+wmo+dquxm/YFpXMkE?= =?iso-8859-1?Q?WNEynTxC/QZR4gsuOR8sdgbwIs+FFr2kvEec8+v0g=3D=3D?= x-microsoft-antispam-prvs: x-forefront-prvs: 09497C15EB x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(376002)(346002)(39860400002)(366004)(396003)(136003)(199004)(189003)(6436002)(14454004)(71190400001)(71200400001)(53936002)(478600001)(446003)(11346002)(105586002)(106356001)(6116002)(76176011)(4326008)(52116002)(36756003)(6246003)(25786009)(86362001)(99286004)(66066001)(316002)(14444005)(81156014)(81166006)(229853002)(2906002)(8676002)(7736002)(8936002)(3846002)(256004)(6512007)(6916009)(54906003)(486006)(102836004)(68736007)(476003)(6506007)(386003)(26005)(2616005)(186003)(97736004)(93886005)(6486002)(305945005);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB3738;H:HE1PR0502MB3641.eurprd05.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX: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: T7xwevxZZt8XrdZbt3SfGMJUqs4c7n5Br6HVnMIHRhLmUn98BbRdTJ7QC1jHSijVP7CqsJ6gAsurX/h6mqskbxkRiU/L3EuVk4bpcA6KCGMgk/OqWdVfoh5JC6y5tu/PF7vFM2WLqz2UagReNAUN8OsTZikFOvp4KXV2MFt65yUxluloriO3HYGzFZ1Wuuj8veJr0dY1kuV+YB2x/2qKzLLYm+pZUjN18ycbcy7JypS9iFpuoTGDKiFGL5S3Q93qr4Etb2AklsN+YWEEMv/XdYxMF9L5ydJwjMvDGVGoFhzQkQoIxFJIJNlliZTKPCRo0i8WNdN7yOBHSFx2dUUKGYjKSQDJnfjVIP2SwfuWXUBAzrIjxX99uEJx7QttF14wtQIwEWip+zWHvZ5n8Oc+N0d3MuT80iCqPtu87rgXIMU= 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: ec2c8440-fa4c-46b8-8120-08d6936240ed X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2019 16:25:51.6863 (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: HE1PR0502MB3738 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri 15 Feb 2019 at 10:47, Stefano Brivio wrote: > On Fri, 15 Feb 2019 10:38:04 +0000 > Vlad Buslov wrote: > >> 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= the >> >> code which changes classifier state in single block in order to allow >> >> following commits in this set to protect it from parallel modificatio= n with >> >> 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 wh= ile >> >> holding a spinlock. In order to execute all sequence of changes to sh= ared >> >> classifier data structures atomically, call fl_hw_replace_filter() be= fore >> >> 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. > > Ah, of course. Thanks for clarifying. By the way, what tricked me here > was this check in fl_change(): > > if (fold && handle && fold->handle !=3D handle) > ... > > which could be turned into: > > if (fold && fold->handle !=3D handle) > ... > > at this point. At this point I don't think this check is needed at all because fold can't suddenly change its handle in between this check and initial lookup in cls API. Looking at commit history, this check is present since original commit by Jiri that implements flower classifier. Maybe semantics of cls API was different back then?