Re: [PATCH 3/3] posix timers: use SIGQUEUE_CANCELLED when the timer is destroyed

[Oleg Nesterov]

On 05/17, Linus Torvalds wrote:
> 
> On Sat, 17 May 2008, Oleg Nesterov wrote:
> 
> > On 05/17, Oleg Nesterov wrote:
> > >
> > > This is a user visible change. With this patch sys_timer_delete() discards
> > > the pending signal which was generated by the timer.
> > 
> > If this change is undesirable, we can (for example) do
> > 
> > 	--- kernel/posix-timers.c
> > 	+++ kernel/posix-timers.c
> > 	@@ -885,6 +885,7 @@ itimer_delete(struct k_itimer *timer)
> > 		timer->it_process = NULL;
> > 	 
> > 		unlock_timer(timer, flags);
> > 	+	tmr->sigq->flags |= SIGQUEUE_CANCELLED;
> > 		release_posix_timer(timer, IT_ID_SET);
> > 	 }
> > 
> > instead, and still fix the "BUG 10460".
> 
> The only reason I like that better is that it makes me nervous when one 
> re-initializes the whole flags field. So your original 3/3 patch
> 
> 	-       q->flags &= ~SIGQUEUE_PREALLOC;
> 	+       q->flags = SIGQUEUE_CANCELLED; /* clears SIGQUEUE_PREALLOC */
> 
> just makes me go "Hmm, what about all the other flag bits?"

Yes, this is another reason for a separate patch.

> Now, admittedly, there are currently (with your patch) just two 
> SIGQUEUE_xyz bits, so by just doing that single assignment, you really 
> only modify the two bits you want to modify. But maybe that will change. 
> So I'd prefer to either write it as
> 
> 	q->flags &= ~SIGQUEUE_PREALLOC;
> 	q->flags |= SIGQUEUE_CANCELLED;

Initially, I did

	q->flags |= SIGQUEUE_CANCELLED;
	spin_lock_irqsave(lock, flags);
	q->flags &= ~SIGQUEUE_PREALLOC;

to document the fact that SIGQUEUE_CANCELLED can be set lockless, but
then "optimized" the code, couldn't help myself... Besides, the code
above looks really confusing without the fat comment.

Another reason. We are losing the control over "q" here, I don't think
we can have other flags which should be preserved once we set _CANCELLED
and cleared _PREALLOC.

But yes, I agree, this is not a good practice. I'd leave the patch as is
at least for now but I don't mind to redo and resend. Fortunately, this
falls to the "cleanup" category.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/