On Wed 2021-09-22 13:05:09, Peter Zijlstra wrote:
> Instead of frobbing around with scheduler internals, use the shiny new
> task_try_func() interface.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> kernel/livepatch/transition.c | 84 ++++++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 47 deletions(-)
>
> --- 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_task(struct task_struct *task, void *arg)
Please, call this klp_check_and_switch_task() to make it clear
that it actually does the switch.
> +{
> + int ret;
> +
> + if (task_curr(task))
> + return -EBUSY;
> +
> + ret = klp_check_stack(task, arg);
> + if (ret)
> + return ret;
> +
> + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> + task->patch_state = klp_target_state;
> + return 0;
> +}
> +
> /*
> * Try to safely switch a task to the target patch state. If it's currently
> * running, or it's sleeping on a to-be-patched or to-be-unpatched function, or
> @@ -305,36 +308,23 @@ static bool klp_try_switch_task(struct t
> * functions. If all goes well, switch the task to the target patch
> * state.
> */
> - rq = task_rq_lock(task, &flags);
> -
> - if (task_running(rq, task) && task != current) {
> - snprintf(err_buf, STACK_ERR_BUF_SIZE,
> - "%s: %s:%d is running\n", __func__, task->comm,
> - task->pid);
> - goto done;
> + ret = task_try_func(task, klp_check_task, &old_name);
> + switch (ret) {
> + case -EBUSY:
> + pr_debug("%s: %s:%d is running\n",
> + __func__, task->comm, task->pid);
> + break;
> + case -EINVAL:
> + pr_debug("%s: %s:%d has an unreliable stack\n",
> + __func__, task->comm, task->pid);
> + break;
> + case -EADDRINUSE:
> + pr_debug("%s: %s:%d is sleeping on function %s\n",
> + __func__, task->comm, task->pid, old_name);
> + break;
I would prefer to be on the safe side and catch error codes that might
eventually appear in the future.
case 0:
/* success */
break;
default:
pr_debug("%s: Unknown error code (%d) when trying to switch %s:%d\n",
__func__, ret, task->comm, task->pid);
> }
>
> - ret = klp_check_stack(task, err_buf);
> - if (ret)
> - goto done;
> -
> - success = true;
> -
> - clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> - task->patch_state = klp_target_state;
> -
> -done:
> - task_rq_unlock(rq, task, &flags);
> -
> - /*
> - * Due to console deadlock issues, pr_debug() can't be used while
> - * holding the task rq lock. Instead we have to use a temporary buffer
> - * and print the debug message after releasing the lock.
> - */
> - if (err_buf[0] != '\0')
> - pr_debug("%s", err_buf);
> -
> - return success;
> + return !ret;
> }
Otherwise, it is great improvement!
Best Regards,
Petr
