From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82f.google.com (mail-qt1-x82f.google.com [IPv6:2607:f8b0:4864:20::82f]) by sourceware.org (Postfix) with ESMTPS id 0B2553846401 for ; Wed, 26 May 2021 19:19:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0B2553846401 Received: by mail-qt1-x82f.google.com with SMTP id t20so1743530qtx.8 for ; Wed, 26 May 2021 12:19:22 -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:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=O2dREBI1WhJm27blvxe5u9/GEPh34xGQ9DaCyB76RFM=; b=Lv0yPR0wxtd771mfzLcpmprfpkPe8p4BhZ4Q5FAwoZtNvhvFNxkui6wSWSgAO7NayF 2sbo2KRS6NxsltONgQTmKXaBlgUvUKvXsk7GhqQBngn05U+5OuDR5hbDnM/QIJfPTFuA aIJcq4YDaCMj5ox75UWgLElrC9CHQlWZQHrInFD24AmI4kzFeRNTi8TquTMrSyQnKijb 0Q6M36RCKis3HXCK9lr7Wykbgx1AHqKmmpktgVgwD+iV7PN2GYXE57TTcKM/UbGYWM2e 0tNIyPM069OVvjo110LdmSgqNS/IvO63b5e3wTKLALAAGfYBWbINz3ZBwmc9msJlvcDj rGbA== X-Gm-Message-State: AOAM532Ob0Wr7M8URUsID0NIpk1H0Tf2YjRUG5xyF2GVU79HsrF4noc9 dDpBUGooDwj7BNPn56tpWD0pIyZdWk2maA== X-Google-Smtp-Source: ABdhPJzCPdACVApNwvwkzkiFa800lK20IoPQ3WWaAywE3IHnZZhXnTztroPMJxu/zilVg3YrwL9d+A== X-Received: by 2002:ac8:5419:: with SMTP id b25mr37595404qtq.328.1622056761383; Wed, 26 May 2021 12:19:21 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id s10sm2150012qkj.77.2021.05.26.12.19.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 May 2021 12:19:21 -0700 (PDT) Subject: Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511) To: Florian Weimer Cc: Adhemerval Zanella via Libc-alpha References: <20210526165728.1772546-1-adhemerval.zanella@linaro.org> <20210526165728.1772546-6-adhemerval.zanella@linaro.org> <87fsy9qu1p.fsf@oldenburg.str.redhat.com> <877djlqswb.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Message-ID: <4a561aad-4ba3-897b-e6b6-31305b06a088@linaro.org> Date: Wed, 26 May 2021 16:19:18 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <877djlqswb.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Wed, 26 May 2021 19:19:23 -0000 On 26/05/2021 14:58, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 26/05/2021 14:33, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> @@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, >>>> CONCURRENCY NOTES above). We can assert that STOPPED_START >>>> must have been true because thread creation didn't fail, but >>>> thread attribute setting did. */ >>>> - /* See bug 19511 which explains why doing nothing here is a >>>> - resource leak for a joinable thread. */ >>>> - assert (stopped_start); >>>> + { >>>> + assert (stopped_start); >>>> + if (stopped_start) >>>> + /* State (a), we own PD. The thread blocked on this lock will >>>> + finish due setup_failed being set. */ >>>> + lll_unlock (pd->lock, LLL_PRIVATE); >>>> + } >>>> else >>>> { >>>> /* State (e) and we have ownership of PD (see CONCURRENCY >>> >>> The assert followed by the if doesn't look right. One should check >>> setup_failed, I assume. Is there any way to trigger the broken assert >>> or the hang from the missing wakeup? >> >> This just check what is described in the CONCURRENCY NOTES: >> >> 204 Axioms: >> 205 >> 206 * The create_thread function can never set stopped_start to false. >> 207 * The created thread can read stopped_start but *never write to it*. > > I meant this: > > + assert (stopped_start); > + if (stopped_start) > > It doesn't look right to me. Yeah, the 'if' check is not really required. It will hit this code path only if clone has succeeded ('thread_ran' is true) but either sched_setaffinity or sched_setscheduler has failed (in this case 'stopped_start' will be always true). I changed to: if (thread_ran) { /* State (c) or (d) and we may not have PD ownership (see CONCURRENCY NOTES above). We can assert that STOPPED_START must have been true because thread creation didn't fail, but thread attribute setting did. */ assert (stopped_start); /* State (a), we own PD. The thread blocked on this lock will finish due setup_failed being set. The thread will be responsible to cleanup any allocated resource. */ lll_unlock (pd->lock, LLL_PRIVATE). } > >>> I happend to have looked at this today for the sigaltstack stuff. >>> >>> I felt that it would be simpler to perform the kernel-side setup >>> (affinity and scheduling policy) on the new thread. Essentially use >>> stopped_start mode unconditionally. And use a separate lock word for >>> the barrier/latch, instead of reusing the pd->lock. >>> >>> The advantage is that we can pass an arbitrary temporary object into the >>> internal start function, such as the signal mask. The creating thread >>> blocks until the initialization has happened and the immediate fate of >>> the thread has been decided. After that point the creating thread >>> deallocates the temporary object, and if the thread couldn't start, any >>> thread-related data. Otherwise the new thread takes ownership of that >>> data. >>> >>> I think it will be much easier to explain what is going on in the >>> concurrency notes. We have an additional synchronization step on all >>> created threads, but I doubt that this matters in practice. >>> >>> What do you think? I'm not sure if the change should happen as part of >>> this series. >> >> I see two potential issues on this approach: >> >> 1. The cpuset information is tied to the pthread_attr_t and can be >> potentially large, it means that if we want to make the created >> thread to consume we will need to either duplicate, add something >> like a reference count, or add more synchronization for the >> consumer. None is really simpler to implement and I don't think >> it is really an improvement. > > No, I think that's covered by the temporary object thing. The creating > thread will block until it's guaranteed that the new thread won't need > this data anymore. > >> 2. For both schedule parameters and affinity ideally we want to inform >> right away to user if the operation has failed. If we move it to >> thread itself, it still would require the synchronization to inform >> pthread_create the operation has failed. I don't see much improvement, >> the pthread_create latency will be essentially the same. > > Correct. But it will be much simpler because we can treat all > kernel-side initialization the same. For example, sigaltstack might > fail with invalid flags, too. And we'd definitely have to run that on > the new thread. The small downside is we will need to consume some of the thread own stack to pass such information, but it can be only a pointer in any case. But I still see setting the scheduling/affinity synchronous to have some advantage: we can fast exit in the case of failure and join the thread on pthread_create (instead of making it act a detached one). This make the failure case not 'leak' the thread context and resource out of pthread_create. It might be somewhat complex to code it right now, since we need to take care of destructor for thread_local variables and thread-local data, __libc_thread_freeres, etc. That is also why I used the current unwind mechanism to exit in case of setup failure (maybe instead of adding the setup_failed on the IS_DETACHED check the patch could just setup the thread as detached, the pd->joinid could be set non-atomically since it owns the pd). In any case, since we do not implement this I don't have a preference. Your arbitrary temporary object seems ok and should work on the scheduler/affinity setup.