lkml.org 
[lkml]   [2015]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 06/18] signal/kthread: Initial implementation of kthread signal handling
    Hi Oleg,

    I am sorry for the late reply. I wanted to think more before answering
    all the mails.

    On Mon 2015-06-08 23:13:36, Oleg Nesterov wrote:
    > I do not. Contrary, I think this needs more code in the likely case.
    > Anyway, this API won't have too many users, so I don't even this this
    > is that important.
    >
    > > > > + if (sig_kernel_stop(signr)) {
    > > > > + __set_current_state(TASK_STOPPED);
    > > > > + spin_unlock_irqrestore(&sighand->siglock, flags);
    > > > > + /* Don't run again until woken by SIGCONT or SIGKILL */
    > > > > + freezable_schedule();
    > > > > + goto relock;
    > > >
    > > > Yes this avoids the race with SIGCONT. But as I said we can add another
    > > > trivial helper which checks JOBCTL_STOP_DEQUEUED. So a kthread can do
    > > > this itself.
    > >
    > > Hmm, the helper would have a strange semantic. You need to take
    > > sighand->siglock, dequeue the signal (SIGSTOP), and call
    > > __set_current_state(TASK_STOPPED) before you release the lock.
    > > But what would happen if the dequeued signal is _not_ SIGSTOP?
    >
    > Perhaps I missed your point, but no. If you want to handle SIGSTOP
    > you can do
    >

    I think that we need to add:

    spin_lock_irq(&sighand->siglock);

    > signr = kthread_signal_dequeue();
    > switch (signr) {
    > case SIGSTOP:
    > something_else();
    > kthread_do_signal_stop();
    > ...
    > }

    And if we want to avoid any race, kthread_do_signal_stop() should look like:

    void kthread_do_signal_stop(unsigned long flags)
    {
    struct sighand_struct *sighand = current->sighand;

    __set_current_state(TASK_STOPPED);
    spin_unlock_irqrestore(&sighand->siglock, flags);
    /* Don't run again until woken by SIGCONT or SIGKILL */
    freezable_schedule();
    }

    It means that we will have spin_lock() in one function and
    spin_unlock() in another one. This is what I meant with
    the strange semantic. This is why I think that it might be
    cleaner to implement some generic kthread_do_signal() or so
    and allow to (re)define/add sigactions via callbacks.

    Note that I am not aware of any kthread that would use SIGSTOP
    non-standard way.

    Anyway, I am going to concentrate on the main structure of the kthread
    API and will put the controversial signal handling a side for now.
    I will get back to it when converting the few kthreads that use
    signals. I will think more about your feedback in the meantime.

    Best Regards,
    Petr


    \
     
     \ /
      Last update: 2015-06-15 15:21    [W:2.319 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site