Re: [PATCH][BZ #13613] Allow a single-threaded process to cancel itself

["Carlos O'Donell" <carlos at systemhalted dot org>]

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.