From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com [IPv6:2607:f8b0:4864:20::f29]) by sourceware.org (Postfix) with ESMTPS id 37172394740B for ; Tue, 1 Jun 2021 13:50:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 37172394740B Received: by mail-qv1-xf29.google.com with SMTP id z1so7176188qvo.4 for ; Tue, 01 Jun 2021 06:50: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:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=0R1REL0rWtBegEwDNZ5XDGnkC6csRdESU0K4AQby/kw=; b=sM2qsInmwkVPu9gFqg6ZmzO44Ykftu+/z+bhI5at4sE8YBXy+xnP995q8e4xa1VO+s b+OyoRRSV+0GGsqrHKUzapGsjITfsvl+xJlcXaVE1/c9Ww490PBZ7oNI2+jQrzo2ZVSu 83F6XBjEGmWp2Xls+EQhwpJ+AEvX4Hcwx90T/ShkOxgY4ZuUVxfoTSBZ5wY97D+/+Jge xC7BQ2Zb6vgWE9l3GiG+EqvjjFqxK234B/LATjRxvJJzlnb2u77v75i1oHFw1Pu2tSIl VpbePJiN+BFJ4toxVdYy5XN/geHwLJaX296LGcKMwzO2Be6IPPSEk+c5jxatpTJZCt1T YwQw== X-Gm-Message-State: AOAM532xtji7vl4Ew6lkYRMR2xNrsGdOlTHHG8QGu3iFRfiV3qNsH9Uh 5RYgFYYjgF3flrQv9CE2gHDnYzMbvG3H0A== X-Google-Smtp-Source: ABdhPJxdg9gxWRldzYRlVgBqbmW2TF7tpxVQoEMX3Ye5Mu4HZ9tJblouI1x+4asZk5OmP3FAJ95u6Q== X-Received: by 2002:a05:6214:1248:: with SMTP id q8mr15353234qvv.47.1622555404613; Tue, 01 Jun 2021 06:50:04 -0700 (PDT) Received: from [192.168.1.4] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id f15sm11298329qkk.41.2021.06.01.06.50.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Jun 2021 06:50:04 -0700 (PDT) Subject: Re: [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK To: Florian Weimer , Adhemerval Zanella via Libc-alpha References: <20210527172823.3461314-1-adhemerval.zanella@linaro.org> <20210527172823.3461314-5-adhemerval.zanella@linaro.org> <87bl8q6jok.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Message-ID: <10f40817-50da-5997-c2e8-3aa4edfab4a4@linaro.org> Date: Tue, 1 Jun 2021 10:50:01 -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: <87bl8q6jok.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Tue, 01 Jun 2021 13:50:15 -0000 On 01/06/2021 06:03, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> The CANCELING_BITMASK is used as an optimization to avoid sending >> the signal when pthread_cancel is called in a concurrent manner. >> >> This requires then to put both the cancellation state and type on >> a shared state (cancelhandling), since 'pthread_cancel' checks >> whether cancellation is enabled and asynchrnous to either cancel >> itself of sending the signal. >> >> It also requires handle the CANCELING_BITMASK on >> __pthread_disable_asynccancel, however this is incurs in the same > > typo: “this is incurs” Ack. > >> issues described on BZ#12683: the cancellation is acting even *after* > > “is acted upon even *after*”? Ack. > >> the syscalls returns with user visible side-effects. >> >> This patch removes this optimization and simplifies the pthread >> cancellation implementation: pthread_cancel now first check if > > typo: check → checks Ack. > >> cancellation is already pending and if not always send a signal > > typo: send → sends Ack. > > Maybe add a comma before “always”. Ack. > >> if the target is not itself. The SIGCANCEL handler is also simpified >> since there is not need to setup a CAS loop. >> >> It also alows to move both the cancellation state and mode out of >> 'cancelhadling' (it is done in subsequent patches). > > typo: alows Ack. > >> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c >> index deb404600c..8dfbcff8c3 100644 >> --- a/nptl/pthread_cancel.c >> +++ b/nptl/pthread_cancel.c > >> @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th) >> " must be installed for pthread_cancel to work\n"); >> } >> + >> + int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK); >> + if ((oldch & CANCELED_BITMASK) != 0) >> + return 0; >> + >> + if (pd == THREAD_SELF) >> { >> + /* A single-threaded process should be able to kill itself, since there >> + is nothing in the POSIX specification that says that it cannot. So >> + we set multiple_threads to true so that cancellation points get >> + executed. */ >> + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1); >> #ifndef TLS_MULTIPLE_THREADS_IN_TCB >> - __libc_multiple_threads = 1; >> + __libc_multiple_threads = 1; >> #endif >> + >> + THREAD_SETMEM (pd, result, PTHREAD_CANCELED); >> + if ((oldch & CANCELTYPE_BITMASK) != 0) >> + __do_cancel (); >> + return 0; >> } > > The last part is for asynchronous self-cancel, right? Don't we have to > check that cancellation is enabled, too? Indeed it should be: if ((oldch & CANCELSTATE_BIT) && (oldch & CANCELTYPE_BITMASK) != 0) I ended up fixing it later in the serie with the cancel type and state refactor. I have fixed it locally.