Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 Jan 2021 10:21:15 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Davidlohr Bueso <dave@stgolabs.net>
Cc:     jason.wessel@windriver.com, kgdb-bugreport@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, Davidlohr Bueso <dbueso@suse.de>
Subject: Re: [PATCH] kgdb: Schedule breakpoints via workqueue
Message-ID: <20210115102115.twv3oy3pmnhdejij@maple.lan>
References: <20210115001344.117108-1-dave@stgolabs.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210115001344.117108-1-dave@stgolabs.net>
Precedence: bulk

On Thu, Jan 14, 2021 at 04:13:44PM -0800, Davidlohr Bueso wrote:
> The original functionality was added back in:
> 
>     1cee5e35f15 (kgdb: Add the ability to schedule a breakpoint via a tasklet)
> 
> However tasklets have long been deprecated as being too heavy on
> the system by running in irq context - and this is not a performance
> critical path. If a higher priority process wants to run, it must
> wait for the tasklet to finish before doing so. Instead, generate
> the breakpoint exception in process context.

I don't agree that "this is not a performance critical path".

kgdb is a stop-the-world debugger: if the developer trying to understand
the system behaviour has commanded the system to halt then that is what
it should be doing. It should not be scheduling tasks that are not
necessary to bring the system a halt.

In other words this code is using tasklets *specifically* to benefit
from their weird calling context.

However I am aware the way the wind is blowing w.r.t. tasklets
and...

> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> Compile-tested only.

... this code can only ever be compile tested since AFAIK it has no
in-kernel callers.

There is a (still maintained) out-of-tree user that provides
kgdb-over-ethernet using the netpoll API. It must defer the stop to a
tasklet to avoid problems with netpoll running alongside the RX handler.

Whilst I have some sympathy, that code has been out-of-tree for more
than 10 years and I don't recall any serious attempt to upstream it at
any point in the last five.

So unless someone yells (convincingly) perhaps it's time to rip this
out and help prepare for a tasklet-free future?


Daniel.


> 
>  kernel/debug/debug_core.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index af6e8b4fb359..e1ff974c6b6f 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -119,7 +119,7 @@ static DEFINE_RAW_SPINLOCK(dbg_slave_lock);
>   */
>  static atomic_t			masters_in_kgdb;
>  static atomic_t			slaves_in_kgdb;
> -static atomic_t			kgdb_break_tasklet_var;
> +static atomic_t			kgdb_break_work_var;
>  atomic_t			kgdb_setting_breakpoint;
>  
>  struct task_struct		*kgdb_usethread;
> @@ -1085,27 +1085,27 @@ static void kgdb_unregister_callbacks(void)
>  }
>  
>  /*
> - * There are times a tasklet needs to be used vs a compiled in
> + * There are times a workqueue needs to be used vs a compiled in
>   * break point so as to cause an exception outside a kgdb I/O module,
>   * such as is the case with kgdboe, where calling a breakpoint in the
>   * I/O driver itself would be fatal.
>   */
> -static void kgdb_tasklet_bpt(unsigned long ing)
> +static void kgdb_work_bpt(struct work_struct *unused)
>  {
>  	kgdb_breakpoint();
> -	atomic_set(&kgdb_break_tasklet_var, 0);
> +	atomic_set(&kgdb_break_work_var, 0);
>  }
>  
> -static DECLARE_TASKLET_OLD(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);
> +static DECLARE_WORK(kgdb_async_breakpoint, kgdb_work_bpt);
>  
>  void kgdb_schedule_breakpoint(void)
>  {
> -	if (atomic_read(&kgdb_break_tasklet_var) ||
> +	if (atomic_read(&kgdb_break_work_var) ||
>  		atomic_read(&kgdb_active) != -1 ||
>  		atomic_read(&kgdb_setting_breakpoint))
>  		return;
> -	atomic_inc(&kgdb_break_tasklet_var);
> -	tasklet_schedule(&kgdb_tasklet_breakpoint);
> +	atomic_inc(&kgdb_break_work_var);
> +	schedule_work(&kgdb_async_breakpoint);
>  }
>  EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
>  
> -- 
> 2.26.2
>