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 79F44C43381 for ; Thu, 21 Feb 2019 17:45:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3D46120818 for ; Thu, 21 Feb 2019 17:45:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=Mellanox.com header.i=@Mellanox.com header.b="rGMkjpKu" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728721AbfBURp5 (ORCPT ); Thu, 21 Feb 2019 12:45:57 -0500 Received: from mail-eopbgr30046.outbound.protection.outlook.com ([40.107.3.46]:7552 "EHLO EUR03-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727859AbfBURp4 (ORCPT ); Thu, 21 Feb 2019 12:45:56 -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=4yRh5NdmXUBjgUz0jEK2Ttyrkyod0bcGM7A0GNhd+jM=; b=rGMkjpKuSHBRhSZ2g1OtXQ2yNdqii8cQbxfSQd3NUmp2BxLbujaZkHfvtORCxVu/sFz0TjzQNd7c6eX21mfaNKzXQTHx5iXuOy2WxbhCPS2bwnB3LN/cbK+l14/rzvGRffkN400M4tjCQk6ayiL8YfWJyrvzyyxr2hthNJgo5os= Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com (10.167.127.11) by HE1PR0502MB3036.eurprd05.prod.outlook.com (10.175.29.22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1643.15; Thu, 21 Feb 2019 17:45:51 +0000 Received: from HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::b03d:8cd4:d259:f749]) by HE1PR0502MB3641.eurprd05.prod.outlook.com ([fe80::b03d:8cd4:d259:f749%5]) with mapi id 15.20.1643.014; Thu, 21 Feb 2019 17:45:51 +0000 From: Vlad Buslov To: Cong Wang CC: Linux Kernel Network Developers , Jamal Hadi Salim , Jiri Pirko , David Miller Subject: Re: [PATCH net-next 01/12] net: sched: flower: don't check for rtnl on head dereference Thread-Topic: [PATCH net-next 01/12] net: sched: flower: don't check for rtnl on head dereference Thread-Index: AQHUxDmfnNJQQFjgm0S2EuLlSiVKMKXl8iaAgADz/YCAAmnzgIABQWoA Date: Thu, 21 Feb 2019 17:45:51 +0000 Message-ID: References: <20190214074712.17846-1-vladbu@mellanox.com> <20190214074712.17846-2-vladbu@mellanox.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: LO2P265CA0093.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:8::33) 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: 557f8940-2f3f-4da1-dd81-08d698246bde x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(5600110)(711020)(4605104)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020);SRVR:HE1PR0502MB3036; x-ms-traffictypediagnostic: HE1PR0502MB3036: x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1;HE1PR0502MB3036;23:z5+mA0O8xuKYDVsSocEvTR6rzVNiG1UWCFJGp?= =?iso-8859-1?Q?KrBv9KeQ5QJmapYHzNld3J4JliPmAoIbij3HJoyvfGSOpusBa9kN7LeN9e?= =?iso-8859-1?Q?JGDN4e5dINJqYNNKL0kyVwNKtBXbqtt20UjQLwwOXfwuvI6w4MuWDq8lpE?= =?iso-8859-1?Q?HGO2CeLCmgWXsPzL3sG8hL2fWawu0ZaidBRzWZX6XrGeLWOun5TrsJT0OS?= =?iso-8859-1?Q?bq26PgM/sDF4LmmghVew2ukK46qm/qy8NRYBBo/vkIiogWDeVcyT/TzHZz?= =?iso-8859-1?Q?ZDvi1Pp+vWJ+e6HJKpwzAPEMwy56FuNdEIde9I39t9toKm/eoCOFNILxdB?= =?iso-8859-1?Q?ATLuloUKzlxpooBX9Fqwc9cvVHiJ/T10cDlNssakrAJ28tagZ+yrn3QoGn?= =?iso-8859-1?Q?GoGftcTFF2zC+RTip0okN51A4DCEDyXSxosRID8f2HPyBfnO8AwCq6uDxK?= =?iso-8859-1?Q?F1/HgNnHbASN2OC05Ygs1pbLLtn9q7DW87YThFbxB2G8wrJZphFgGHs8eO?= =?iso-8859-1?Q?iMDXEFbLNysy6vZ7Ix1UKV2QLY8V4HhHqVqaITJcdLwjmC+ux1uQS8doqr?= =?iso-8859-1?Q?x9S4HeEhqxVulSF+ZXrstNq2vBjY6/H3tQ5J6tWoGJui2OrSEltCF3r60l?= =?iso-8859-1?Q?unlqe4j6nQ6fVdh0PmyXK67vnnKJ5G1NeFUsLqLnQVk7md5NkhdXDu/e0N?= =?iso-8859-1?Q?BaJs53iepyT5CJLZDkX2u1klOcRLWAY1UGzaEWjyJgcESv6tE6mpSjZwqi?= =?iso-8859-1?Q?3qM39BnkXNSyZqlPYOTNb5lJpOQ2QL45YeJsaGsKzryHqIgbSmQ0GKEFmI?= =?iso-8859-1?Q?avb3CKp87j1BepntyIruT8FYE8uvq+Y518yM50pDm+Jzg6b/hzizrP7922?= =?iso-8859-1?Q?aXK/mUx7qNnhuw05FXbDaw+Epfc3iRjMoU4e+DGFzroRATKX0TIb1abxIh?= =?iso-8859-1?Q?9xqWzdsRFuKYcDG7E/jN4MDA4szawVtgUNevl5NORS3MPragqxmAYMc3c5?= =?iso-8859-1?Q?s5L+R8pDuARP52GM+AzJoJOkT8rYuSD5wEU+4ryfXQuHr4/fX27RAiPXmd?= =?iso-8859-1?Q?3alpfgb9hZ0xaOcsgMSs86INEXQz9Q8tCUBJTfvXVaVLxJIFNFaByR+5RX?= =?iso-8859-1?Q?n97zoXlCapkC3Al91b4gabjBGHs8gIiVqjy7QM32oneJSYyyGnofV7rItZ?= =?iso-8859-1?Q?IRhj/8b/J7HBPf02+aokTfh39fvPkjGOr0PeRk7y4sMF5OPrzD8eKGbIBL?= =?iso-8859-1?Q?nig4yMG8dAHdwI1lcJ/Ogn+0NfxgJe1ag7dI9UTGtFWyIDahmX1mSHX+D3?= =?iso-8859-1?Q?cMS2J37JxQskoq/hMAg15o4B/mFBjVPbqrmetv/bairfX+w=3D=3D?= x-microsoft-antispam-prvs: x-forefront-prvs: 09555FB1AD x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(136003)(366004)(396003)(346002)(376002)(39860400002)(199004)(189003)(54534003)(8676002)(81156014)(26005)(68736007)(8936002)(6486002)(6436002)(6506007)(81166006)(53546011)(2906002)(6512007)(54906003)(6246003)(36756003)(3846002)(93886005)(6116002)(5660300002)(386003)(102836004)(316002)(66066001)(86362001)(97736004)(4326008)(99286004)(71200400001)(14454004)(478600001)(7736002)(25786009)(305945005)(2616005)(52116002)(6916009)(106356001)(476003)(76176011)(11346002)(14444005)(486006)(446003)(229853002)(186003)(256004)(53936002)(105586002)(71190400001);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0502MB3036;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: 4jD9supFLeovW+HYbb96ZLMCb3OwQHaBRCvnzNy2ojO3TXEGq55OORm8B3+FN7jYSK/6eL5j5A/VKWDAkltC848Fw678985tnAPEjeyVw0VZWygyizhAbHoS4DkgQfniDn8zoZdIy1lFACDo0uNeAoCl3U3aToOw8uLY8Os03YSwir8eslx+gqFjAvPW30qhtmIO9u9zphVmK/+dtCXQSHPln42snkiuz8zTgKajRfgNLHkSBVmpSB1LHVIg9I2LKpicd6W1OdZ143d9dcwD7Qas3igXHBKHFAUxtRSAN00faVkn/VzHKiuZOlQoh6rKaA6KU+mxbEUYCIHU9uSchUcJXXgDuirKfb8Cxfr7yr/7lcPd0ON09g7cYyzM11Gk/QKmf7LlCxsqybaFIILkHJjdMFw6vxAjzYYxyY2CsHo= 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: 557f8940-2f3f-4da1-dd81-08d698246bde X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Feb 2019 17:45:50.7820 (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: HE1PR0502MB3036 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed 20 Feb 2019 at 22:33, Cong Wang wrote: > On Tue, Feb 19, 2019 at 1:46 AM Vlad Buslov wrote: >> >> >> On Mon 18 Feb 2019 at 19:08, Cong Wang wrote: >> > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov wro= te: >> >> >> >> Flower classifier only changes root pointer during init and destroy. = Cls >> >> API implements reference counting for tcf_proto, so there is no dange= r of >> >> concurrent access to tp when it is being destroyed, even without prot= ection >> >> provided by rtnl lock. >> > >> > How about atomicity? Refcnt doesn't guarantee atomicity, how do >> > you make sure two concurrent modifications are atomic? >> >> In order to guarantee atomicity I lock shared flower classifier data >> structures with tp->lock in following patches. > > Sure, I meant the atomicity of the _whole_ change, as you know > the TC filters are stored in hierarchical structures: a block, a chain, > a tp struct, some type-specific data structure like a hash table. > > Locking tp only solves a partial of the atomicity here. Are you > going to restart the whole change from top down to the bottom? You mean in cases where there is a conflict? Yes, processing EAGAIN in tc_new_tfilter() involves releasing and re-obtaining all locks and references. > > >> >> > >> > >> >> >> >> Implement new function fl_head_dereference() to dereference tp->root >> >> without checking for rtnl lock. Use it in all flower function that ob= tain >> >> head pointer instead of rtnl_dereference(). >> >> >> > >> > So what lock protects RCU writers after this patch? >> >> I explained it in comment for fl_head_dereference(), but should have >> copied this information to changelog as well: >> Flower classifier only changes root pointer during init and destroy. >> Cls API implements reference counting for tcf_proto, so there is no >> danger of concurrent access to tp when it is being destroyed, even >> without protection provided by rtnl lock. > > So you are saying an RCU pointer is okay to deference without > any lock eve without RCU read lock, right? > > >> >> In initial version of this change I used tp->lock to protect tp->root >> access and verified it with lockdep, but during internal review Jiri >> noted that this is not needed in current flower implementation. > > Let's see what you have on top of your own branch > unlocked_flower_cong_1: > > 1458 static int fl_change(struct net *net, struct sk_buff *in_skb, > 1459 struct tcf_proto *tp, unsigned long base, > 1460 u32 handle, struct nlattr **tca, > 1461 void **arg, bool ovr, bool rtnl_held, > 1462 struct netlink_ext_ack *extack) > 1463 { > 1464 struct cls_fl_head *head =3D fl_head_dereference(tp); > > At the point of line 1464, there is no lock taken, tp->lock is taken > after it, block->lock or chain lock is already unlocked before ->change()= . > > So, what protects this RCU structure? According to RCU, it must be > either RCU read lock or some writer lock. I see none here. :( > > What am I missing? Initially I had flower implementation that protected tp->root access with tp->lock, re-obtaining lock and head reference each time it was used, sometimes multiple times per function (head reference is usually obtained in beginning of every flower API function and used multiple times afterwards). This complicated the code and reduced parallelism. However, in case of flower classifier tp->root is never reassigned after init, so essentially there is no "CU" part in this "RCU" pointer. This realization allowed me to refactor unlocked flower code to look much nicer and perform better. Am I missing something in flower classifier implementation?