[v2,2/3] list_debug: Introduce inline wrappers for debug checks

  Turn the list debug checking functions __list_*_valid() into inline
functions that wrap the out-of-line functions. Care is taken to ensure
the inline wrappers are always inlined, so that additional compiler
instrumentation (such as sanitizers) does not result in redundant
outlining.

This change is preparation for performing checks in the inline wrappers.

No functional change intended.

Signed-off-by: Marco Elver <elver@google.com>
---
 arch/arm64/kvm/hyp/nvhe/list_debug.c |  6 +++---
 include/linux/list.h                 | 15 +++++++++++++--
 lib/list_debug.c                     | 11 +++++------
 3 files changed, 21 insertions(+), 11 deletions(-)
  

On Fri,  4 Aug 2023 11:02:57 +0200
Marco Elver <elver@google.com> wrote:

> Turn the list debug checking functions __list_*_valid() into inline
> functions that wrap the out-of-line functions. Care is taken to ensure
> the inline wrappers are always inlined, so that additional compiler
> instrumentation (such as sanitizers) does not result in redundant
> outlining.
> 
> This change is preparation for performing checks in the inline wrappers.
> 
> No functional change intended.

I think the entire underscoring functions calling more underscoring
functions in the kernel is an abomination. Yes, there's lots of precedence
to this craziness, but let's not extend it.

Can we give actual real names to why the function is "special" besides that
it now has another underscore added to it?

I've been guilty of this madness myself, but I have learned the errors of
my ways, and have been avoiding doing so in any new code I write.

-- Steve


