lkml.org 
[lkml]   [2017]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/3] livepatch: send a fake signal to all blocking tasks
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

\
 
 \ /
  Last update: 2017-08-14 16:29    [W:0.067 / U:0.420 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site