[PATCH 0/5] cleanups

[PATCH 0/5] cleanups

[Jules Irenge]

Jules Irenge (5):
  rcu/rcutorture: replace 0 with false
  rcu: replace 1 with true
  rcu: replace + with |
  x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and
    ftrace_arch_code_modify_post_process()
  sfc: add  missing annotation for efx_ef10_try_update_nic_stats_vf()

 arch/x86/kernel/ftrace.c        |  2 ++
 drivers/net/ethernet/sfc/ef10.c |  1 +
 kernel/rcu/rcutorture.c         |  2 +-
 kernel/rcu/tree_plugin.h        | 22 +++++++++++-----------
 kernel/rcu/update.c             |  2 +-
 5 files changed, 16 insertions(+), 13 deletions(-)

-- 
2.18.2

[PATCH 1/5] rcu/rcutorture: replace 0 with false

[Jules Irenge]

Coccinelle reports a warning

WARNING: Assignment of 0/1 to bool variable

The root cause is the variable lastphase  is of bool type is initialised with integer 0
Replacing 0 with false fixes the issue.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/rcu/rcutorture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 5453bd557f43..a082bc402f9b 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -2065,7 +2065,7 @@ static void rcu_torture_barrier1cb(void *rcu_void)
 static int rcu_torture_barrier_cbs(void *arg)
 {
 	long myid = (long)arg;
-	bool lastphase = 0;
+	bool lastphase = false;
 	bool newphase;
 	struct rcu_head rcu;
 
-- 
2.18.2

[PATCH 2/5] rcu: replace 1 with true

[Jules Irenge]

Coccinelle reports a warning

WARNING: Assignment of 0/1 to bool variable

The root cause is the variable rcu_boot_ended  of bool type is initialised with integer 1
Replacing 1 with true fixes the issue.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/rcu/update.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 28a8bdc5072f..c18ae0cca512 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -193,7 +193,7 @@ void rcu_end_inkernel_boot(void)
 	rcu_unexpedite_gp();
 	if (rcu_normal_after_boot)
 		WRITE_ONCE(rcu_normal, 1);
-	rcu_boot_ended = 1;
+	rcu_boot_ended = true;
 }
 
 /*
-- 
2.18.2

[PATCH 3/5] rcu: replace + with |

[Jules Irenge]

Coccinelle reports warnings at rcu_preempt_ctxt_queue()

WARNING: sum of probable bitmasks, consider |

The root cause is the use of addition operator + for bitmask defined macros variables
Replacing + with | fixes the issue.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/rcu/tree_plugin.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 097635c41135..a20135ece06a 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -153,9 +153,9 @@ static void rcu_preempt_ctxt_queue(struct rcu_node *rnp, struct rcu_data *rdp)
 	switch (blkd_state) {
 	case 0:
 	case                RCU_EXP_TASKS:
-	case                RCU_EXP_TASKS + RCU_GP_BLKD:
+	case                RCU_EXP_TASKS | RCU_GP_BLKD:
 	case RCU_GP_TASKS:
-	case RCU_GP_TASKS + RCU_EXP_TASKS:
+	case RCU_GP_TASKS | RCU_EXP_TASKS:
 
 		/*
 		 * Blocking neither GP, or first task blocking the normal
@@ -168,10 +168,10 @@ static void rcu_preempt_ctxt_queue(struct rcu_node *rnp, struct rcu_data *rdp)
 
 	case                                              RCU_EXP_BLKD:
 	case                                RCU_GP_BLKD:
-	case                                RCU_GP_BLKD + RCU_EXP_BLKD:
-	case RCU_GP_TASKS +                               RCU_EXP_BLKD:
-	case RCU_GP_TASKS +                 RCU_GP_BLKD + RCU_EXP_BLKD:
-	case RCU_GP_TASKS + RCU_EXP_TASKS + RCU_GP_BLKD + RCU_EXP_BLKD:
+	case                                RCU_GP_BLKD | RCU_EXP_BLKD:
+	case RCU_GP_TASKS |                               RCU_EXP_BLKD:
+	case RCU_GP_TASKS |                 RCU_GP_BLKD | RCU_EXP_BLKD:
+	case RCU_GP_TASKS | RCU_EXP_TASKS | RCU_GP_BLKD | RCU_EXP_BLKD:
 
 		/*
 		 * First task arriving that blocks either GP, or first task
@@ -184,9 +184,9 @@ static void rcu_preempt_ctxt_queue(struct rcu_node *rnp, struct rcu_data *rdp)
 		list_add_tail(&t->rcu_node_entry, &rnp->blkd_tasks);
 		break;
 
-	case                RCU_EXP_TASKS +               RCU_EXP_BLKD:
-	case                RCU_EXP_TASKS + RCU_GP_BLKD + RCU_EXP_BLKD:
-	case RCU_GP_TASKS + RCU_EXP_TASKS +               RCU_EXP_BLKD:
+	case                RCU_EXP_TASKS |               RCU_EXP_BLKD:
+	case                RCU_EXP_TASKS | RCU_GP_BLKD | RCU_EXP_BLKD:
+	case RCU_GP_TASKS | RCU_EXP_TASKS |               RCU_EXP_BLKD:
 
 		/*
 		 * Second or subsequent task blocking the expedited GP.
@@ -197,8 +197,8 @@ static void rcu_preempt_ctxt_queue(struct rcu_node *rnp, struct rcu_data *rdp)
 		list_add(&t->rcu_node_entry, rnp->exp_tasks);
 		break;
 
-	case RCU_GP_TASKS +                 RCU_GP_BLKD:
-	case RCU_GP_TASKS + RCU_EXP_TASKS + RCU_GP_BLKD:
+	case RCU_GP_TASKS |                 RCU_GP_BLKD:
+	case RCU_GP_TASKS | RCU_EXP_TASKS | RCU_GP_BLKD:
 
 		/*
 		 * Second or subsequent task blocking the normal GP.
-- 
2.18.2

[PATCH 4/5] x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and ftrace_arch_code_modify_post_process()

[Jules Irenge]

Sparse reports warnings

warning: context imbalance in ftrace_arch_code_modify_prepare()
	- wrong count at exit
warning: context imbalance in ftrace_arch_code_modify_post_process()
	- wrong count at exit

The root cause is that even if
 the annotations on the function are correct,
mutex do not support annotation
This makes Sparse to complain.
To fix this,
__acquire(&text_mutex) and
 __release(&text_mutex) annotations are added
inside ftrace_arch_code_modify_prepare()
and ftrace_arch_code_modify_post_process()
respectively.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 arch/x86/kernel/ftrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 37a0aeaf89e7..737c07ab2e07 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -46,6 +46,7 @@ int ftrace_arch_code_modify_prepare(void)
 	 * ftrace has it set to "read/write".
 	 */
 	mutex_lock(&text_mutex);
