lkml.org 
[lkml]   [2021]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 03/11] sched,livepatch: Use task_call_func()
On Tue, Oct 05, 2021 at 01:40:24PM +0200, Petr Mladek wrote:
> On Wed 2021-09-29 17:17:26, Peter Zijlstra wrote:
> > Instead of frobbing around with scheduler internals, use the shiny new
> > task_call_func() interface.
> >
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -274,6 +266,22 @@ static int klp_check_stack(struct task_s
> > return 0;
> > }
> >
> > +static int klp_check_and_switch_task(struct task_struct *task, void *arg)
> > +{
> > + int ret;
> > +
> > + if (task_curr(task))
>
> This must be
>
> if (task_curr(task) && task != current)
>
> , otherwise the task is not able to migrate itself. The condition was
> lost when reshuffling the original code, see below.

Urgh, yeah, I misread that and figued task_curr() should already capture
current, but the extra clause excludes current :/

> JFYI, I have missed it during review. I am actually surprised that the
> process could check its own stack reliably. But it seems to work.

Ah, unwinding yourself is actually the only sane option ;-)

> > - rq = task_rq_lock(task, &flags);
> > + ret = task_call_func(task, klp_check_and_switch_task, &old_name);
>
> It looks correct. JFYI, this is why:
>
> The logic seems to be exactly the same, except for the one fallout
> mentioned above. So the only problem might be races.
>
> The only important thing is that the task must not be running on any CPU
> when klp_check_stack(task, arg) is called. By other word, the stack
> must stay the same when being checked.
>
> The original code prevented races by taking task_rq_lock().
> And task_call_func() is slightly more relaxed but it looks safe enough:
>
> + it still takes rq lock when the task is in runnable state.
> + it always takes p->pi_lock that prevents moving the task
> into runnable state by try_to_wake_up().

Correct, the new task_call_func() is trying hard to not take rq->lock,
but should be effectively identical to task_rq_lock().

> With the added (task != current) check:

Done

> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Tested-by: Petr Mladek <pmladek@suse.com>

Thanks!

\
 
 \ /
  Last update: 2021-10-05 16:08    [W:0.131 / U:0.616 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site