From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760524AbcBYMD3 (ORCPT ); Thu, 25 Feb 2016 07:03:29 -0500 Received: from mail-db3on0073.outbound.protection.outlook.com ([157.55.234.73]:37970 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753606AbcBYMDY (ORCPT ); Thu, 25 Feb 2016 07:03:24 -0500 X-Greylist: delayed 77762 seconds by postgrey-1.27 at vger.kernel.org; Thu, 25 Feb 2016 07:03:23 EST Authentication-Results: spf=pass (sender IP is 193.47.165.134) smtp.mailfrom=mellanox.com; vger.kernel.org; dkim=none (message not signed) header.d=none;vger.kernel.org; dmarc=pass action=none header.from=mellanox.com; Subject: Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller To: Parav Pandit References: <1455966006-13774-1-git-send-email-pandit.parav@gmail.com> <1455966006-13774-2-git-send-email-pandit.parav@gmail.com> <56CDAC7A.6030206@mellanox.com> CC: , , , , Tejun Heo , , Johannes Weiner , Doug Ledford , Liran Liss , "Hefty, Sean" , Jason Gunthorpe , Jonathan Corbet , , , Or Gerlitz , Matan Barak , , , From: Haggai Eran Message-ID: <56CEED81.7010507@mellanox.com> Date: Thu, 25 Feb 2016 14:03:13 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.0.52.254] X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;DB3FFO11FD012;1:tdWxZXcYlRDhAjMEN0FjK53k90Itr0i77jMXm/C0KX6mD0DMXKTTMtNTBBhUOxJ/fbCE/NroovB8gFHqacDFZ5uJekgUKwoPciIXHeeBMRkeDg7G2hTqBObu6tTIVvlXDza/tihFm8c+N+p+229Jv3AqKQdRIj0OhZIuLCiSADs4ueqBhX/781OebLWaAEz7pPUsc2S1KJML3F0BvGxpmgF40AohxRh+J2yCsl7QosVaNGNQQRCzHfmndmR5Io5J2lVH8s71gW1xhfuZ/7JiVJksru1UeZNk1fVix1/3DIRiuonkeqdjebaoygFevGJYYMf7Gt0M9JyoCcSiM4ScxtSMOnRuCscgDYRiqMxgWLqJQ/DrWskdzJW4PlpDOzFZ878HKzlN+3wQHO6x6D+6LKNS+7uUs5GiNzRJ8FvfPUNTokY/ViOf7DAQkelpkApc5xD/1FyKPzi5O0o0H6JS5g== X-Forefront-Antispam-Report: CIP:193.47.165.134;CTRY:IL;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(2980300002)(438002)(24454002)(199003)(479174004)(164054003)(189002)(87936001)(50986999)(3846002)(11100500001)(65816999)(76176999)(189998001)(6806005)(54356999)(23676002)(65956001)(4326007)(110136002)(230700001)(36756003)(5008740100001)(5004730100002)(586003)(1220700001)(47776003)(2906002)(86362001)(6116002)(33656002)(50466002)(92566002)(4001350100001)(77096005)(2950100001)(106466001)(80316001)(1096002)(3940600001);DIR:OUT;SFP:1101;SCL:1;SRVR:AMSPR05MB422;H:mtlcas13.mtl.com;FPR:;SPF:Pass;MLV:sfv;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;AMSPR05MB422;2:PkQYGzE5KuwWLl1zgH4VofWcLaeVBlvbTAnx6+iMSuwHgKxb+B3CkDNPVLMDeECxew7CDdBNXpsvwtxwIzZU1Mw4VW3cpde5Yrrygn1yRIPCvR4oiBowyHt6JtpGwPA+f1eDViISYy6HIgidGHUkUw==;3:ZNjtSZ9kwi6PimP1/j5JQbZmPRVy7m/v7V0idFdx/tb8E51uepeqVQ6u0iNCu8FRevtC6NYYEQcGG/JYd0OpM9iNznJXFsky9QzGkgDXqBGDR6LSv4JuMKbQZvBKuzAqOzuUlxe5IqoHcVJ2j7o0wAGF3xMf+t7VcNyap5WwEaCqgGtbSN0HJx/oxn4PG+rhr2ETfbxM4D1KJ+MaG3FNxgQM991rnYdXlfRaE2LtwRHFNl6BRp0IFjS+EwqET9I2/PKaQSDiZOTWqOboe3CFfw==;25:WkZhNeWtxjKCcROiU2+oQO3OynqDj5T5RyY6qSXCXll8QVHieUdn1AQE01+y7vRbEiQvsTwd00Z+YRkYWjT2zxpBWDp4Y56+Td3gweGFSIRHNslo896i4vsmsmT+kXTJvRnVGbLMaeHQGxbR9SyqOWqay1wXSc0JwzlO0HFbntauNWleTZENvgFDW8JFLtonbwwbhfzmyvICa4TjPRBgfiIk3k2v+p7uNn09+QK4AoWd1XLXiAEAPmdWi1GOjMeDehRw5qxhTOM6ojUKGXthokiTsVTcV9y48/yuewlnlLpPp67S+KlFEhGFV5vudJBtfpIfGFO0bindMF06U9ar1Q== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(8251501001);SRVR:AMSPR05MB422; X-MS-Office365-Filtering-Correlation-Id: 9d4ea1fd-2b46-4528-39f5-08d33ddba6a4 X-Microsoft-Exchange-Diagnostics: 1;AMSPR05MB422;20:cJk72+KmmXkNpP1HVFCcKke2NITfwlefenVM4kIjId/63+RYj/vzwxgi/j9WeLBzpLv3lHD2alYh32tzJlW3xhRynqdD6CzMpnVxesTsOjpLcWwGFYnhaPs+mMerUqBNaprz967HomyF10SaBeOhcpIfCcUqqKg1zvakelYnxMri0ukyO1AgqCoHp5PsFNDtJ28E9DOVLMpAavWHYu7foSwvjudJ9bleZvFbui3yIfJyFOYZBpVKLTKHq8Y69yG4dLvw2W98iz6aKnBBHgb3X+KCEFWjuhLQBqyGAORWzOdiVWteqRTurYeyV4Q7+kc2Z8XHOK8neSLK36bk7AZJT0RNWMY/nclnF2NhreML+914Wqd72xipQJCkBARn+1Hv9Byxcbc5N6q77PLHJIWNokjY+nXg1gq4UUHnKbxst1CpyGfhPk1jzgUu1H7zJVbBmRfyIntyLlOzmbMz8nCDCQT6lx78qG5HOd2KhrRYjRArISyfofsVUDjn/8SVLe8E X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(13018025)(13017025)(5005006)(8121501046)(13024025)(13015025)(13023025)(10201501046)(3002001);SRVR:AMSPR05MB422;BCL:0;PCL:0;RULEID:;SRVR:AMSPR05MB422; X-Microsoft-Exchange-Diagnostics: 1;AMSPR05MB422;4:w9LcRBvGnxms1YEfGOyHdyIa5HeNCjgnwKwiHi0jKl5fwFcdvu9VUKDAr2XvxIruOlWxJQjWCA0U7LzaBUax/Q2Ue5ZaJoQU5xxlgNhx5Jz3BMB5cRizxWZh9Xkx7Kjoj1L9UlEjbM6B3RaLlIOBYC9IGDknz32PaEScwQWSkC67WbMr5wMKG58al2UH4YhPRFwa/JPwTlpsZ9f8/W8z8oV7CPb9L4Q0KEPWzz1eelLJ0Fg7Y4racbpk321uyRj3It5m+rmWYgK0ps4dU6uurfT3mUbz0DgQLD4E8oKZ8vquojRNueN7bSIFi8HiYsUjoka0eK/xY4IKdvS3qIuEinvwrn2RZ6+GGY/Z5SeitJbk4AzOvO8zWOsl9SP0bvFjGfXhcVj4XsaOHwDC71nfT0LjNTSktignMPGRa11HESM0sr270CDeNZf65SjuJJiYBhVxXzaJPfS8lcwFWNpTqQ== X-Forefront-PRVS: 08635C03D4 X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtBTVNQUjA1TUI0MjI7MjM6clMzaVlENXNtVWh2eHlEOFNydmhUb0dhckNI?= =?utf-8?B?ZW91a2VYdy9PM1hoczdOQUd4MTl6SDEzdnlhTEtyVklQSnVkclhhMi9uRUht?= =?utf-8?B?T2NzTGpOaUluNDZNUUZ5UkJzM1VxV1lZQWU3clA5T0tvay9JbEN4OXVYK3Vo?= =?utf-8?B?NkF4WFJndlltdGZxbmNpOVpIZWNKNTRmN2EydjhvV2h3SlJVWnZ4NHgxWmln?= =?utf-8?B?U25DR3BzaUxvV3doKzdVVmVsVHc3NWZjUndWQWIra1FITTRkQnNma2RPQWRY?= =?utf-8?B?TFNUN1VBSFpIK0V0czdBOFNvQk1adzJNYWhDME1PWFFjWDdkYXBJd3V2NnBu?= =?utf-8?B?TE5sQWd2ekFVdklXZWkvTmxDUEY4clZyNTIxd1RzRyt2eFM2VmpJQ1BacDBM?= =?utf-8?B?N2RYeXBpaHpydHhENklmMUNpdkt5T3lOQmxWUVNGdlREdHlIZW5seFhRVlpV?= =?utf-8?B?c0dpYldmT3RKOFFTM1FndldaNlZjK29zbUNWL3k3L2xBM2V4V3E4N2RqcVRY?= =?utf-8?B?L3ZiUWZSOU1yNmdUYk9MRURxV0VFZ3Z4QWJpSUlub3Y1azJSN3JRUk5mNFlX?= =?utf-8?B?QXJ0Zm9pYzZld1Z2M3BGUXV4c01leXF3d3RYQzVWVzltNno4djduMDUrSEE4?= =?utf-8?B?K0MwMytoaXdudDdLRzc4QkNxaE0xdnhhVWhkWnExZlhLY29sSzFnbXhSdW83?= =?utf-8?B?b2xtQjRIM1IzVDJBUjREbFgwd1orT2NCeDBSZmZSeUNlUEFCTUJXeUF6dVJL?= =?utf-8?B?bmh1d3pVR0pnM1ZQWWp6YnBScmFlY25KVWg5RklKQVRCM1F1alFNU0pPZFpk?= =?utf-8?B?YTBNQThJV1ZUNU56aWcvMUZ0LzZFRVl0RFRFQjdtQVNhR3N4Z0toVVpwTmN4?= =?utf-8?B?SytNVmpvU3E5ckdYWFRzUHkvQXgyN2NSNkl4dHhSM0hXWjlJZWVqK1FMK2h6?= =?utf-8?B?MWtyT3RSck5qMFJwTFRLTVRCS0JCTVJ4eVh2c0dRSFBlZlFHVmh0ZmxsQU9T?= =?utf-8?B?aWJhZ2R0WFZDK2FPcXBpdGgvb1dWN3BqSXdHSFJOamtkZHQ3SFlFaDNkZkZa?= =?utf-8?B?S1VXaXpkckRxZkU3S0lvcmUrd0NoaWt0QUVray80MU81OHVieVFydEJvamt4?= =?utf-8?B?ZXVXcllSMUV1YWtRS2hqYm9lZnM2U2h3ZDNCY0FtYmtrV2xsSG9CNkVoSlJs?= =?utf-8?B?ZVpySklDWnI3Z0tMazlzWkdOcGQrNjhSZGpldTcxUm42cVVVZW53cXh1N01P?= =?utf-8?B?ZENPbWJwVnV4eGlhc2hvMzQwWHFXckNYdk1YemtnajRjcTExOTZCV1hLemNU?= =?utf-8?B?UGpoS2tuWG5NZEZ1ZDBSZEtDSUhOWDlyREJ4WFpmWW44ZjFTTk5xV281S2FX?= =?utf-8?B?Smw0dXZNdVZlUkZLRzNFdHJhMTB1TUR2a2FiVVczSjhBTGREdVlVWkFzVlJD?= =?utf-8?Q?wIl9lGWFoX4TVZIIaOaUTWn5in?= X-Microsoft-Exchange-Diagnostics: 1;AMSPR05MB422;5:jBp+YiW7VpwH1C2gQOHNeFOjPrgRwf9YBjGpQCovuSul4/VbBOY0dvodW0/PthzbCGuxF8DlAI+bIeW9VaQjF5domervpXk2PBsN9ODKGPKjKhIPFdQ/h8TTf7JopY3HyM2u05gumSOV4TUpwFflww==;24:+IS/U7W9ZkRQtwGioXMAuxyUIy4Ua64NexD0SPoXyhiyOfXsvkmDksPKFUGGvZDi58s+i3o0ISfTLVTieaD4SMs3xbHcJTUQrHLrjYTGcK4= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Feb 2016 12:03:18.6824 (UTC) X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=a652971c-7d2e-4d9b-a6a4-d149256f461b;Ip=[193.47.165.134];Helo=[mtlcas13.mtl.com] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: AMSPR05MB422 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24/02/2016 18:16, Parav Pandit wrote: >>> + struct rdmacg_resource_pool *rpool; >>> + struct rdmacg_pool_info *pool_info = &device->pool_info; >>> + >>> + spin_lock(&cg->rpool_list_lock); >>> + rpool = find_cg_rpool_locked(cg, device); >> Is it possible for rpool to be NULL? >> > Unlikely, unless we have but in cgroup implementation. > It may be worth to add WARN_ON and return from here to avoid kernel crash. Sounds good. >>> +static int charge_cg_resource(struct rdma_cgroup *cg, >>> + struct rdmacg_device *device, >>> + int index, int num) >>> +{ >>> + struct rdmacg_resource_pool *rpool; >>> + s64 new; >>> + int ret = 0; >>> + >>> +retry: >>> + spin_lock(&cg->rpool_list_lock); >>> + rpool = find_cg_rpool_locked(cg, device); >>> + if (!rpool) { >>> + spin_unlock(&cg->rpool_list_lock); >>> + ret = alloc_cg_rpool(cg, device); >>> + if (ret) >>> + goto err; >>> + else >>> + goto retry; >> Instead of retrying after allocation of a new rpool, why not just return the >> newly allocated rpool (or the existing one) from alloc_cg_rpool? > > It can be done, but locking semantics just becomes difficult to > review/maintain with that where alloc_cg_rpool will unlock and lock > conditionally later on. Maybe I'm missing something, but couldn't you simply lock rpool_list_lock inside alloc_cg_rpool()? It already does that around its call to find_cg_rpool_locked() and the insertion to cg_list. > This path will be hit anyway on first allocation typically. Once > application is warm up, it will be unlikely to enter here. > I should change if(!rpool) to if (unlikely(!rpool)). Theoretically the new allocated rpool can be released again by the time you get to the second call to find_cg_rpool_locked(). >>> + spin_lock(&cg->rpool_list_lock); >>> + rpool = find_cg_rpool_locked(cg, device); >>> + if (!rpool) { >>> + spin_unlock(&cg->rpool_list_lock); >>> + ret = alloc_cg_rpool(cg, device); >>> + if (ret) >>> + goto opt_err; >>> + else >>> + goto retry; >> You can avoid the retry here too. Perhaps this can go into a function. >> > In v5 I had wrapper around code which used to similar hiding using > get_cg_rpool and put_cg_rpool helper functions. > But Tejun was of opinion that I should have locks outside of all those > functions. With that approach, this is done. > So I think its ok. to have it this way. I thought that was about functions that only locked the lock, called the find function, and released the lock. What I'm suggesting is to have one function that does "lock + find + allocate if needed + unlock", and another function that does (under caller's lock) "check ref count + check max count + release rpool". >>> + } >>> + >>> + /* now set the new limits of the rpool */ >>> + while (enables) { >>> + /* if user set the limit, enables bit is set */ >>> + if (enables & BIT(i)) { >>> + enables &= ~BIT(i); >>> + set_resource_limit(rpool, i, new_limits[i]); >>> + } >>> + i++; >>> + } >>> + if (rpool->refcnt == 0 && >>> + rpool->num_max_cnt == pool_info->table_len) { >>> + /* >>> + * No user of the rpool and all entries are >>> + * set to max, so safe to delete this rpool. >>> + */ >>> + list_del(&rpool->cg_list); >>> + spin_unlock(&cg->rpool_list_lock); >>> + free_cg_rpool(rpool); >>> + } else { >>> + spin_unlock(&cg->rpool_list_lock); >>> + } >> You should consider putting this piece of code in a function (the >> check of the reference counts and release of the rpool). >> > Yes. I did. Same as above comment. Also this function will have to > unlock. Its usually better to lock/unlock from same function level, > instead of locking at one level and unlocking from inside the > function. > Or > I should have > cg_rpool_cond_free_unlock() for above code (check of the reference > counts and release of the rpool)? It is confusing to lock and unlock in different contexts. Why not lock in the caller context? free_cg_rpool() can be called under rpool_list_lock, couldn't it? It locks device->rpool_lock, but uncharge_cg_resource() also locks both in the same order. Thanks, Haggai