+	__acquire(&text_mutex);
 	ftrace_poke_late = 1;
 	return 0;
 }
@@ -61,6 +62,7 @@ int ftrace_arch_code_modify_post_process(void)
 	text_poke_finish();
 	ftrace_poke_late = 0;
 	mutex_unlock(&text_mutex);
+	__release(&text_mutex);
 	return 0;
 }
 
-- 
2.18.2

Re: [PATCH 4/5] x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and ftrace_arch_code_modify_post_process()

[Steven Rostedt]

On Mon,  1 Jun 2020 19:45:51 +0100
Jules Irenge <jbi.octave@gmail.com> wrote:

> Sparse reports warnings
> 
> warning: context imbalance in ftrace_arch_code_modify_prepare()
> 	- wrong count at exit
> warning: context imbalance in ftrace_arch_code_modify_post_process()
> 	- wrong count at exit
> 
> The root cause is that even if
>  the annotations on the function are correct,
> mutex do not support annotation
> This makes Sparse to complain.
> To fix this,
> __acquire(&text_mutex) and
>  __release(&text_mutex) annotations are added
> inside ftrace_arch_code_modify_prepare()
> and ftrace_arch_code_modify_post_process()
> respectively.

Wait what? This looks like either a bug in sparse, or we just remove the
annotations. This just makes the code ugly, and looks silly.

