On Fri, 11 Aug 2017, Josh Poimboeuf wrote:
> On Thu, Aug 10, 2017 at 12:48:14PM +0200, Miroslav Benes wrote:
> > Last, sending the fake signal is not automatic. It is done only when
> > admin requests it by writing 1 to force sysfs attribute in livepatch
> > sysfs directory.
>
> 'writing 1' -> 'writing "signal"'
>
> (unless you take my suggestion to change to two separate sysfs files)
I'll take two separate sysfs files instead.
> > @@ -468,7 +468,12 @@ static ssize_t force_store(struct kobject *kobj,
> > struct kobj_attribute *attr,
> > return -EINVAL;
> > }
> >
> > - return -EINVAL;
> > + if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
> > + klp_force_signals();
>
> Any reason why you can't just do a strcmp()?
Not really IIRC. I just borrowed the code from
mm/huge_memory.c:enabled_store().
> > +++ b/kernel/livepatch/transition.c
> > @@ -577,3 +577,43 @@ void klp_copy_process(struct task_struct *child)
> >
> > /* TIF_PATCH_PENDING gets copied in setup_thread_stack() */
> > }
> > +
> > +/*
> > + * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set.
> > + * Kthreads with TIF_PATCH_PENDING set are woken up. Only admin can
> > request this
> > + * action currently.
> > + */
> > +void klp_force_signals(void)
> > +{
> > + struct task_struct *g, *task;
> > +
> > + pr_notice("signalling remaining tasks\n");
>
> As a native US speaker with possible OCD spelling tendencies, it bothers
> me to see "signalling" with two l's instead of one. According to
> Google, the UK spelling of the word has two l's, so maybe it's not a
> typo. I'll forgive you if you don't fix it :-)
If it bothers you, I'll fix it. As a non-native speaker, I can live with
both.
> > +
> > + read_lock(&tasklist_lock);
> > + for_each_process_thread(g, task) {
> > + if (!klp_patch_pending(task))
> > + continue;
> > +
> > + /*
> > + * There is a small race here. We could see TIF_PATCH_PENDING
> > + * set and decide to wake up a kthread or send a fake signal.
> > + * Meanwhile the task could migrate itself and the action
> > + * would be meaningless. It is not serious though.
> > + */
> > + if (task->flags & PF_KTHREAD) {
> > + /*
> > + * Wake up a kthread which still has not been migrated.
> > + */
> > + wake_up_process(task);
> > + } else {
> > + /*
> > + * Send fake signal to all non-kthread tasks which are
> > + * still not migrated.
> > + */
> > + spin_lock_irq(&task->sighand->siglock);
> > + signal_wake_up(task, 0);
> > + spin_unlock_irq(&task->sighand->siglock);
> > + }
> > + }
> > + read_unlock(&tasklist_lock);
>
> I can't remember if we talked about this before, is it possible to also
> signal/wake the idle tasks?
Jiri mentioned that in his email. It is not that easy. Take a look at
pick_next_task() in kernel/sched/core.c. idle_sched_class is always the
last one to be checked. Of course we could do something like this
there...
if (klp_patch_pending(rq->idle)) {
p = idle_sched_class.pick_next_task(rq, prev);
return p;
}
... but people may be watching, so I didn't say anything.
Thanks,
Miroslav
