Messages in this thread | | | From | Roland McGrath <> | Subject | Re: [PATCH] posix-timers: Do not modify an already queued timer signal | Date | Sun, 20 Jul 2008 17:47:22 -0700 (PDT) |
| |
> Yes, thanks, I see. But does it have any meaning for the user-space? [si_sys_private]
No, it's not part of the user ABI. It's not even copied out (see copy_siginfo_to_user). (A spare siginfo_t field was just used as a handy bit of storage in the signal queue element. It's a kludge.)
> Let me repeat. Can't we make a simple fix for now for this nasty and > ancient bug, before we do the more clever changes,
Probably. I don't find it easy to be sure there aren't more bad problems caused by trying to re-send the same sigqueue entry.
You do need to clear si_overrun there to be correct in the usual case (not already queued). The fields set explicitly in posix_timer_event, plus si_overrun, are the only meaningful fields for SI_TIMER (again see copy_siginfo_to_user). The memset is already superfluous.
It would be a perfectly fine and worthwhile optimization/cleanup on its own just to move all the initialization of sigq->info (everything but si_sys_private) to alloc_posix_timer. That would also happen to mask the symptom of this double-queuing problem that's been observed.
Even if it's a fine stopgap, I'm not comfortable calling this a real "fix". It seems likely this is the good choice for the stable branch.
> Suppose that the signal was already dequeued but sigq->info wasn't copied > to the user-space. The thread which does dequeue_signal() unlocked siglock > and waits for ->it_lock.
This is fine. sigq->info is copied into a local (kernel stack) copy in collect_signal(), i.e. inside dequeue_signal() long before the unlock. In short, once __dequeue_signal() returns, the signal goes from "queued" to "delivered". The delivery is on its way to happening, and from the instant that thread released the siglock it's no different from if it had completed handler setup, returned to user, and had a long scheduling delay before actually executing the first user instruction. At this point, any other event is not "racing", it's just "after".
> posix_timer_event() sees list_empty(), proceeds and calls send_sigqueue(). > Now we requeue this ->sigq instead of incrementing si_overrun.
It's not "requeue". It's "queue" after it was no longer queued, with semantics no different from the first time you queue it.
> The thread which does dequeue_signal() continues and re-schedules the > timer while ->sigq is queued. Then it copies sigq->info to the user space.
The thread that dequeued the first timer signal had ceased all reference to sigq by the time it unlocked siglock. When its do_schedule_next_timer call gets it_lock, it can do bookkeeping in struct k_itimer to figure out what posix_timer_event or timer_settime has done lately. (Like I said, the ->fired flag alone might not really cover all the angles. I was expecting you to figure out the exactly details for me. ;-)
> While we are here, off-topics question.
You know I hate those. We aren't here. We're in chairs reading email. We'll be in the same places if you send a separate message for a separate topic.
> Is it really useful to convert the SIGEV_THREAD_ID timer to the group-wide > timer when the thread dies?
Not really. I think the main intent there is just to be sure we never hold on to an old task_struct pointer (which might have been the bug in the past).
Thanks, Roland
| |