Nack!

-- Steve


> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  arch/x86/kernel/ftrace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 37a0aeaf89e7..737c07ab2e07 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -46,6 +46,7 @@ int ftrace_arch_code_modify_prepare(void)
>  	 * ftrace has it set to "read/write".
>  	 */
>  	mutex_lock(&text_mutex);
> +	__acquire(&text_mutex);
>  	ftrace_poke_late = 1;
>  	return 0;
>  }
> @@ -61,6 +62,7 @@ int ftrace_arch_code_modify_post_process(void)
>  	text_poke_finish();
>  	ftrace_poke_late = 0;
>  	mutex_unlock(&text_mutex);
> +	__release(&text_mutex);
>  	return 0;
>  }
>  

Re: [PATCH 4/5] x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and ftrace_arch_code_modify_post_process()

[ý€€€„]

On Mon, 1 Jun 2020, Steven Rostedt wrote:

> On Mon,  1 Jun 2020 19:45:51 +0100
> Jules Irenge <jbi.octave@gmail.com> wrote:
>
>> Sparse reports warnings
>>
>> warning: context imbalance in ftrace_arch_code_modify_prepare()
>> 	- wrong count at exit
>> warning: context imbalance in ftrace_arch_code_modify_post_process()
>> 	- wrong count at exit
>>
>> The root cause is that even if
>>  the annotations on the function are correct,
>> mutex do not support annotation
>> This makes Sparse to complain.
>> To fix this,
>> __acquire(&text_mutex) and
>>  __release(&text_mutex) annotations are added
>> inside ftrace_arch_code_modify_prepare()
>> and ftrace_arch_code_modify_post_process()
>> respectively.
>
> Wait what? This looks like either a bug in sparse, or we just remove the
> annotations. This just makes the code ugly, and looks silly.
>
> Nack!
>
> -- Steve
>
>
>
Thanks for the feedback, I take good note.
Jules

Re: [PATCH 4/5] x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and ftrace_arch_code_modify_post_process()

[Luc Van Oostenryck]

On Mon, Jun 01, 2020 at 03:46:47PM -0400, Steven Rostedt wrote:
> On Mon,  1 Jun 2020 19:45:51 +0100
> Jules Irenge <jbi.octave@gmail.com> wrote:
> 
> > Sparse reports warnings
> > 
> > warning: context imbalance in ftrace_arch_code_modify_prepare()
> > 	- wrong count at exit
> > warning: context imbalance in ftrace_arch_code_modify_post_process()
> > 	- wrong count at exit
> > 
> > The root cause is that even if
> >  the annotations on the function are correct,
> > mutex do not support annotation

Yes.

> Wait what? This looks like either a bug in sparse, or we just remove the
> annotations. This just makes the code ugly, and looks silly.

