From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82a.google.com (mail-qt1-x82a.google.com [IPv6:2607:f8b0:4864:20::82a]) by sourceware.org (Postfix) with ESMTPS id 8BAE7399C885 for ; Wed, 9 Jun 2021 17:07:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8BAE7399C885 Received: by mail-qt1-x82a.google.com with SMTP id 93so11219274qtc.10 for ; Wed, 09 Jun 2021 10:07:05 -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=b4xTvu2brUEFLAO+Y/SDW2pS841QabF7bNZ0jqxVtZg=; b=ZGHW+kCw9Y+agyag2qLG6KdiVzy+BzT8pzbEwUNyQj1KEqlxpcMrFKLg5mK6TnAGCj JpNrfbjz7YyG2E498ZPjkiU+qTk2Bql8JXiIM2WtqpIovjm0h3g53LlNIMenJ3KqAVzp 8OY09/peroxOB/zEyvCVcdmNIOpFSUYn9+d1oDNdFLUSzpwhQklFYD0jw8IhLK3cunSs 1IrUk6FN2KQWlmlzWYFOsdK/Rda0hrVCU5H0F+rBznStFvBHoJC+9LV29VgBqykIwNEA Gd2TH7kS5PIyCOATQW8S6ZtRf2HI6fK1PwSoW1Cu0r+GqBBI4i3eKuBWymnOu8fRgTX/ YN/A== X-Gm-Message-State: AOAM532wAUir+E38efOLH4nhvHiQxK/k+dYMcwiDhPlwwBTElrq7FDzs f89heSanN18kgQO8GESxfGD7wTS4PhfyjA== X-Google-Smtp-Source: ABdhPJwk3tZqVBYoHhVkljsxNSa4m2z8ROJqz0PezgW3jcw3PvpJU6kvY7Mfysjte3vHB/GvWwh0vA== X-Received: by 2002:a05:622a:87:: with SMTP id o7mr904397qtw.137.1623258425031; Wed, 09 Jun 2021 10:07:05 -0700 (PDT) Received: from [192.168.1.4] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id x15sm478076qkh.19.2021.06.09.10.07.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Jun 2021 10:07:04 -0700 (PDT) Subject: Re: [PATCH v3] nptl: Deallocate the thread stack on setup failure (BZ #19511) To: Florian Weimer Cc: Adhemerval Zanella via Libc-alpha References: <20210527172823.3461314-3-adhemerval.zanella@linaro.org> <20210602125644.3725112-1-adhemerval.zanella@linaro.org> <87lf7kbp5b.fsf@oldenburg.str.redhat.com> <14ba289b-9168-45bd-4547-ebd3f33f64b2@linaro.org> <871r9b5erp.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Message-ID: <410bac38-fcd9-1cb4-0961-cbb6eb1fdc8b@linaro.org> Date: Wed, 9 Jun 2021 14:07:02 -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: <871r9b5erp.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.3 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, 09 Jun 2021 17:07:06 -0000 On 09/06/2021 10:49, Florian Weimer wrote: > * Adhemerval Zanella: > >> THREAD_RAN only signals if whether clone() or thread setup has failed, >> there is no synchronization involved and it is already local to >> creating thread. I changed the concurrency comment to the following, >> to indicate that (c) and (d) are essentially the same as you noted: >> >> (c) If either a joinable or detached thread setup failed and THREAD_RAN >> is true, then the creating thread releases ownership to the new thread, >> the created thread sees the failed setup through PD SETUP_FAILED > > PD->setup_failed (I think this is the GNU convention). Ack (I was in doubt which would the rule here). > >> member, releases the PD ownership, and exits. The creating thread will >> be responsible for cleanup the allocated resources. The THREAD_RAN is >> local to creating thread and indicate whether thread creation or setup >> has failed. >> >> (d) If the thread creation failed and THREAD_RAN is false (meaning >> ARCH_CLONE has failed), then the creating thread retains ownership >> of PD and must cleanup he allocated resource. No waiting for the new >> thread is required because it never started. > > Yes, that's much better. > >> I think for such case you will need to set PD->stopped_start to true >> before thread creation (similar to what affinity and schedule set do), >> add the required initialization after the setup failure check, and >> handle potential errors. >> >> Something like (I removed the comments to simplify): > > Yeah, that is what I expect as well. > >> Below it is the updated patch based on your suggestion, including the >> concurrency state descriptor regarding THREAD_RAN. Are this version >> ok to commit? > > This version looks okay to me, with the PD->setup_failed change above. Thanks, I will run a check on some architecture and commit the patchset.