Message ID | 5A2FE5ED.1080300@arm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 63491 invoked by alias); 12 Dec 2017 14:21:42 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 63437 invoked by uid 89); 12 Dec 2017 14:21:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LOTSOFHASH, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: EUR03-VE1-obe.outbound.protection.outlook.com Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Szabolcs.Nagy@arm.com; Message-ID: <5A2FE5ED.1080300@arm.com> Date: Tue, 12 Dec 2017 14:21:33 +0000 From: Szabolcs Nagy <szabolcs.nagy@arm.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: GNU C Library <libc-alpha@sourceware.org> CC: nd@arm.com Subject: [PATCH][BZ #11787] Fix stack guard size accounting Content-Type: multipart/mixed; boundary="------------090805030809030301070609" X-ClientProxiedBy: DB6PR07CA0083.eurprd07.prod.outlook.com (2603:10a6:6:2b::21) To DB6PR0802MB2485.eurprd08.prod.outlook.com (2603:10a6:4:9b::23) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 841e4b5c-d9aa-40e5-6595-08d5416ba671 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(5600026)(4604075)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(48565401081)(2017052603307)(49563074); SRVR:DB6PR0802MB2485; X-Microsoft-Exchange-Diagnostics: 1; DB6PR0802MB2485; 3:Svo44YyyD9gAZF8iCV8Zc7HmKXyOF13RgYl+9iG4KeZYcoLtYNnmWPYhQUGKgpiupn31vqfw3a2a4YDrPfCdcbtC1hLpB1otTvV8N1E+HvR+6ymJQ8RjfN1hGthn94Pi9cPD0T0NbIhZFFk0AClEaUQ+u0tWI7V/5LL/aeU1oMdpVybk1N67CmzQyx7+QC5kk5xLRkJ4ISZyAycMnZctQ07cIMYxHjAGT4GtkcjHQc/9EOvztTvUEnTVEpJYYgey; 25:0JXcnL8nCt7rUZoV3otiK3CGqneecwICoqSbKhC+ArnMBzFG/nY+1TUXGiFY7xYDjCW9wOzLS4xldEaHextur/nbolSO6kxqbEI11GcPgLOXoXmASdADCBeQIkTv2yzdGI7u2BCMDgFI6odLwf61kSJkoPHIpdsziEa9f7bF2x+ax3+D+zyV7z8dopD4ZyC5NPGvC80s0fqePpAll514X8hwO3T82bAnga4bHqDvvLRoCMECJzGj+tGtCqxck8K/SYAu1EZ97WjKh7GzJtOawKt4IFCsOBMqpn8Dw+J1c/DseQ00JSOsfYalESXi7Y+zwsyxptOTtPz8CJIiJfEyQw==; 31:nKWGm0vjvXs7EgIXQA28gKcZwB2yOA4SHvNhhtu6X7Fj/0c4onbYQxoco3FypksUZSVhh3ItXsuIrnsSxbRUl8Ld7I3tMAMA/fZNdrwzLbYMWZyL1Yf4835zZioPukzHX6LnLXwBIgZ8qTt8C8p6vcnaBzNtq9x9+ve9+rNoYMP84byv2XTdf8xJL4JgloK6u2gQP47fdmy+cBItiS1A/NFmVsV4L3oc/cg1wbkfO0o= X-MS-TrafficTypeDiagnostic: DB6PR0802MB2485: NoDisclaimer: True X-Microsoft-Exchange-Diagnostics: 1; DB6PR0802MB2485; 20:AmPNi1xJ/IEHXhCKXWFsoxeU3bCPGdEvwezC+zHbh9d4Ea/xEEePPXdKtIN9h4C/K2eBzJ/fI2xJKQMLfN52Z1hqJioHqpbzYlxA/rDij3XWUdGbpP1y8YcAKUFDiiSRE5Bt/mC461aR6IQND+y97n6WCSqSxyT58MYUIr22h+I=; 4:i1WIIFOXWx4a3vYLghveCg/lrLkdO8weZSVhbU35dV+pbarnYxvyipsN7u8xGYCGjSBjprRzxdsbGJfJkRqckrNx/5774MNgrRuDtmlYUSBZ/FLnUsjE955o7TxErXTx3pgPoZYrH8yRkBx3DikmeykaCteif9+ywcA/BDpulIVcbLmuQ4syyxoizelF3d1nO6h6i3FMR5pkBy1cwvq1fUygDvAzRFe8yGxzTfWa/82gV4mvd+Kq+t4ts+lsxvPkHQW+fNOxebBP9jJg6qX2eaDhRstFfrUwDF5SIhSc/T5VBCZEDktCL3wMq/VSetPDpEd/Kvn89LvdxC7/RNMW3eRRqG3jvpwV6PubcXVlFNo= X-Microsoft-Antispam-PRVS: <DB6PR0802MB2485880DC9BB019C9289B6D1ED340@DB6PR0802MB2485.eurprd08.prod.outlook.com> X-Exchange-Antispam-Report-Test: UriScan:(180628864354917)(211171220733660); X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(102415395)(6040450)(2401047)(5005006)(8121501046)(3002001)(93006095)(93001095)(3231023)(10201501046)(6055026)(6041248)(20161123560025)(20161123558100)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123562025)(20161123555025)(6072148)(201708071742011); SRVR:DB6PR0802MB2485; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:DB6PR0802MB2485; X-Forefront-PRVS: 051900244E X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(6049001)(376002)(346002)(39860400002)(366004)(189003)(199004)(377424004)(5890100001)(16586007)(59896002)(53936002)(58126008)(16576012)(8936002)(4610100001)(5000100001)(316002)(6916009)(33656002)(36756003)(105586002)(80316001)(15650500001)(106356001)(2476003)(3846002)(25786009)(65816011)(6116002)(84326002)(33964004)(4326008)(65956001)(87266011)(72206003)(2906002)(68736007)(4001150100001)(478600001)(64126003)(8676002)(83506002)(5660300001)(81156014)(21490400003)(568964002)(270700001)(66066001)(97736004)(77096006)(6486002)(81166006)(52116002)(86362001)(65806001)(16526018)(305945005)(7736002); DIR:OUT; SFP:1101; SCL:1; SRVR:DB6PR0802MB2485; H:[10.2.206.69]; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; Received-SPF: None (protection.outlook.com: arm.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1; DB6PR0802MB2485; 23:nmjsKHO1WZc1fVi8WZTXbBJ2e8mTIn0uIuzA1Yo?= =?us-ascii?Q?Rd9ho8Nv02fNt8+0xgE62tvdtdzLFonGJNtHjCB76mb4wF03y/LGkw9Po+Fu?= =?us-ascii?Q?9vk9+TIA+tcKfX2vEHVJGINExnlOR7+8NYEDmHdciib+m8SXChIhuhdXNeTY?= =?us-ascii?Q?PMe5LKn4metQ9w0QarCXp68AwpVRBeiCIVX/PKjWMD9iszy+yjfbfRhO0SGm?= =?us-ascii?Q?CPfbUcppshGguO4huxP+/er+kQSfb/PMnTMfMlv4SGozq17+OkllVQlL01W2?= =?us-ascii?Q?D86tv3zcC/ixy+46/mufMy7/IH97v0t/DjdikJUeKtNuNTUT6w2ngUF24noo?= =?us-ascii?Q?/IN3j2tHhh84kndG5pThr75E/coMBg06eypHJWM8Fdm9YehvMwWI/zZ2BApq?= =?us-ascii?Q?GZmnbT6QyFg7ZjYOjZOh0YPkM6tMOCZQvY19Goa0wdymI8KfoQWf2zE7BE7T?= =?us-ascii?Q?NitiXakCATClT8zZPHzeDmGanWhXVWJp6GiXTM3dca+icWWPLvBBhCXVQUL3?= =?us-ascii?Q?tKX81z6BzcgJHCGlVvBRLvxaUb8HEzKnBcN+uXCo+KaHy4fFfbD2rxc12DOo?= =?us-ascii?Q?pbJvBtZRXPOJHV61J2LQaDPdnnPnU06bhkS9wP8bZgNG80FGN2t4boLotvZj?= =?us-ascii?Q?NdI9KDMboDAoqfIUrrlLQmKOaiZM5c9bEwhbuASIUCnilD+Dnk2AS3CEJ2Us?= =?us-ascii?Q?/wRap3aJPxjtlQoNizaCbd1iOjtD/2OaCHkPNcGaWfSzmi+bz/4wjO5IAi5f?= =?us-ascii?Q?KIU96P4C+oO7ryjRs1wktZlL6UeLuphWpxB5vgu/txEOIiRU/DCtavNRF14E?= =?us-ascii?Q?jXWfMcxShB9Dw343Qv+GkJ4+KDs3rRb8L3Zl3a+1TEAtOSbv7xaqH81UMwBM?= =?us-ascii?Q?46zLSQzvkOgLAUypFNJ//4H/XYJUCOOOQo9gEsGW5XlbFIDcWNsR83gnHbTL?= =?us-ascii?Q?YHZ8Vq2OHhKob+LwcO5VLyfaRV+eJJ+Kn+v/6/06NSEUfRxdUjH2Hz9drRRI?= =?us-ascii?Q?Jjl8YRqMeVY4s2AeouY0okkuj7ShW8BDIOQ2X4JIwY2Ovtzahm7YVTHGKSUl?= =?us-ascii?Q?pm7br5VuTPKxqtZ5LCA1nB9bIzIVm+pBP56letY6m/mUwDwV7q2Y4dfbIVmo?= =?us-ascii?Q?plzHzmDWPLZUnqw/RgankgLS0ECPcbWJy4uyBUfHTnbp3Sf5TGbmUQ+wlmCg?= =?us-ascii?Q?SXY56E28IoWmLE6QIfU6901oahh8sf79opTlfQ564jewn1XM1GCyeal3Izq5?= =?us-ascii?Q?usBhRuQesuIf1HDIqkCFus90Zxr/xA1VK/Ed0SjPAj9esqv19v9wdcfie3US?= =?us-ascii?Q?1U3ZkobAkYrBf3acFaMN3D0+hbTOkCp7J88G8X8N2jV9EQCSPznSWzicS822?= =?us-ascii?Q?0CxInZTGHryiDjhz7f8u/qEHkHMdKhmxDp62miduh//Uuc2aw?= X-Microsoft-Exchange-Diagnostics: 1; DB6PR0802MB2485; 6:UUpmIAljXGB6mCBEMVTHCEzkOt3wUIUXW30VgI3SbVt9hCjVIt1gNOivF09stNTLF93WvohGheyqd+9vuvc7r28CN38XvWX7djG+0b5+AdooSrQeqV+QswNSBZr5krfyoVN6r33aWmx3yammXUDp7BQSyBwkd3Cw9jVvW6ezMUe9GzaIaNC70QRc/enaUFm0ldS7eSFPLnfkh0bUM+JbNRhFnKQJB1xRW1JDmk064Md8biov1kUpa6Seg3ftd1PkrOV3IcIJb2I7xRr8Agy78xLE3hDW2hEyOeGdjWcQmewfchUKKLa+lGUC/W6vw5DetOd7dkGzDxs3Lpne8ohDWdm50w+In5vSJ8Rugt8gySk=; 5:sZUiqCtj8mBJfNzKRogBLqAIYYydusLZoOo7RusTCXCCWlHqD0go75Bio5WfSUtGV8oeBRJTM9ewTIUFYVE0qKQsrOCYqFvk7CyFkaLTPBkAp5XIE5u3Mq9iXJX+Bmb9cbFxmUW1zF2eN5p06qPSUsglMUXUdr1pTAXJ90xa0S0=; 24:2SiyuSag80o90gDFCn5ce8tp59xfz46QPYBZje7T+zLTybAroqqIV0HyGhKIUOCIsB/hqQxrIWcwlVoF2FCUZ3ZZZbbO3oUKUB+SkKHR4nM=; 7:piJ/mlrPd7aDXfDIIociqACRJsyvUcHvMhdhKfeHOm8e4lk6m8flzXZYWOztCdiAYxG16gs1cXPEsIUTsCJdq3A44U4g7nq3V4mdLWJMkS2SmgQ3+PBaF7UWr+zCc4C+VyI4ZURGORpdrH+4sOYGs/pWecQJa0ffAXjaJeTEu2LhF3+f3T8yb5TkHUuVukivLw/pPcDYxI9bQnKBCA30gc5CAnsGcCDM39wSg+WkAO/hQgSwftdLOzsUuHOvI/qL SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2017 14:21:35.1924 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 841e4b5c-d9aa-40e5-6595-08d5416ba671 X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2485 |
Commit Message
Szabolcs Nagy
Dec. 12, 2017, 2:21 p.m. UTC
Previously if user requested S stack and G guard when creating a thread, the total mapping was S and the actual available stack was S - G - static_tls, which is not what the user requested. This patch fixes the guard size accounting by pretending the user requested S + G stack. This way all later logic works out except when reporting the user requested stack size (pthread_getattr_np) or when computing the minimal stack size (__pthread_get_minstack). Normally this will increase thread stack allocations by one page. TLS accounting is not affected, that will require a separate fix. 2017-12-12 Szabolcs Nagy <szabolcs.nagy@arm.com> [BZ #11787] * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from stacksize. * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise.
Comments
On 12/12/17 14:21, Szabolcs Nagy wrote: > Previously if user requested S stack and G guard when creating a > thread, the total mapping was S and the actual available stack was > S - G - static_tls, which is not what the user requested. > > This patch fixes the guard size accounting by pretending the user > requested S + G stack. This way all later logic works out except > when reporting the user requested stack size (pthread_getattr_np) > or when computing the minimal stack size (__pthread_get_minstack). > > Normally this will increase thread stack allocations by one page. > TLS accounting is not affected, that will require a separate fix. > > 2017-12-12 Szabolcs Nagy <szabolcs.nagy@arm.com> > > [BZ #11787] > * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. > * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from > stacksize. > * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. > ping.
On 12/12/2017 03:21 PM, Szabolcs Nagy wrote: > Previously if user requested S stack and G guard when creating a > thread, the total mapping was S and the actual available stack was > S - G - static_tls, which is not what the user requested. > > This patch fixes the guard size accounting by pretending the user > requested S + G stack. This way all later logic works out except > when reporting the user requested stack size (pthread_getattr_np) > or when computing the minimal stack size (__pthread_get_minstack). > > Normally this will increase thread stack allocations by one page. > TLS accounting is not affected, that will require a separate fix. Should this fix use a separate bug number? > 2017-12-12 Szabolcs Nagy<szabolcs.nagy@arm.com> > > [BZ #11787] > * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. > * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from > stacksize. > * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. Patch looks good in general. The computation for stackblock_size looks right to me. > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index 1cc789319564b468cf07bdb1304b27dc5a91e86f..9525322b1f92bb34aa21dcab28566aecd7434e90 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -532,6 +532,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, > /* Make sure the size of the stack is enough for the guard and > eventually the thread descriptor. */ > guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1; > + size += guardsize; > if (__builtin_expect (size < ((guardsize + __static_tls_size > + MINIMAL_REST_STACK + pagesize_m1) > & ~pagesize_m1), I wonder if we should add an overflow check there and return EINVAL if guardsize < attr->guardsize || size + guardsize < guardsize Thanks, Florian
On 20/12/17 13:09, Florian Weimer wrote: > On 12/12/2017 03:21 PM, Szabolcs Nagy wrote: >> Previously if user requested S stack and G guard when creating a >> thread, the total mapping was S and the actual available stack was >> S - G - static_tls, which is not what the user requested. >> >> This patch fixes the guard size accounting by pretending the user >> requested S + G stack. This way all later logic works out except >> when reporting the user requested stack size (pthread_getattr_np) >> or when computing the minimal stack size (__pthread_get_minstack). >> >> Normally this will increase thread stack allocations by one page. >> TLS accounting is not affected, that will require a separate fix. > > Should this fix use a separate bug number? > i can open a separate bug. >> 2017-12-12 Szabolcs Nagy<szabolcs.nagy@arm.com> >> >> [BZ #11787] >> * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. >> * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from >> stacksize. >> * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. > > Patch looks good in general. The computation for stackblock_size looks right to me. > >> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c >> index 1cc789319564b468cf07bdb1304b27dc5a91e86f..9525322b1f92bb34aa21dcab28566aecd7434e90 100644 >> --- a/nptl/allocatestack.c >> +++ b/nptl/allocatestack.c >> @@ -532,6 +532,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, >> /* Make sure the size of the stack is enough for the guard and >> eventually the thread descriptor. */ >> guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1; >> + size += guardsize; >> if (__builtin_expect (size < ((guardsize + __static_tls_size >> + MINIMAL_REST_STACK + pagesize_m1) >> & ~pagesize_m1), > > I wonder if we should add an overflow check there and return EINVAL if > > guardsize < attr->guardsize || size + guardsize < guardsize > makes sense. but i saw several overflowing arithmetics around stack computation.
On 12/12/2017 06:21 AM, Szabolcs Nagy wrote: > Previously if user requested S stack and G guard when creating a > thread, the total mapping was S and the actual available stack was > S - G - static_tls, which is not what the user requested. > > This patch fixes the guard size accounting by pretending the user > requested S + G stack. This way all later logic works out except > when reporting the user requested stack size (pthread_getattr_np) > or when computing the minimal stack size (__pthread_get_minstack). > > Normally this will increase thread stack allocations by one page. > TLS accounting is not affected, that will require a separate fix. > > 2017-12-12 Szabolcs Nagy <szabolcs.nagy@arm.com> > > [BZ #11787] > * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. > * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from > stacksize. > * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. LGTM with the refactoring fixed. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > > > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index 1cc789319564b468cf07bdb1304b27dc5a91e86f..9525322b1f92bb34aa21dcab28566aecd7434e90 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -532,6 +532,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, > /* Make sure the size of the stack is enough for the guard and > eventually the thread descriptor. */ > guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1; > + size += guardsize; OK. > if (__builtin_expect (size < ((guardsize + __static_tls_size > + MINIMAL_REST_STACK + pagesize_m1) > & ~pagesize_m1), > diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c > index 869e926f17aa218ab0b122eeda935069ef419ae5..e5c0bdfbebbcc501e4135280f09d55d021943363 100644 > --- a/nptl/nptl-init.c > +++ b/nptl/nptl-init.c > @@ -473,8 +473,5 @@ strong_alias (__pthread_initialize_minimal_internal, > size_t > __pthread_get_minstack (const pthread_attr_t *attr) > { > - struct pthread_attr *iattr = (struct pthread_attr *) attr; > - > - return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN > - + iattr->guardsize); > + return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN; > } We no longer need pthread_attr_t. Please refactor. > diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c > index 06093b3d9270c2fcaa5af191852c9eca25dd506f..c5f82f8d22c74fd9e2433dff0a0d98d78e272484 100644 > --- a/nptl/pthread_getattr_np.c > +++ b/nptl/pthread_getattr_np.c > @@ -57,9 +57,10 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr) > /* The sizes are subject to alignment. */ > if (__glibc_likely (thread->stackblock != NULL)) > { > - iattr->stacksize = thread->stackblock_size; > + iattr->stacksize = thread->stackblock_size - thread->guardsize; OK. > #if _STACK_GROWS_DOWN > - iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize; > + iattr->stackaddr = (char *) thread->stackblock > + + thread->stackblock_size; OK. > #else > iattr->stackaddr = (char *) thread->stackblock; > #endif
On 12/20/2017 06:46 PM, Carlos O'Donell wrote: >> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c >> index 869e926f17aa218ab0b122eeda935069ef419ae5..e5c0bdfbebbcc501e4135280f09d55d021943363 100644 >> --- a/nptl/nptl-init.c >> +++ b/nptl/nptl-init.c >> @@ -473,8 +473,5 @@ strong_alias (__pthread_initialize_minimal_internal, >> size_t >> __pthread_get_minstack (const pthread_attr_t *attr) >> { >> - struct pthread_attr *iattr = (struct pthread_attr *) attr; >> - >> - return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN >> - + iattr->guardsize); >> + return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN; >> } > We no longer need pthread_attr_t. Please refactor. Please move this into a separate patch. The function also needs to be renamed because there is code out there which attempts to call it if it exists (notably, in Firefox and Rust—you might not see it because some refactoring broke it and switched from C linkage to Rust linkage, so the function is never found). Thanks, Florian
On 12/12/2017 06:21 AM, Szabolcs Nagy wrote: > Previously if user requested S stack and G guard when creating a > thread, the total mapping was S and the actual available stack was > S - G - static_tls, which is not what the user requested. > > This patch fixes the guard size accounting by pretending the user > requested S + G stack. This way all later logic works out except > when reporting the user requested stack size (pthread_getattr_np) > or when computing the minimal stack size (__pthread_get_minstack). > > Normally this will increase thread stack allocations by one page. > TLS accounting is not affected, that will require a separate fix. > > 2017-12-12 Szabolcs Nagy <szabolcs.nagy@arm.com> > > [BZ #11787] > * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. > * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from > stacksize. > * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. > Any status on this? I would like to get this in as soon as possible because the current Intel fxsave/xsave/xsavec consume more stack than before, and we have had at least one report of an application failing because of the additional stack usage (breaks ntpd helper threads running with PTHREAD_STACK_MIN, see the other discussion about what good is PTHREADS_STACK_MIN). It looks like a next step would be: * Split into two patches. * First patch is as you intended (and can be checked in right away) * Second patch removed the unused pthread_attr_r and renames __pthread_get_minstack to avoid usage by other programs.
On 01/08/2018 04:20 PM, Carlos O'Donell wrote: > On 12/12/2017 06:21 AM, Szabolcs Nagy wrote: >> Previously if user requested S stack and G guard when creating a >> thread, the total mapping was S and the actual available stack was >> S - G - static_tls, which is not what the user requested. >> >> This patch fixes the guard size accounting by pretending the user >> requested S + G stack. This way all later logic works out except >> when reporting the user requested stack size (pthread_getattr_np) >> or when computing the minimal stack size (__pthread_get_minstack). >> >> Normally this will increase thread stack allocations by one page. >> TLS accounting is not affected, that will require a separate fix. >> >> 2017-12-12 Szabolcs Nagy <szabolcs.nagy@arm.com> >> >> [BZ #11787] >> * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. >> * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from >> stacksize. >> * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. >> > > Any status on this? > > I would like to get this in as soon as possible because the current > Intel fxsave/xsave/xsavec consume more stack than before, and we have > had at least one report of an application failing because of the additional > stack usage (breaks ntpd helper threads running with PTHREAD_STACK_MIN, see > the other discussion about what good is PTHREADS_STACK_MIN). > > It looks like a next step would be: > > * Split into two patches. > * First patch is as you intended (and can be checked in right away) Agreed. > * Second patch removed the unused pthread_attr_r and renames __pthread_get_minstack > to avoid usage by other programs. The proposed second patch *still* breaks Rust programs with large static TLS data because it will switch to PTHREAD_STACK_MIN. We should do this only after __pthread_get_minstack turns constant (that is, returns PTHREAD_STACK_MIN). Thanks, Florian
On 01/08/2018 07:22 AM, Florian Weimer wrote: >> * Second patch removed the unused pthread_attr_r and renames __pthread_get_minstack >> to avoid usage by other programs. > > The proposed second patch *still* breaks Rust programs with large > static TLS data because it will switch to PTHREAD_STACK_MIN. We > should do this only after __pthread_get_minstack turns constant (that > is, returns PTHREAD_STACK_MIN). I had not considered that. Yes, applications are basically working around bug 11787 by calling __ptrhead_get_minstack. Perhaps you are right, until we disentangle TLS, such applications need the workaround. OK, I'm happy with the patch going in as-is then, and leaving the second half of the cleanup until later.
On 08/01/18 15:22, Florian Weimer wrote: > On 01/08/2018 04:20 PM, Carlos O'Donell wrote: >> On 12/12/2017 06:21 AM, Szabolcs Nagy wrote: >>> Previously if user requested S stack and G guard when creating a >>> thread, the total mapping was S and the actual available stack was >>> S - G - static_tls, which is not what the user requested. >>> >>> This patch fixes the guard size accounting by pretending the user >>> requested S + G stack. This way all later logic works out except >>> when reporting the user requested stack size (pthread_getattr_np) >>> or when computing the minimal stack size (__pthread_get_minstack). >>> >>> Normally this will increase thread stack allocations by one page. >>> TLS accounting is not affected, that will require a separate fix. >>> >>> 2017-12-12 Szabolcs Nagy <szabolcs.nagy@arm.com> >>> >>> [BZ #11787] >>> * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize. >>> * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from >>> stacksize. >>> * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise. >>> >> >> Any status on this? >> >> I would like to get this in as soon as possible because the current >> Intel fxsave/xsave/xsavec consume more stack than before, and we have >> had at least one report of an application failing because of the additional >> stack usage (breaks ntpd helper threads running with PTHREAD_STACK_MIN, see >> the other discussion about what good is PTHREADS_STACK_MIN). >> >> It looks like a next step would be: >> >> * Split into two patches. >> * First patch is as you intended (and can be checked in right away) > > Agreed. > sorry i did not work on this while on holiday will submit an updated patch >> * Second patch removed the unused pthread_attr_r and renames __pthread_get_minstack >> to avoid usage by other programs. > > The proposed second patch *still* breaks Rust programs with large static TLS data because it will switch to > PTHREAD_STACK_MIN. We should do this only after __pthread_get_minstack turns constant (that is, returns > PTHREAD_STACK_MIN). i knew __pthread_get_minstack was used outside of glibc but thought removing the guardsize was safe since we add it back later. the patch increases stack allocations by +guardsize, so if user computes its stack via __pthread_get_minstack that may now return a smaller value to get the same stack allocation as before in the end.
On 01/08/2018 04:41 PM, Szabolcs Nagy wrote: > i knew __pthread_get_minstack was used outside of glibc > but thought removing the guardsize was safe since we > add it back later. I agree that the patch as posted should be safe because the return value of __pthread_get_minstack does not change. However, renaming the function would trigger the Rust default implementation, which is PTHREAD_STACK_MIN, and that doesn't take the static TLS size into account. This is why I think we shouldn't do the renaming right now. Thanks, Florian
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index 1cc789319564b468cf07bdb1304b27dc5a91e86f..9525322b1f92bb34aa21dcab28566aecd7434e90 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -532,6 +532,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, /* Make sure the size of the stack is enough for the guard and eventually the thread descriptor. */ guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1; + size += guardsize; if (__builtin_expect (size < ((guardsize + __static_tls_size + MINIMAL_REST_STACK + pagesize_m1) & ~pagesize_m1), diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c index 869e926f17aa218ab0b122eeda935069ef419ae5..e5c0bdfbebbcc501e4135280f09d55d021943363 100644 --- a/nptl/nptl-init.c +++ b/nptl/nptl-init.c @@ -473,8 +473,5 @@ strong_alias (__pthread_initialize_minimal_internal, size_t __pthread_get_minstack (const pthread_attr_t *attr) { - struct pthread_attr *iattr = (struct pthread_attr *) attr; - - return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN - + iattr->guardsize); + return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN; } diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c index 06093b3d9270c2fcaa5af191852c9eca25dd506f..c5f82f8d22c74fd9e2433dff0a0d98d78e272484 100644 --- a/nptl/pthread_getattr_np.c +++ b/nptl/pthread_getattr_np.c @@ -57,9 +57,10 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr) /* The sizes are subject to alignment. */ if (__glibc_likely (thread->stackblock != NULL)) { - iattr->stacksize = thread->stackblock_size; + iattr->stacksize = thread->stackblock_size - thread->guardsize; #if _STACK_GROWS_DOWN - iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize; + iattr->stackaddr = (char *) thread->stackblock + + thread->stackblock_size; #else iattr->stackaddr = (char *) thread->stackblock; #endif