The annotations added by commit
  074376ac0e1d ("ftrace/x86: Anotate text_mutex split between ...
are indeed wrong (because they don't match what the functions are
really doing / mutex operations have never been annotated).
The're also pointless since their prototypes are un-annotated.

-- Luc

Re: [PATCH 4/5] x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and ftrace_arch_code_modify_post_process()

[ý€€€„]

On Wed, 3 Jun 2020, Luc Van Oostenryck wrote:

> On Mon, Jun 01, 2020 at 03:46:47PM -0400, Steven Rostedt wrote:
>> On Mon,  1 Jun 2020 19:45:51 +0100
>> Jules Irenge <jbi.octave@gmail.com> wrote:
>>
>>> Sparse reports warnings
>>>
>>> warning: context imbalance in ftrace_arch_code_modify_prepare()
>>> 	- wrong count at exit
>>> warning: context imbalance in ftrace_arch_code_modify_post_process()
>>> 	- wrong count at exit
>>>
>>> The root cause is that even if
>>>  the annotations on the function are correct,
>>> mutex do not support annotation
>
> Yes.
>
>> Wait what? This looks like either a bug in sparse, or we just remove the
>> annotations. This just makes the code ugly, and looks silly.
>
> The annotations added by commit
>  074376ac0e1d ("ftrace/x86: Anotate text_mutex split between ...
> are indeed wrong (because they don't match what the functions are
> really doing / mutex operations have never been annotated).
> The're also pointless since their prototypes are un-annotated.

Interesting, I would think the best way would then be to remove the 
annotations. There are quite a number of them.

I will have to investigate more on mutex and annotation before moving 
forward.

Thanks for feedback.

Jules

[PATCH 5/5] sfc: add missing annotation for efx_ef10_try_update_nic_stats_vf()

[Jules Irenge]

Sparse reports a warning at efx_ef10_try_update_nic_stats_vf()
warning: context imbalance in efx_ef10_try_update_nic_stats_vf()
	- unexpected unlock
The root cause is the missing annotation at
efx_ef10_try_update_nic_stats_vf()
Add the missing _must_hold(&efx->stats_lock) annotation

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 drivers/net/ethernet/sfc/ef10.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 3f16bd807c6e..e8bbbd366625 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -1820,6 +1820,7 @@ static size_t efx_ef10_update_stats_pf(struct efx_nic *efx, u64 *full_stats,
 }
 
 static int efx_ef10_try_update_nic_stats_vf(struct efx_nic *efx)
+	__must_hold(&efx->stats_lock)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_MAC_STATS_IN_LEN);
 	struct efx_ef10_nic_data *nic_data = efx->nic_data;
-- 
2.18.2

Re: [PATCH 5/5] sfc: add missing annotation for efx_ef10_try_update_nic_stats_vf()

[David Miller]

From: Jules Irenge <jbi.octave@gmail.com>
Date: Mon,  1 Jun 2020 19:45:52 +0100

> Sparse reports a warning at efx_ef10_try_update_nic_stats_vf()
> warning: context imbalance in efx_ef10_try_update_nic_stats_vf()
> 	- unexpected unlock
> The root cause is the missing annotation at
> efx_ef10_try_update_nic_stats_vf()
> Add the missing _must_hold(&efx->stats_lock) annotation
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Applied.

Re: [PATCH 0/5] cleanups

[Paul E. McKenney]

On Mon, Jun 01, 2020 at 07:45:47PM +0100, Jules Irenge wrote:
> 
> Jules Irenge (5):
>   rcu/rcutorture: replace 0 with false
>   rcu: replace 1 with true

I queued these two, thank you!

>   rcu: replace + with |

This one I am not all that excited about, so I am leaving it off.

							Thanx, Paul

>   x86/ftrace: Add annotations for ftrace_arch_code_modify_prepare() and
>     ftrace_arch_code_modify_post_process()
>   sfc: add  missing annotation for efx_ef10_try_update_nic_stats_vf()
> 
>  arch/x86/kernel/ftrace.c        |  2 ++
>  drivers/net/ethernet/sfc/ef10.c |  1 +
>  kernel/rcu/rcutorture.c         |  2 +-
>  kernel/rcu/tree_plugin.h        | 22 +++++++++++-----------
>  kernel/rcu/update.c             |  2 +-
>  5 files changed, 16 insertions(+), 13 deletions(-)
> 
> -- 
> 2.18.2
> 

Re: [PATCH 0/5] cleanups

[ý€€€„]

On Tue, 2 Jun 2020, Paul E. McKenney wrote:

> On Mon, Jun 01, 2020 at 07:45:47PM +0100, Jules Irenge wrote:
>>
>> Jules Irenge (5):
>>   rcu/rcutorture: replace 0 with false
>>   rcu: replace 1 with true
>
> I queued these two, thank you!
>
>>   rcu: replace + with |
>
> This one I am not all that excited about, so I am leaving it off.
>
> 							Thanx, Paul

Thanks for the feedback.

Jules