> 
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/list_debug.c |  6 +++---
>  include/linux/list.h                 | 15 +++++++++++++--
>  lib/list_debug.c                     | 11 +++++------
>  3 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/list_debug.c b/arch/arm64/kvm/hyp/nvhe/list_debug.c
> index d68abd7ea124..589284496ac5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/list_debug.c
> +++ b/arch/arm64/kvm/hyp/nvhe/list_debug.c
> @@ -26,8 +26,8 @@ static inline __must_check bool nvhe_check_data_corruption(bool v)
>  
>  /* The predicates checked here are taken from lib/list_debug.c. */
>  
> -bool __list_add_valid(struct list_head *new, struct list_head *prev,
> -		      struct list_head *next)
> +bool ___list_add_valid(struct list_head *new, struct list_head *prev,
> +		       struct list_head *next)
>  {
>  	if (NVHE_CHECK_DATA_CORRUPTION(next->prev != prev) ||
>  	    NVHE_CHECK_DATA_CORRUPTION(prev->next != next) ||
> @@ -37,7 +37,7 @@ bool __list_add_valid(struct list_head *new, struct list_head *prev,
>  	return true;
>  }
>  
> -bool __list_del_entry_valid(struct list_head *entry)
> +bool ___list_del_entry_valid(struct list_head *entry)
>  {
>  	struct list_head *prev, *next;
>  
> diff --git a/include/linux/list.h b/include/linux/list.h
> index f10344dbad4d..e0b2cf904409 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -39,10 +39,21 @@ static inline void INIT_LIST_HEAD(struct list_head *list)
>  }
>  
>  #ifdef CONFIG_DEBUG_LIST
> -extern bool __list_add_valid(struct list_head *new,
> +extern bool ___list_add_valid(struct list_head *new,
>  			      struct list_head *prev,
>  			      struct list_head *next);
> -extern bool __list_del_entry_valid(struct list_head *entry);
> +static __always_inline bool __list_add_valid(struct list_head *new,
> +					     struct list_head *prev,
> +					     struct list_head *next)
> +{
> +	return ___list_add_valid(new, prev, next);
> +}
> +
> +extern bool ___list_del_entry_valid(struct list_head *entry);
> +static __always_inline bool __list_del_entry_valid(struct list_head *entry)
> +{
> +	return ___list_del_entry_valid(entry);
> +}
>  #else
>  static inline bool __list_add_valid(struct list_head *new,
>  				struct list_head *prev,
> diff --git a/lib/list_debug.c b/lib/list_debug.c
> index d98d43f80958..fd69009cc696 100644
> --- a/lib/list_debug.c
> +++ b/lib/list_debug.c
> @@ -17,8 +17,8 @@
>   * attempt).
>   */
>  
> -bool __list_add_valid(struct list_head *new, struct list_head *prev,
> -		      struct list_head *next)
> +bool ___list_add_valid(struct list_head *new, struct list_head *prev,
> +		       struct list_head *next)
>  {
>  	if (CHECK_DATA_CORRUPTION(prev == NULL,
>  			"list_add corruption. prev is NULL.\n") ||
> @@ -37,9 +37,9 @@ bool __list_add_valid(struct list_head *new, struct list_head *prev,
>  
>  	return true;
>  }
> -EXPORT_SYMBOL(__list_add_valid);
> +EXPORT_SYMBOL(___list_add_valid);
>  
> -bool __list_del_entry_valid(struct list_head *entry)
> +bool ___list_del_entry_valid(struct list_head *entry)
>  {
>  	struct list_head *prev, *next;
>  
> @@ -65,6 +65,5 @@ bool __list_del_entry_valid(struct list_head *entry)
>  		return false;
>  
>  	return true;
> -
>  }
> -EXPORT_SYMBOL(__list_del_entry_valid);
> +EXPORT_SYMBOL(___list_del_entry_valid);
  

On Fri, 4 Aug 2023 at 18:03, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri,  4 Aug 2023 11:02:57 +0200
> Marco Elver <elver@google.com> wrote:
>
> > Turn the list debug checking functions __list_*_valid() into inline
> > functions that wrap the out-of-line functions. Care is taken to ensure
> > the inline wrappers are always inlined, so that additional compiler
> > instrumentation (such as sanitizers) does not result in redundant
> > outlining.
> >
> > This change is preparation for performing checks in the inline wrappers.
> >
> > No functional change intended.
>
> I think the entire underscoring functions calling more underscoring
> functions in the kernel is an abomination. Yes, there's lots of precedence
> to this craziness, but let's not extend it.
>
> Can we give actual real names to why the function is "special" besides that
> it now has another underscore added to it?
>
> I've been guilty of this madness myself, but I have learned the errors of
> my ways, and have been avoiding doing so in any new code I write.

That's fair. We can call them __list_*_valid() (inline), and
__list_*_valid_or_report() ?
  

On Fri, 4 Aug 2023 19:49:48 +0200
Marco Elver <elver@google.com> wrote:

> > I've been guilty of this madness myself, but I have learned the errors of
> > my ways, and have been avoiding doing so in any new code I write.  
> 
> That's fair. We can call them __list_*_valid() (inline), and
> __list_*_valid_or_report() ?

__list_*_valid_check() ?

-- Steve
  

On Fri, 4 Aug 2023 13:57:57 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 4 Aug 2023 19:49:48 +0200
> Marco Elver <elver@google.com> wrote:
> 
> > > I've been guilty of this madness myself, but I have learned the errors of
> > > my ways, and have been avoiding doing so in any new code I write.    
> > 
> > That's fair. We can call them __list_*_valid() (inline), and
> > __list_*_valid_or_report() ?  
> 
> __list_*_valid_check() ?
> 

I have to admit, I think the main reason kernel developers default to using
these useless underscores is because kernel developers are notoriously
lousy at naming. ;-)

-- Steve
  

On Fri, 4 Aug 2023 at 19:59, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 4 Aug 2023 13:57:57 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Fri, 4 Aug 2023 19:49:48 +0200
> > Marco Elver <elver@google.com> wrote:
> >
> > > > I've been guilty of this madness myself, but I have learned the errors of
> > > > my ways, and have been avoiding doing so in any new code I write.
> > >
> > > That's fair. We can call them __list_*_valid() (inline), and
> > > __list_*_valid_or_report() ?
> >
> > __list_*_valid_check() ?

Well, in patch 3/3, the inline function will also do a reduced set of
checking, so "valid_check" is also misleading because both will do
checks.

The key distinguishing thing between the inline and non-inline version
is that the non-inline version will check more things, and also
produce reports.

So I can see

 1. __list_*_valid_or_report()
 2. __list_*_full_valid()

To be appropriate. Preference?

> I have to admit, I think the main reason kernel developers default to using
> these useless underscores is because kernel developers are notoriously
> lousy at naming. ;-)

Heh, naming is hard. ;-)
  

On Fri, Aug 04, 2023 at 01:59:02PM -0400, Steven Rostedt wrote:
> On Fri, 4 Aug 2023 13:57:57 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 4 Aug 2023 19:49:48 +0200
> > Marco Elver <elver@google.com> wrote:
> > 
> > > > I've been guilty of this madness myself, but I have learned the errors of
> > > > my ways, and have been avoiding doing so in any new code I write.    
> > > 
> > > That's fair. We can call them __list_*_valid() (inline), and
> > > __list_*_valid_or_report() ?  
> > 
> > __list_*_valid_check() ?
> > 
> 
> I have to admit, I think the main reason kernel developers default to using
> these useless underscores is because kernel developers are notoriously
> lousy at naming. ;-)

Well, that and I detest novella length identifiers.
  

On Fri, Aug 4, 2023 at 6:03 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Can we give actual real names to why the function is "special" besides that
> it now has another underscore added to it?

+1, some docs on top of that can also help a lot to explain the
intended difference quickly to a reader.

Cheers,
Miguel