Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

2019-05-07 Thread Josh Poimboeuf 
On Tue, May 07, 2019 at 01:50:29PM +0200, Petr Mladek wrote:
> On Mon 2019-05-06 19:43:19, Josh Poimboeuf wrote:
> > On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> > > klp_try_switch_task() is called under klp_mutex. The buffer for
> > > debugging messages might be static.
> > 
> > The patch description is missing a "why" (presumably to reduce stack
> > usage).
> 
> Exactly. I thought that it was obvious. But I am infected by printk
> code where line buffers are 1k and nobody wants them on the stack.
> 
> 128bytes in klp_try_switch_task() context are acceptable but
> it is still rather big buffer.
> 
> OK, what about the following commit message?
> 
> "klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static to reduce stack usage."

It's better to use imperative language.  It would also be good to
reverse the order of the wording by starting with the problem.

Something like:

The err_buf array uses 128 bytes of stack space.  Move it off the stack
by making it static.  It's safe to use a shared buffer because
klp_try_switch_task() is called under klp_mutex.

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

2019-05-07 Thread Petr Mladek 
On Mon 2019-05-06 19:43:19, Josh Poimboeuf wrote:
> On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> > klp_try_switch_task() is called under klp_mutex. The buffer for
> > debugging messages might be static.
> 
> The patch description is missing a "why" (presumably to reduce stack
> usage).

Exactly. I thought that it was obvious. But I am infected by printk
code where line buffers are 1k and nobody wants them on the stack.

128bytes in klp_try_switch_task() context are acceptable but
it is still rather big buffer.

OK, what about the following commit message?

"klp_try_switch_task() is called under klp_mutex. The buffer for
debugging messages might be static to reduce stack usage."

Best Regards,
Petr

Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

2019-05-06 Thread Josh Poimboeuf 
On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static.

The patch description is missing a "why" (presumably to reduce stack
usage).

> 
> Signed-off-by: Petr Mladek 
> ---
>  kernel/livepatch/transition.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 8e0274075e75..d66086fd6d75 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -304,11 +304,11 @@ static int klp_check_stack(struct task_struct *task, 
> char *err_buf)
>   */
>  static bool klp_try_switch_task(struct task_struct *task)
>  {
> + static char err_buf[STACK_ERR_BUF_SIZE];
>   struct rq *rq;
>   struct rq_flags flags;
>   int ret;
>   bool success = false;
> - char err_buf[STACK_ERR_BUF_SIZE];
>  
>   err_buf[0] = '\0';
>  
> @@ -351,7 +351,6 @@ static bool klp_try_switch_task(struct task_struct *task)
>   pr_debug("%s", err_buf);
>  
>   return success;
> -
>  }
>  
>  /*
> -- 
> 2.16.4
> 

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

2019-04-30 Thread Kamalesh Babulal 
On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote:
> klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static.
> 
> Signed-off-by: Petr Mladek 

Reviewed-by: Kamalesh Babulal

Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

2019-04-30 Thread Miroslav Benes 
On Tue, 30 Apr 2019, Petr Mladek wrote:

> klp_try_switch_task() is called under klp_mutex. The buffer for
> debugging messages might be static.
> 
> Signed-off-by: Petr Mladek 

Acked-by: Miroslav Benes 

Miroslav

[PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock

2019-04-30 Thread Petr Mladek 
klp_try_switch_task() is called under klp_mutex. The buffer for
debugging messages might be static.

Signed-off-by: Petr Mladek 
---
 kernel/livepatch/transition.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 8e0274075e75..d66086fd6d75 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -304,11 +304,11 @@ static int klp_check_stack(struct task_struct *task, char 
*err_buf)
  */
 static bool klp_try_switch_task(struct task_struct *task)
 {
+   static char err_buf[STACK_ERR_BUF_SIZE];
struct rq *rq;
struct rq_flags flags;
int ret;
bool success = false;
-   char err_buf[STACK_ERR_BUF_SIZE];
 
err_buf[0] = '\0';
 
@@ -351,7 +351,6 @@ static bool klp_try_switch_task(struct task_struct *task)
pr_debug("%s", err_buf);
 
return success;
-
 }
 
 /*
-- 
2.16.4