From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com [IPv6:2607:f8b0:4864:20::f2c]) by sourceware.org (Postfix) with ESMTPS id 9A574385742A for ; Thu, 20 May 2021 19:57:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9A574385742A Received: by mail-qv1-xf2c.google.com with SMTP id h7so8616932qvs.12 for ; Thu, 20 May 2021 12:57: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=t/jgNjOodyaGcjF2oFEC02mebbDqqlUPDoRL4bDmojU=; b=f4PsuKS1GUIz/HPuLM3lY8oqTDR8sKq8AWhVcDL7OaQb4dY7iK7NmXZzVKWGeU3cpO j8HXKrUSt4WLHcdNJ2dYkH3nNkTdxCo2pmr2tNJflJ3/oVS/bGa+c/UJvUCdeYArt6R2 EtMDl91dwy6gp3sXDzCdZ29bUIJtmkiaMb//llFWqcXzz6Hkz5HkOWUYeiqbafTM+2IO qoxTgOkmW6tYS0gHeOxMm/HE0y9+GvkqqHT73NIRDsNJEgCTzGItzcVrKkG4l9oZDYQF cHUdVIbqNAuUg2eovcJhyv9Tge6q3E131+gIdAiADBra+MWfjJadAaH+dECoVCkV7IpI uq2g== X-Gm-Message-State: AOAM5306PMbXPSriNiMuX5UqB+2EmrFMij+NnDP/TKw5M7xBpEb9FZLq KDYUPeulM/oHZfjzBPiIUflO2HRkuPvOAg== X-Google-Smtp-Source: ABdhPJxtQv54jq8JqNGkCycFXzdjsLdaX3kQ4TRB0S6LGoelCs6ZyGMPtPLOZ3jMVYKX/+yS3UJk6w== X-Received: by 2002:a0c:db85:: with SMTP id m5mr7853282qvk.1.1621540641869; Thu, 20 May 2021 12:57:21 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id d28sm2924957qkl.105.2021.05.20.12.57.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 20 May 2021 12:57:21 -0700 (PDT) Subject: Re: [PATCH 01/10] nptl: Perform signal initialization upon pthread_create To: Florian Weimer Cc: libc-alpha@sourceware.org References: <87bl95jiqs.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Message-ID: <46b68c50-e1d9-1450-d3ea-a302b86e2949@linaro.org> Date: Thu, 20 May 2021 16:57:19 -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: <87bl95jiqs.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=-6.7 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: Thu, 20 May 2021 19:57:24 -0000 On 20/05/2021 16:41, Florian Weimer wrote: > * Adhemerval Zanella: > >>> int >>> __pthread_cancel (pthread_t th) >>> @@ -72,14 +129,23 @@ __pthread_cancel (pthread_t th) >>> oldval)) >>> goto again; >>> >>> - /* The cancellation handler will take care of marking the >>> - thread as canceled. */ >>> - pid_t pid = __getpid (); >>> - >>> - int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, >>> - SIGCANCEL); >>> - if (INTERNAL_SYSCALL_ERROR_P (val)) >>> - result = INTERNAL_SYSCALL_ERRNO (val); >>> + if (pd == THREAD_SELF) >>> + /* This is not merely an optimization: An application may >>> + call pthread_cancel (pthread_self ()) without calling >>> + pthread_create, so the signal handler may not have been >>> + set up for a self-cancel. */ >>> + sigcancel_handler (); >> >> I think it would be simple to just call __pthread_exit (PTHREAD_CANCELED) >> here, it won't require to split the cancellation handler, it already >> unwind if cancel state is enabled and asynchronous, and it does not >> require add another PTHREAD_STATIC_FN_REQUIRE hack. >> >> It would require an extra __libc_unwind_link_get call, but I think we >> can optimize it later (I am working on a patch to simplify it). > > It would be correct, I think. pthread_cancel is not a cancellation > point. > > #include > #include > > int > main (void) > { > pthread_cancel (pthread_self ()); > puts ("about to exit"); > } > > This should print “about to exit”. Yes, this is essentially sysdeps/pthread/tst-cancel-self.c. > >>> +/* This performs the initialization necessary when going from >>> + single-threaded to multi-threaded mode for the first time. */ >>> +static void >>> +late_init (void) >>> +{ >>> + struct sigaction sa; >>> + __sigemptyset (&sa.sa_mask); >>> + >>> + /* Install the cancellation signal handler (in static builds only if >>> + pthread_cancel has been linked in). If for some reason we cannot >>> + install the handler we do not abort. Maybe we should, but it is >>> + only asynchronous cancellation which is affected. */ >>> +#ifndef SHARED >>> + extern __typeof (__nptl_sigcancel_handler) __nptl_sigcancel_handler >>> + __attribute__ ((weak)); >>> + if (__nptl_sigcancel_handler != NULL) >>> +#endif >> >> This weak symbol can be avoided if we move the cancellation setup >> on pthread_cancel instead. I still think this is best approach, >> it disentangle the cancellation handling. > > But then we either have to introduce yet another global flag or install > the signal handler unconditionally before every cancel operation. I do > not think this results in a simplification. The flag will be just a static bool or int only define on pthread_cancel, something like: int __pthread_cancel (pthread_t th) { [...] static int init = 0; if (atomic_load_relaxed (&init) == 0) { install_sighandler (); init = 1; } [...] } Bu the main advantage is to move the cancellation code logically when it is actually used, and it is small improvement on both static linking (since the static code will be used solely is cancellation is used) and on runtime (since sigaction will be set only if pthread_cancel is called).