lkml.org 
[lkml]   [2008]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] posix-timers: Do not modify an already queued timer signal
    Date
    > 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


    \
     
     \ /
      Last update: 2008-07-21 02:51    [W:0.025 / U:30.076 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site