This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #13613] Allow a single-threaded process to cancel itself
- From: "Carlos O'Donell" <carlos at systemhalted dot org>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: libc-alpha at sourceware dot org, jakub at redhat dot com
- Date: Wed, 9 May 2012 11:30:57 -0400
- Subject: Re: [PATCH][BZ #13613] Allow a single-threaded process to cancel itself
- References: <20120509202437.268a72b9@spoyarek>
On Wed, May 9, 2012 at 10:54 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> A single-threaded process cannot cancel itself using pthread_cancel()
> as reported in the bz. This is because header.multiple_threads is not
> set till a thread is cloned and cancellation point entries are guarded
> by this.
>
> This is a little odd in terms of a request since this can be easily
> done for the main process using exit and atexit. However, there is
> nothing in the specification of pthread_cancel that explicitly
> disallows this, so for the sake of compliance, pthread_cancel should
> work for a single-threaded process too.
I agree.
If pthread_self() works then pthread_cancel() should also work.
> Attached patch unconditionally enables multiple_threads in the caller
> of pthread_cancel. This should have an impact only for a
> single-threaded process since any threads in a multi-threaded process
> should have that flag enabled. This was suggested in the bz by Jakub
> Jelinek. I have also added a test case to verify that this is fixed.
>
> I have run this on x86_64 with the test case with and without the
> patch to make sure that this is fixed. I did not find any regressions
> introduced as a result of this patch.
>
> Regards,
> Siddhesh
>
> nptl/ChangeLog:
>
> 2012-05-09 ?Siddhesh Poyarekar ?<siddhesh@redhat.com>
> ? ? ? ? ? ?Jakub Jelinek ?<jakub@redhat.com>
>
> ? ? ? ?[BZ #13613]
> ? ? ? ?* nptl/pthread_cancel.c (pthread_cancel): Enable
> ? ? ? ?multiple_threads before marking the thread as cancelled.
> ? ? ? ?* nptl/tst-cancel-self.c: New test case.
> ? ? ? ?* nptl/Makefile (tests): Add tst-cancel-self.
I like the idea of the patch.
I don't like this implementation.
It has two problems:
* After calling pthread_cancel() *all* of the optimizations that could
have used SINGLE_THREAD_P are not available, not just those related to
cancellation.
* It overloads multiple_threads with a new meaning i.e. "Is true if
either more than one thread is running or if the one thread called
pthread_cancel()", which is bad for maintainability.
I would suggest an alternate solution:
* Add a new per-thread variable e.g. enable_cancellation_checking.
* Find all places where multiple_threads is used to guard cancellation
and change them to use the new variable (through a macro).
- 19 sysdep-cancel.h files.
- Numerous *.c files.
This has the following beneifts:
* Previous programs that don't call pthread_cancel() have all the
performance benefits.
* New programs that call pthread_cancel() work.
* Uses of SINGLE_THREAD_P that aren't related to cancellation work as expected.
I understand that this isn't an easy fix, and I'm sorry :-(
Comments?
Cheers,
Carlos.