| Message ID | 20230804090621.400-2-elver@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] compiler_types: Introduce the Clang __preserve_most function attribute | expand |
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
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);
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(-)