From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752686AbcFUSdy (ORCPT ); Tue, 21 Jun 2016 14:33:54 -0400 Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:44104 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751999AbcFUSdv (ORCPT ); Tue, 21 Jun 2016 14:33:51 -0400 From: Kenny Yu To: Tejun Heo CC: "lizefan@huawei.com" , "hannes@cmpxchg.org" , "cyphar@cyphar.com" , "cgroups@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Kernel Team Subject: Re: [PATCH v3] cgroup: Add pids controller event when fork fails because of pid limit Thread-Topic: [PATCH v3] cgroup: Add pids controller event when fork fails because of pid limit Thread-Index: AQHRy94Fy1nePRYnm0O4ykrQ3WVYkp/0KF0A//+N2oA= Date: Tue, 21 Jun 2016 17:23:40 +0000 Message-ID: <9874B6F0-99B8-4739-B099-B7EEE99478F3@fb.com> References: <20160621163402.GI3262@mtj.duckdns.org> <1466528198-3416939-1-git-send-email-kennyyu@fb.com> <20160621171212.GL3262@mtj.duckdns.org> In-Reply-To: <20160621171212.GL3262@mtj.duckdns.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [199.201.65.130] x-ms-office365-filtering-correlation-id: 710046fd-4f0b-425e-aede-08d399f8c98c x-microsoft-exchange-diagnostics: 1;CY1PR15MB0172;20:VWCdihzJVoEkZde13dz+ClMbQNy2Hofjyf0TwFafHfLJZftuZxOP4sNWY7S+lmnuEItfd7d9tm9p+HNEvRJdc0dpl5VSwk2QJLeKcPA5Gw1zwlH7nN4Uvx1TuDBphqTXG27t6gdhY3C3Osp+gWJWFNvotV0ZpJFTMxYN4euNHwA= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR15MB0172; x-ld-processed: 8ae927fe-1255-47a7-a2af-5f3a069daaa2,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001);SRVR:CY1PR15MB0172;BCL:0;PCL:0;RULEID:;SRVR:CY1PR15MB0172; x-forefront-prvs: 098076C36C x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(7916002)(51914003)(199003)(24454002)(377454003)(189002)(6116002)(102836003)(3846002)(8936002)(99286002)(7846002)(15975445007)(2906002)(77096005)(10400500002)(3280700002)(11100500001)(5002640100001)(66066001)(68736007)(122556002)(36756003)(92566002)(586003)(189998001)(50986999)(76176999)(87936001)(106116001)(8676002)(2950100001)(105586002)(2900100001)(97736004)(110136002)(33656002)(86362001)(81166006)(81156014)(54356999)(83716003)(82746002)(106356001)(101416001)(3660700001)(4326007)(19580405001)(19580395003)(7736002)(21314002)(104396002);DIR:OUT;SFP:1102;SCL:1;SRVR:CY1PR15MB0172;H:CY1PR15MB0169.namprd15.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: <9F61BFCA10F55047AAF7E0935E7D8F93@namprd15.prod.outlook.com> MIME-Version: 1.0 X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jun 2016 17:23:40.0340 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR15MB0172 X-OriginatorOrg: fb.com X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-06-21_09:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u5LIXxhU003909 Thanks for the feedback Tejun! On 6/21/16, 1:12 PM, "Tejun Heo" wrote: >Hello, > >Just a couple nits. > >On Tue, Jun 21, 2016 at 09:56:38AM -0700, Kenny Yu wrote: >> Summary: > >No need for "Summary:" tag. > >> This patch adds more visibility into the pids controller when the controller >> rejects a fork request. Whenever fork fails because the limit on the number of >> pids in the cgroup is reached, the controller will log this and also notify the >> newly added cgroups events file. The `max` key in the events file represents >> the number of times fork failed because of the pids controller. >> >> This change also adds an atomic boolean to prevent logging too much (e.g. a fork >> bomb). The message is logged once per cgroup until the next time the pids limit >> changes. > >The above paragraph isn't uptodate anymore. Thanks! Will change. > >> @@ -213,10 +220,23 @@ static int pids_can_fork(struct task_struct *task) >> { >> struct cgroup_subsys_state *css; >> struct pids_cgroup *pids; >> + int err; >> + int events_limit; >> >> css = task_css_check(current, pids_cgrp_id, true); >> pids = css_pids(css); >> - return pids_try_charge(pids, 1); >> + err = pids_try_charge(pids, 1); >> + if (err) { >> + events_limit = atomic64_inc_return(&pids->events_limit); >> + cgroup_file_notify(&pids->events_file); >> + /* Only log the first time events_limit is incremented. */ >> + if (events_limit == 1) { >> + pr_info("cgroup: fork rejected by pids controller in "); >> + pr_cont_cgroup_path(task_cgroup(current, pids_cgrp_id)); >> + pr_cont("\n"); >> + } >> + } >> + return err; >> } > >It'd be better to use atomic64_inc_and_test() instead. > > if (err) { > if (atomic64_inc_and_test()) { > pr_xxx...; > } > cgroup_file_notify(&pids->events_file); > } > According to the docs https://www.kernel.org/doc/Documentation/atomic_ops.txt , it looks like atomic_inc_and_test returns "a boolean indicating whether the resulting counter value was zero or not", which will only happen when the counter goes from negative to 0. I'll keep it as atomic_inc_return and get rid of the temp variable. >Thanks. > >-- >tejun Thanks, Kenny