From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id C364F1F55B for ; Tue, 2 Jun 2020 03:47:56 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A906C383E836; Tue, 2 Jun 2020 03:47:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A906C383E836 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1591069675; bh=3hTqGWGFYsZGQYLg+skK+O6YD61s2drisWgjnZMPbbk=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=wc4ibMWw97bM0zpPLtdKfac/PxFSF8YN6VaPrblF1cARUdLyDSyTg3eakKXH9gyWj YEgIKMonEyOiSXh0xpL8GNOSY5+1FlRRffC2gH8fbjYS+P7E4lKMMHeC9mRwdY4qUC oOs4veX4ybRy9Oe7gG4L8ziice4bl8aKe9/E3P28= Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id 3690F383E829 for ; Tue, 2 Jun 2020 03:47:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3690F383E829 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-223-yJp8j0omNlGUj5kDg-9OkQ-1; Mon, 01 Jun 2020 23:47:48 -0400 X-MC-Unique: yJp8j0omNlGUj5kDg-9OkQ-1 Received: by mail-qt1-f200.google.com with SMTP id e8so12503116qtq.22 for ; Mon, 01 Jun 2020 20:47:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=3hTqGWGFYsZGQYLg+skK+O6YD61s2drisWgjnZMPbbk=; b=reZk4deviBCDNo1/MHcvq6xBimI7CO3C9kacUa1TtIdHQy5kgywFuEEYyZSEgBreFM sFdy21flNJV6O3fiRMr3qHF0vP5FCL7tGUpe+TK+rnbnpkbPGdF9ERMNm6336YNUMHx4 ydKOTK/R8npKpsL7Dg4TACsgs7zew4kSPdr7ZF50FK2HjhW8enmOkMvo5swD+ty5dqlt dxLzKdEEZ95xirCpxdA52zuJYONMGSI0jlzxBQJsxv4p8Ck+8QiO6YjQFLN8P+H3cDbL nat8pLebYxnNsomUswSpccpJDgfLVi0q3MlvGwQtND439az1C+/B6KLX2m/Hwp7JgSlY qM/Q== X-Gm-Message-State: AOAM531ymvgobTOfddZ72/CB+ZKy7sVbCesjqUgICpm8cPhcqjdo9B8D Z4jN9aVLNg9XtAELMk6F4XChCkn4gSQ41Obrfji7LAJFOpqYuFVSCoPpjsVcM/kCgWGcMxSop4g lmq17JAEZADTED/C3+/rO X-Received: by 2002:ac8:378f:: with SMTP id d15mr8550713qtc.136.1591069667427; Mon, 01 Jun 2020 20:47:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzNsaLLIeZCezBHsBlAfwK8I/hAfTkXqrEMMvUFQnisVTjTTNSikUVMNtdDSAq4L+GJbfhumg== X-Received: by 2002:ac8:378f:: with SMTP id d15mr8550694qtc.136.1591069666971; Mon, 01 Jun 2020 20:47:46 -0700 (PDT) Received: from [192.168.1.4] (198-84-170-103.cpe.teksavvy.com. [198.84.170.103]) by smtp.gmail.com with ESMTPSA id u7sm1256672qkb.7.2020.06.01.20.47.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 Jun 2020 20:47:46 -0700 (PDT) Subject: Re: [PATCH 16/19] nptl: Make pthread_attr_t dynamically extensible To: Florian Weimer , libc-alpha@sourceware.org References: <0372d63243979fb93d566609b913cf1ee3c15097.1589884403.git.fweimer@redhat.com> Organization: Red Hat Message-ID: <706407d4-e0a2-4405-8419-2b2299751317@redhat.com> Date: Mon, 1 Jun 2020 23:47:45 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <0372d63243979fb93d566609b913cf1ee3c15097.1589884403.git.fweimer@redhat.com> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Carlos O'Donell via Libc-alpha Reply-To: Carlos O'Donell Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" On 5/19/20 6:44 AM, Florian Weimer via Libc-alpha wrote: > This introduces the function __pthread_attr_extension to allocate the > extension space, which is freed by pthread_attr_destroy. OK for master with one typo fixed. Reviewed-by: Carlos O'Donell > --- > nptl/Makefile | 1 + > nptl/pthreadP.h | 6 +++++ > nptl/pthread_attr_copy.c | 16 +++++++------ > nptl/pthread_attr_destroy.c | 12 ++++++---- > nptl/pthread_attr_extension.c | 32 ++++++++++++++++++++++++++ > nptl/pthread_attr_getaffinity.c | 14 +++++------ > nptl/pthread_attr_setaffinity.c | 23 +++++++++++------- > nptl/pthread_create.c | 2 +- > sysdeps/nptl/internaltypes.h | 16 ++++++++++--- > sysdeps/unix/sysv/linux/createthread.c | 9 +++++--- > 10 files changed, 98 insertions(+), 33 deletions(-) > create mode 100644 nptl/pthread_attr_extension.c > > diff --git a/nptl/Makefile b/nptl/Makefile > index e5686b20ac..d6f12d0371 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -41,6 +41,7 @@ routines = \ > pthread_atfork \ > pthread_attr_copy \ > pthread_attr_destroy \ > + pthread_attr_extension \ OK. > pthread_attr_getdetachstate \ > pthread_attr_getinheritsched \ > pthread_attr_getschedparam \ > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index ed30b72923..7b3153594e 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -578,6 +578,12 @@ extern void __shm_directory_freeres (void) attribute_hidden; > > extern void __wait_lookup_done (void) attribute_hidden; > > +/* Allocates the extension space for ATTR. Returns an error code on > + memory allocation failure, zero on success. If ATTR already has an > + extension space, this function does nothing. */ > +int __pthread_attr_extension (struct pthread_attr *attr) attribute_hidden > + __attribute_warn_unused_result__; OK. Good comment. > + > #ifdef SHARED > # define PTHREAD_STATIC_FN_REQUIRE(name) > #else > diff --git a/nptl/pthread_attr_copy.c b/nptl/pthread_attr_copy.c > index 77a1a43eeb..eb29557505 100644 > --- a/nptl/pthread_attr_copy.c > +++ b/nptl/pthread_attr_copy.c > @@ -29,18 +29,20 @@ __pthread_attr_copy (pthread_attr_t *target, const pthread_attr_t *source) > temp.external = *source; > > /* Force new allocation. This function has full ownership of temp. */ > - temp.internal.cpuset = NULL; > - temp.internal.cpusetsize = 0; > + temp.internal.extension = NULL; OK. Init. > > int ret = 0; > > struct pthread_attr *isource = (struct pthread_attr *) source; > > - /* Propagate affinity mask information. */ > - if (isource->cpusetsize > 0) > - ret = __pthread_attr_setaffinity_np (&temp.external, > - isource->cpusetsize, > - isource->cpuset); > + if (isource->extension != NULL) > + { > + /* Propagate affinity mask information. */ > + if (isource->extension->cpusetsize > 0) > + ret = __pthread_attr_setaffinity_np (&temp.external, > + isource->extension->cpusetsize, > + isource->extension->cpuset); > + } OK. Use extension space. > > if (ret != 0) > { > diff --git a/nptl/pthread_attr_destroy.c b/nptl/pthread_attr_destroy.c > index 21f8026a2c..b6a3cca657 100644 > --- a/nptl/pthread_attr_destroy.c > +++ b/nptl/pthread_attr_destroy.c > @@ -30,12 +30,16 @@ __pthread_attr_destroy (pthread_attr_t *attr) > iattr = (struct pthread_attr *) attr; > > #if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_1) > - /* In old struct pthread_attr, neither next nor cpuset are > - present. */ > + /* In old struct pthread_attr, the extension member is missing. */ > if (__builtin_expect ((iattr->flags & ATTR_FLAG_OLDATTR), 0) == 0) > #endif > - /* The affinity CPU set might be allocated dynamically. */ > - free (iattr->cpuset); > + { > + if (iattr->extension != NULL) > + { > + free (iattr->extension->cpuset); > + free (iattr->extension); OK. > + } > + } > > return 0; > } > diff --git a/nptl/pthread_attr_extension.c b/nptl/pthread_attr_extension.c > new file mode 100644 > index 0000000000..e8521b1556 > --- /dev/null > +++ b/nptl/pthread_attr_extension.c > @@ -0,0 +1,32 @@ > +/* Allocate the extension space for strucht pthread_attr. Typo: s/strucht/struct/g > + Copyright (C) 2020 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > +#include > + > +int > + __pthread_attr_extension (struct pthread_attr *attr) > +{ > + if (attr->extension != NULL) > + return 0; > + attr->extension = calloc (sizeof (*attr->extension), 1); > + if (attr->extension == NULL) > + return errno; > + return 0; > +} OK. Dynamically allocate space for the extension via calloc. > diff --git a/nptl/pthread_attr_getaffinity.c b/nptl/pthread_attr_getaffinity.c > index 212c1f7c0a..9483f69ddc 100644 > --- a/nptl/pthread_attr_getaffinity.c > +++ b/nptl/pthread_attr_getaffinity.c > @@ -33,22 +33,22 @@ __pthread_attr_getaffinity_new (const pthread_attr_t *attr, size_t cpusetsize, > > iattr = (const struct pthread_attr *) attr; > > - if (iattr->cpuset != NULL) > + if (iattr->extension != NULL && iattr->extension->cpuset != NULL) OK. > { > /* Check whether there are any bits set beyond the limits > the user requested. */ > - for (size_t cnt = cpusetsize; cnt < iattr->cpusetsize; ++cnt) > - if (((char *) iattr->cpuset)[cnt] != 0) > + for (size_t cnt = cpusetsize; cnt < iattr->extension->cpusetsize; ++cnt) > + if (((char *) iattr->extension->cpuset)[cnt] != 0) OK. > return EINVAL; > > /* Copy over the cpuset from the thread attribute object. Limit the copy > to the minimum of the source and destination sizes to prevent a buffer > overrun. If the destination is larger, fill the remaining space with > zeroes. */ > - void *p = mempcpy (cpuset, iattr->cpuset, > - MIN (iattr->cpusetsize, cpusetsize)); > - if (cpusetsize > iattr->cpusetsize) > - memset (p, '\0', cpusetsize - iattr->cpusetsize); > + void *p = mempcpy (cpuset, iattr->extension->cpuset, > + MIN (iattr->extension->cpusetsize, cpusetsize)); > + if (cpusetsize > iattr->extension->cpusetsize) > + memset (p, '\0', cpusetsize - iattr->extension->cpusetsize); OK. > } > else > /* We have no information. */ > diff --git a/nptl/pthread_attr_setaffinity.c b/nptl/pthread_attr_setaffinity.c > index a42ffd92f4..9f9a70dee0 100644 > --- a/nptl/pthread_attr_setaffinity.c > +++ b/nptl/pthread_attr_setaffinity.c > @@ -34,23 +34,30 @@ __pthread_attr_setaffinity_np (pthread_attr_t *attr, size_t cpusetsize, > > if (cpuset == NULL || cpusetsize == 0) > { > - free (iattr->cpuset); > - iattr->cpuset = NULL; > - iattr->cpusetsize = 0; > + if (iattr->extension != NULL) > + { > + free (iattr->extension->cpuset); > + iattr->extension->cpuset = NULL; > + iattr->extension->cpusetsize = 0; OK. > + } > } > else > { > - if (iattr->cpusetsize != cpusetsize) > + int ret = __pthread_attr_extension (iattr); > + if (ret != 0) > + return ret; > + > + if (iattr->extension->cpusetsize != cpusetsize) > { > - void *newp = (cpu_set_t *) realloc (iattr->cpuset, cpusetsize); > + void *newp = realloc (iattr->extension->cpuset, cpusetsize); > if (newp == NULL) > return ENOMEM; > > - iattr->cpuset = newp; > - iattr->cpusetsize = cpusetsize; > + iattr->extension->cpuset = newp; > + iattr->extension->cpusetsize = cpusetsize; > } > > - memcpy (iattr->cpuset, cpuset, cpusetsize); > + memcpy (iattr->extension->cpuset, cpuset, cpusetsize); OK. > } > > return 0; > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 86fbeb5218..f6418eb5ed 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -884,7 +884,7 @@ __pthread_create_2_0 (pthread_t *newthread, const pthread_attr_t *attr, > new_attr.guardsize = ps; > new_attr.stackaddr = NULL; > new_attr.stacksize = 0; > - new_attr.cpuset = NULL; > + new_attr.extension = NULL; OK. > > /* We will pass this value on to the real implementation. */ > attr = (pthread_attr_t *) &new_attr; > diff --git a/sysdeps/nptl/internaltypes.h b/sysdeps/nptl/internaltypes.h > index 6d06a76baf..ca57c315d4 100644 > --- a/sysdeps/nptl/internaltypes.h > +++ b/sysdeps/nptl/internaltypes.h > @@ -36,9 +36,10 @@ struct pthread_attr > /* Stack handling. */ > void *stackaddr; > size_t stacksize; > - /* Affinity map. */ > - cpu_set_t *cpuset; > - size_t cpusetsize; > + > + /* Allocated via a call to __pthread_attr_extension once needed. */ > + struct pthread_attr_extension *extension; > + void *unused; OK. Keep the same size, but shuffle the usage around to get an extension pointer and an unused pointer. > }; > > #define ATTR_FLAG_DETACHSTATE 0x0001 > @@ -57,6 +58,15 @@ union pthread_attr_transparent > struct pthread_attr internal; > }; > > +/* Extension space for pthread attributes. Referenced by the > + extension member of struct pthread_attr. */ > +struct pthread_attr_extension > +{ > + /* Affinity map. */ > + cpu_set_t *cpuset; > + size_t cpusetsize; OK. At worst an applicaiton that is copying the attribute around is going to just get a copied pointer to the same affinity map. Such an application was not correct anyway. > +}; > + > /* Mutex attribute data structure. */ > struct pthread_mutexattr > { > diff --git a/sysdeps/unix/sysv/linux/createthread.c b/sysdeps/unix/sysv/linux/createthread.c > index 21f9d24f2d..6588893ba5 100644 > --- a/sysdeps/unix/sysv/linux/createthread.c > +++ b/sysdeps/unix/sysv/linux/createthread.c > @@ -52,8 +52,10 @@ create_thread (struct pthread *pd, const struct pthread_attr *attr, > /* Determine whether the newly created threads has to be started > stopped since we have to set the scheduling parameters or set the > affinity. */ > + bool need_setaffinity = (attr != NULL && attr->extension != NULL > + && attr->extension->cpuset != 0); OK. > if (attr != NULL > - && (__glibc_unlikely (attr->cpuset != NULL) > + && (__glibc_unlikely (need_setaffinity) > || __glibc_unlikely ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0))) > *stopped_start = true; > > @@ -113,12 +115,13 @@ create_thread (struct pthread *pd, const struct pthread_attr *attr, > int res; > > /* Set the affinity mask if necessary. */ > - if (attr->cpuset != NULL) > + if (need_setaffinity) > { > assert (*stopped_start); > > res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid, > - attr->cpusetsize, attr->cpuset); > + attr->extension->cpusetsize, > + attr->extension->cpuset); OK. > > if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res))) > err_out: > -- Cheers, Carlos.