Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On 14/12/16 14:42, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 11:33:19AM +, Colin Ian King wrote: >> On 13/12/16 11:21, Boqun Feng wrote: >>> On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: From: Colin Ian Kingmask and bit are unsigned longs, so if bit is 31 we end up sign extending the 1 and mask ends up as 0x8000. Fix this by explicitly adding integer suffix UL ensure 1 is a unsigned long rather than an signed int. >>> >>> Right, you are, and the tool is ;-) >>> >>> If @bit is greater than 32, we even got an undefined behavior in C ;-( >>> This is my careless mistake, thank you for finding it out and fix it! >>> Issue found with static analysis with CoverityScan, CID 1388564 Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") Signed-off-by: Colin Ian King >>> >>> I think Paul only queued that for running tests and I have almost >>> finished a v2. I will fold your fix in my patch and add your SoB along >>> with mine, does that work for you? >> >> Sure, that's good with me. >> > > Colin, as I'm going to take Mark's suggestion and use > leaf_node_cpu_bit() instead. So I'm going to drop your SoB but keep a > commit message saying you spotted the problem at the first place. Hope > that works with you ;-) Yep, that's totally fine. Thanks. Colin > > Regards, > Boqun > >>> >>> TBH, this situation is kinda new to me, so if anyone has any suggestion, >>> please let me know ;-) >>> >>> Regards, >>> Boqun >>> --- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 10162ac..6ecedd8 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) - mask |= 1 << bit; + mask |= 1UL << bit; if (mask != 0) { /* Idle/offline CPUs, report (releases rnp->lock. */ -- 2.10.2 >> >> > > > signature.asc Description: OpenPGP digital signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On 14/12/16 14:42, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 11:33:19AM +, Colin Ian King wrote: >> On 13/12/16 11:21, Boqun Feng wrote: >>> On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: From: Colin Ian King mask and bit are unsigned longs, so if bit is 31 we end up sign extending the 1 and mask ends up as 0x8000. Fix this by explicitly adding integer suffix UL ensure 1 is a unsigned long rather than an signed int. >>> >>> Right, you are, and the tool is ;-) >>> >>> If @bit is greater than 32, we even got an undefined behavior in C ;-( >>> This is my careless mistake, thank you for finding it out and fix it! >>> Issue found with static analysis with CoverityScan, CID 1388564 Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") Signed-off-by: Colin Ian King >>> >>> I think Paul only queued that for running tests and I have almost >>> finished a v2. I will fold your fix in my patch and add your SoB along >>> with mine, does that work for you? >> >> Sure, that's good with me. >> > > Colin, as I'm going to take Mark's suggestion and use > leaf_node_cpu_bit() instead. So I'm going to drop your SoB but keep a > commit message saying you spotted the problem at the first place. Hope > that works with you ;-) Yep, that's totally fine. Thanks. Colin > > Regards, > Boqun > >>> >>> TBH, this situation is kinda new to me, so if anyone has any suggestion, >>> please let me know ;-) >>> >>> Regards, >>> Boqun >>> --- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 10162ac..6ecedd8 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) - mask |= 1 << bit; + mask |= 1UL << bit; if (mask != 0) { /* Idle/offline CPUs, report (releases rnp->lock. */ -- 2.10.2 >> >> > > > signature.asc Description: OpenPGP digital signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 11:33:19AM +, Colin Ian King wrote: > On 13/12/16 11:21, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > >> From: Colin Ian King> >> > >> mask and bit are unsigned longs, so if bit is 31 we end up sign > >> extending the 1 and mask ends up as 0x8000. Fix this > >> by explicitly adding integer suffix UL ensure 1 is a unsigned long > >> rather than an signed int. > >> > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > >> Issue found with static analysis with CoverityScan, CID 1388564 > >> > >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() > >> in force_qs_rnp()") > >> Signed-off-by: Colin Ian King > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > Sure, that's good with me. > Colin, as I'm going to take Mark's suggestion and use leaf_node_cpu_bit() instead. So I'm going to drop your SoB but keep a commit message saying you spotted the problem at the first place. Hope that works with you ;-) Regards, Boqun > > > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > >> --- > >> kernel/rcu/tree.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 10162ac..6ecedd8 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > >> > >>leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > >>if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > >> - mask |= 1 << bit; > >> + mask |= 1UL << bit; > >> > >>if (mask != 0) { > >>/* Idle/offline CPUs, report (releases rnp->lock. */ > >> -- > >> 2.10.2 > >> > > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 11:33:19AM +, Colin Ian King wrote: > On 13/12/16 11:21, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > >> From: Colin Ian King > >> > >> mask and bit are unsigned longs, so if bit is 31 we end up sign > >> extending the 1 and mask ends up as 0x8000. Fix this > >> by explicitly adding integer suffix UL ensure 1 is a unsigned long > >> rather than an signed int. > >> > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > >> Issue found with static analysis with CoverityScan, CID 1388564 > >> > >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() > >> in force_qs_rnp()") > >> Signed-off-by: Colin Ian King > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > Sure, that's good with me. > Colin, as I'm going to take Mark's suggestion and use leaf_node_cpu_bit() instead. So I'm going to drop your SoB but keep a commit message saying you spotted the problem at the first place. Hope that works with you ;-) Regards, Boqun > > > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > >> --- > >> kernel/rcu/tree.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 10162ac..6ecedd8 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > >> > >>leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > >>if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > >> - mask |= 1 << bit; > >> + mask |= 1UL << bit; > >> > >>if (mask != 0) { > >>/* Idle/offline CPUs, report (releases rnp->lock. */ > >> -- > >> 2.10.2 > >> > > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 10:55:07AM +, Mark Rutland wrote: > On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > > > MASK_BITS(mask)); \ > > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > > if (!cpu_possible(cpu)) \ > > > > > continue; \ > > > > > else > > > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > > just an over-care check. > > > > I think I probably will remove this check eventually, let me audit the > > code a little more for safety ;-) > > FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse > possible cpus"), the only places I saw accesses to (percpu) data for > !possible cpus were the places I fixed up. IIRC I tested with a version > of the patch below. > > That won't catch erroneous non-percpu accesses, but it covers part of > the problem, at least. ;) > Good point ;-) I will also add a WARN_ON_ONCE() in macro for_each_leaf_node_cpu() and help me watch if anyone try to play an interesting game on those masks. Regards, Boqun > Thanks, > Mark. > > >8 > From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 > From: Mark Rutland> Date: Mon, 16 May 2016 16:08:29 +0100 > Subject: [PATCH] percpu: add possible cpu validation > > Recently, the RCU tree code was seen to access per-cpu data for CPUs not > in cpu_possible_mask, for which a per-cpu region (and offset) had not > been allocated. Often this went unnoticed because valid (but erroneous) > pointers were generated, and the accesses hit some other data. > > This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr > will verify that the provided CPU id is possible, and therefore there is > a backing percpu area. When the CPU is not possible, we WARN, though the > access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU > behaviour. > > As the option can adversely affect performance, it is disabled by > default. > > Signed-off-by: Mark Rutland > --- > include/linux/percpu-defs.h | 16 ++-- > lib/Kconfig.debug | 10 ++ > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h > index 8f16299..1525352 100644 > --- a/include/linux/percpu-defs.h > +++ b/include/linux/percpu-defs.h > @@ -207,6 +207,16 @@ > (void)__vpp_verify; \ > } while (0) > > +/* > + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid > + * percpu region. > + */ > +#ifdef CONFIG_DEBUG_PER_CPU > +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) > +#else > +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) > +#endif > + > #ifdef CONFIG_SMP > > /* > @@ -219,8 +229,10 @@ > > #define per_cpu_ptr(ptr, cpu) > \ > ({ \ > + int cpu = (cpu);\ > + __verify_pcpu_cpu(cpu); \ > __verify_pcpu_ptr(ptr); \ > - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ > + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ > }) > > #define raw_cpu_ptr(ptr) \ > @@ -247,7 +259,7 @@ > (typeof(*(__p)) __kernel __force *)(__p); \ > }) > > -#define per_cpu_ptr(ptr, cpu)({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); > }) > +#define per_cpu_ptr(ptr, cpu)({ __verify_pcpu_cpu(cpu); > VERIFY_PERCPU_PTR(ptr); }) > #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) > #define this_cpu_ptr(ptr)raw_cpu_ptr(ptr) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a6c8db1..14678d2 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS > > Say N if unsure. > > +config DEBUG_PER_CPU > + bool "Debug access to percpu objects" > + depends on DEBUG_KERNEL > + help > + Say Y to verify that addresses are only generated for valid
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 10:55:07AM +, Mark Rutland wrote: > On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > > > MASK_BITS(mask)); \ > > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > > if (!cpu_possible(cpu)) \ > > > > > continue; \ > > > > > else > > > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > > just an over-care check. > > > > I think I probably will remove this check eventually, let me audit the > > code a little more for safety ;-) > > FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse > possible cpus"), the only places I saw accesses to (percpu) data for > !possible cpus were the places I fixed up. IIRC I tested with a version > of the patch below. > > That won't catch erroneous non-percpu accesses, but it covers part of > the problem, at least. ;) > Good point ;-) I will also add a WARN_ON_ONCE() in macro for_each_leaf_node_cpu() and help me watch if anyone try to play an interesting game on those masks. Regards, Boqun > Thanks, > Mark. > > >8 > From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 > From: Mark Rutland > Date: Mon, 16 May 2016 16:08:29 +0100 > Subject: [PATCH] percpu: add possible cpu validation > > Recently, the RCU tree code was seen to access per-cpu data for CPUs not > in cpu_possible_mask, for which a per-cpu region (and offset) had not > been allocated. Often this went unnoticed because valid (but erroneous) > pointers were generated, and the accesses hit some other data. > > This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr > will verify that the provided CPU id is possible, and therefore there is > a backing percpu area. When the CPU is not possible, we WARN, though the > access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU > behaviour. > > As the option can adversely affect performance, it is disabled by > default. > > Signed-off-by: Mark Rutland > --- > include/linux/percpu-defs.h | 16 ++-- > lib/Kconfig.debug | 10 ++ > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h > index 8f16299..1525352 100644 > --- a/include/linux/percpu-defs.h > +++ b/include/linux/percpu-defs.h > @@ -207,6 +207,16 @@ > (void)__vpp_verify; \ > } while (0) > > +/* > + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid > + * percpu region. > + */ > +#ifdef CONFIG_DEBUG_PER_CPU > +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) > +#else > +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) > +#endif > + > #ifdef CONFIG_SMP > > /* > @@ -219,8 +229,10 @@ > > #define per_cpu_ptr(ptr, cpu) > \ > ({ \ > + int cpu = (cpu);\ > + __verify_pcpu_cpu(cpu); \ > __verify_pcpu_ptr(ptr); \ > - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ > + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ > }) > > #define raw_cpu_ptr(ptr) \ > @@ -247,7 +259,7 @@ > (typeof(*(__p)) __kernel __force *)(__p); \ > }) > > -#define per_cpu_ptr(ptr, cpu)({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); > }) > +#define per_cpu_ptr(ptr, cpu)({ __verify_pcpu_cpu(cpu); > VERIFY_PERCPU_PTR(ptr); }) > #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) > #define this_cpu_ptr(ptr)raw_cpu_ptr(ptr) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a6c8db1..14678d2 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS > > Say N if unsure. > > +config DEBUG_PER_CPU > + bool "Debug access to percpu objects" > + depends on DEBUG_KERNEL > + help > + Say Y to verify that addresses are only generated for valid percpu > + objects (i.e. for a
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > > MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), the only places I saw accesses to (percpu) data for !possible cpus were the places I fixed up. IIRC I tested with a version of the patch below. That won't catch erroneous non-percpu accesses, but it covers part of the problem, at least. ;) Thanks, Mark. >8 >From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 From: Mark RutlandDate: Mon, 16 May 2016 16:08:29 +0100 Subject: [PATCH] percpu: add possible cpu validation Recently, the RCU tree code was seen to access per-cpu data for CPUs not in cpu_possible_mask, for which a per-cpu region (and offset) had not been allocated. Often this went unnoticed because valid (but erroneous) pointers were generated, and the accesses hit some other data. This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr will verify that the provided CPU id is possible, and therefore there is a backing percpu area. When the CPU is not possible, we WARN, though the access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU behaviour. As the option can adversely affect performance, it is disabled by default. Signed-off-by: Mark Rutland --- include/linux/percpu-defs.h | 16 ++-- lib/Kconfig.debug | 10 ++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h index 8f16299..1525352 100644 --- a/include/linux/percpu-defs.h +++ b/include/linux/percpu-defs.h @@ -207,6 +207,16 @@ (void)__vpp_verify; \ } while (0) +/* + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid + * percpu region. + */ +#ifdef CONFIG_DEBUG_PER_CPU +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) +#else +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) +#endif + #ifdef CONFIG_SMP /* @@ -219,8 +229,10 @@ #define per_cpu_ptr(ptr, cpu) \ ({ \ + int cpu = (cpu);\ + __verify_pcpu_cpu(cpu); \ __verify_pcpu_ptr(ptr); \ - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ }) #define raw_cpu_ptr(ptr) \ @@ -247,7 +259,7 @@ (typeof(*(__p)) __kernel __force *)(__p); \ }) -#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) +#define per_cpu_ptr(ptr, cpu) ({ __verify_pcpu_cpu(cpu); VERIFY_PERCPU_PTR(ptr); }) #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a6c8db1..14678d2 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS Say N if unsure. +config DEBUG_PER_CPU + bool "Debug access to percpu objects" + depends on DEBUG_KERNEL + help + Say Y to verify that addresses are only generated for valid percpu + objects (i.e. for a possible CPU). This adds additional code and + decreases performance. + + Sey N if unsure. + config DEBUG_HIGHMEM bool "Highmem debugging" depends on DEBUG_KERNEL && HIGHMEM -- 1.9.1
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > > MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), the only places I saw accesses to (percpu) data for !possible cpus were the places I fixed up. IIRC I tested with a version of the patch below. That won't catch erroneous non-percpu accesses, but it covers part of the problem, at least. ;) Thanks, Mark. >8 >From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Mon, 16 May 2016 16:08:29 +0100 Subject: [PATCH] percpu: add possible cpu validation Recently, the RCU tree code was seen to access per-cpu data for CPUs not in cpu_possible_mask, for which a per-cpu region (and offset) had not been allocated. Often this went unnoticed because valid (but erroneous) pointers were generated, and the accesses hit some other data. This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr will verify that the provided CPU id is possible, and therefore there is a backing percpu area. When the CPU is not possible, we WARN, though the access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU behaviour. As the option can adversely affect performance, it is disabled by default. Signed-off-by: Mark Rutland --- include/linux/percpu-defs.h | 16 ++-- lib/Kconfig.debug | 10 ++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h index 8f16299..1525352 100644 --- a/include/linux/percpu-defs.h +++ b/include/linux/percpu-defs.h @@ -207,6 +207,16 @@ (void)__vpp_verify; \ } while (0) +/* + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid + * percpu region. + */ +#ifdef CONFIG_DEBUG_PER_CPU +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) +#else +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) +#endif + #ifdef CONFIG_SMP /* @@ -219,8 +229,10 @@ #define per_cpu_ptr(ptr, cpu) \ ({ \ + int cpu = (cpu);\ + __verify_pcpu_cpu(cpu); \ __verify_pcpu_ptr(ptr); \ - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ }) #define raw_cpu_ptr(ptr) \ @@ -247,7 +259,7 @@ (typeof(*(__p)) __kernel __force *)(__p); \ }) -#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) +#define per_cpu_ptr(ptr, cpu) ({ __verify_pcpu_cpu(cpu); VERIFY_PERCPU_PTR(ptr); }) #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a6c8db1..14678d2 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS Say N if unsure. +config DEBUG_PER_CPU + bool "Debug access to percpu objects" + depends on DEBUG_KERNEL + help + Say Y to verify that addresses are only generated for valid percpu + objects (i.e. for a possible CPU). This adds additional code and + decreases performance. + + Sey N if unsure. + config DEBUG_HIGHMEM bool "Highmem debugging" depends on DEBUG_KERNEL && HIGHMEM -- 1.9.1
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > 2016年12月14日 上午1:17,"Mark Rutland"写道: > > > > > > > > > > Hi, > > > > > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > > > > From: Colin Ian King > > > > > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > > > extending the 1 and mask ends up as 0x8000. Fix this > > > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > > > rather than an signed int. > > > > > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > > > Signed-off-by: Colin Ian King > > > > > > --- > > > > > > kernel/rcu/tree.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > > index 10162ac..6ecedd8 100644 > > > > > > --- a/kernel/rcu/tree.c > > > > > > +++ b/kernel/rcu/tree.c > > > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state > > > > > > *rsp, > > > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > > > bit, cpu) > > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, > > > > > > maxj)) > > > > > > - mask |= 1 << bit; > > > > > > + mask |= 1UL << bit; > > > > > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, > > > > > cpu) > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > > > otherwise. > > > > > > > > Good point. ;-) > > > > > > > > We can > > > > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > > MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) Much better! ;-) Thanx, Paul > Regards, > Boqun > > > Regards, > > Boqun > > > > > Thanx, Paul > > > > > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > > > > > Regards, > > > > Boqun > > > > > > > > > Thanks, > > > > > Mark. > > > > >
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > 2016年12月14日 上午1:17,"Mark Rutland" 写道: > > > > > > > > > > Hi, > > > > > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > > > > From: Colin Ian King > > > > > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > > > extending the 1 and mask ends up as 0x8000. Fix this > > > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > > > rather than an signed int. > > > > > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > > > Signed-off-by: Colin Ian King > > > > > > --- > > > > > > kernel/rcu/tree.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > > index 10162ac..6ecedd8 100644 > > > > > > --- a/kernel/rcu/tree.c > > > > > > +++ b/kernel/rcu/tree.c > > > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state > > > > > > *rsp, > > > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > > > bit, cpu) > > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, > > > > > > maxj)) > > > > > > - mask |= 1 << bit; > > > > > > + mask |= 1UL << bit; > > > > > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, > > > > > cpu) > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > > > otherwise. > > > > > > > > Good point. ;-) > > > > > > > > We can > > > > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > > MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) Much better! ;-) Thanx, Paul > Regards, > Boqun > > > Regards, > > Boqun > > > > > Thanx, Paul > > > > > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > > > > > Regards, > > > > Boqun > > > > > > > > > Thanks, > > > > > Mark. > > > > >
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > 2016年12月14日 上午1:17,"Mark Rutland"写道: > > > > > > > > Hi, > > > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > > > From: Colin Ian King > > > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > > extending the 1 and mask ends up as 0x8000. Fix this > > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > > rather than an signed int. > > > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > > Signed-off-by: Colin Ian King > > > > > --- > > > > > kernel/rcu/tree.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > index 10162ac..6ecedd8 100644 > > > > > --- a/kernel/rcu/tree.c > > > > > +++ b/kernel/rcu/tree.c > > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > > bit, cpu) > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > - mask |= 1 << bit; > > > > > + mask |= 1UL << bit; > > > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > > otherwise. > > > > > > Good point. ;-) > > > > > > We can > > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > MASK_BITS(mask)); \ > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > if (!cpu_possible(cpu)) \ > > > continue; \ > > > else > > > > What is the purpose of the cpu_possible() check? > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is just an over-care check. I think I probably will remove this check eventually, let me audit the code a little more for safety ;-) Regards, Boqun > Regards, > Boqun > > > Thanx, Paul > > > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > > > Regards, > > > Boqun > > > > > > > Thanks, > > > > Mark. > > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > 2016年12月14日 上午1:17,"Mark Rutland" 写道: > > > > > > > > Hi, > > > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > > > From: Colin Ian King > > > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > > extending the 1 and mask ends up as 0x8000. Fix this > > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > > rather than an signed int. > > > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > > Signed-off-by: Colin Ian King > > > > > --- > > > > > kernel/rcu/tree.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > index 10162ac..6ecedd8 100644 > > > > > --- a/kernel/rcu/tree.c > > > > > +++ b/kernel/rcu/tree.c > > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > > bit, cpu) > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > - mask |= 1 << bit; > > > > > + mask |= 1UL << bit; > > > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > > otherwise. > > > > > > Good point. ;-) > > > > > > We can > > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, > > > MASK_BITS(mask)); \ > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > if (!cpu_possible(cpu)) \ > > > continue; \ > > > else > > > > What is the purpose of the cpu_possible() check? > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is just an over-care check. I think I probably will remove this check eventually, let me audit the code a little more for safety ;-) Regards, Boqun > Regards, > Boqun > > > Thanx, Paul > > > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > > > Regards, > > > Boqun > > > > > > > Thanks, > > > > Mark. > > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > 2016年12月14日 上午1:17,"Mark Rutland"写道: > > > > Hi, > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > From: Colin Ian King > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0x8000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > otherwise. > > > > Good point. ;-) > > We can > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > leaf_node_cpu_bit(rnp, cpu) + 1))) \ Oops, this part is wrong.. > if (!cpu_possible(cpu)) \ > continue; \ > else > Fixed version: /* * Iterate over all possible CPUs a leaf RCU node which are still masked in * @mask. * * Note @rnp has to be a leaf node and @mask has to belong to @rnp. */ #define for_each_leaf_node_cpu(rnp, mask, cpu) \ for ((cpu) = (rnp)->grplo + find_first_bit(&(mask), MASK_BITS(mask)); \ (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ (cpu) = (rnp)->grplo + find_next_bit(&(mask), MASK_BITS(mask), \ (cpu) - (rnp)->grplo + 1)) \ if (!cpu_possible(cpu)) \ continue; \ else Regards, Boqun > Typing from my cellphone, plz ignore the bad formatting ;-) > > Regards, > Boqun > > > Thanks, > > Mark. signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > 2016年12月14日 上午1:17,"Mark Rutland" 写道: > > > > Hi, > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > From: Colin Ian King > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0x8000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > otherwise. > > > > Good point. ;-) > > We can > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > leaf_node_cpu_bit(rnp, cpu) + 1))) \ Oops, this part is wrong.. > if (!cpu_possible(cpu)) \ > continue; \ > else > Fixed version: /* * Iterate over all possible CPUs a leaf RCU node which are still masked in * @mask. * * Note @rnp has to be a leaf node and @mask has to belong to @rnp. */ #define for_each_leaf_node_cpu(rnp, mask, cpu) \ for ((cpu) = (rnp)->grplo + find_first_bit(&(mask), MASK_BITS(mask)); \ (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ (cpu) = (rnp)->grplo + find_next_bit(&(mask), MASK_BITS(mask), \ (cpu) - (rnp)->grplo + 1)) \ if (!cpu_possible(cpu)) \ continue; \ else Regards, Boqun > Typing from my cellphone, plz ignore the bad formatting ;-) > > Regards, > Boqun > > > Thanks, > > Mark. signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > 2016年12月14日 上午1:17,"Mark Rutland"写道: > > > > > > Hi, > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > > From: Colin Ian King > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > extending the 1 and mask ends up as 0x8000. Fix this > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > rather than an signed int. > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > Signed-off-by: Colin Ian King > > > > --- > > > > kernel/rcu/tree.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index 10162ac..6ecedd8 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > bit, cpu) > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > - mask |= 1 << bit; > > > > + mask |= 1UL << bit; > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > otherwise. > > > > Good point. ;-) > > > > We can > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > if (!cpu_possible(cpu)) \ > > continue; \ > > else > > What is the purpose of the cpu_possible() check? > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. Regards, Boqun > Thanx, Paul > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > Regards, > > Boqun > > > > > Thanks, > > > Mark. > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > 2016年12月14日 上午1:17,"Mark Rutland" 写道: > > > > > > Hi, > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > > From: Colin Ian King > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > extending the 1 and mask ends up as 0x8000. Fix this > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > rather than an signed int. > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > Signed-off-by: Colin Ian King > > > > --- > > > > kernel/rcu/tree.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index 10162ac..6ecedd8 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > bit, cpu) > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > - mask |= 1 << bit; > > > > + mask |= 1UL << bit; > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > otherwise. > > > > Good point. ;-) > > > > We can > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > if (!cpu_possible(cpu)) \ > > continue; \ > > else > > What is the purpose of the cpu_possible() check? > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. Regards, Boqun > Thanx, Paul > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > Regards, > > Boqun > > > > > Thanks, > > > Mark. > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > 2016年12月14日 上午1:17,"Mark Rutland"写道: > > > > Hi, > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > From: Colin Ian King > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0x8000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > otherwise. > > Good point. ;-) > > We can > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > if (!cpu_possible(cpu)) \ > continue; \ > else What is the purpose of the cpu_possible() check? Thanx, Paul > Typing from my cellphone, plz ignore the bad formatting ;-) > > Regards, > Boqun > > > Thanks, > > Mark.
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > 2016年12月14日 上午1:17,"Mark Rutland" 写道: > > > > Hi, > > > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > From: Colin Ian King > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0x8000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > otherwise. > > Good point. ;-) > > We can > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > if (!cpu_possible(cpu)) \ > continue; \ > else What is the purpose of the cpu_possible() check? Thanx, Paul > Typing from my cellphone, plz ignore the bad formatting ;-) > > Regards, > Boqun > > > Thanks, > > Mark.
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
Hi, On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > From: Colin Ian King> > mask and bit are unsigned longs, so if bit is 31 we end up sign > extending the 1 and mask ends up as 0x8000. Fix this > by explicitly adding integer suffix UL ensure 1 is a unsigned long > rather than an signed int. > > Issue found with static analysis with CoverityScan, CID 1388564 > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in > force_qs_rnp()") > Signed-off-by: Colin Ian King > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 10162ac..6ecedd8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > - mask |= 1 << bit; > + mask |= 1UL << bit; So as to match the rest of the code altered in commit bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), and regardless of naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) mask |= leaf_node_cpu_bit(rnp, cpu); IMO, it would be nice to hide the iterator bit somehow, to match for_each_leaf_node_possible_cpu(), which this largely looks similar to otherwise. Thanks, Mark.
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
Hi, On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > From: Colin Ian King > > mask and bit are unsigned longs, so if bit is 31 we end up sign > extending the 1 and mask ends up as 0x8000. Fix this > by explicitly adding integer suffix UL ensure 1 is a unsigned long > rather than an signed int. > > Issue found with static analysis with CoverityScan, CID 1388564 > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in > force_qs_rnp()") > Signed-off-by: Colin Ian King > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 10162ac..6ecedd8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > - mask |= 1 << bit; > + mask |= 1UL << bit; So as to match the rest of the code altered in commit bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), and regardless of naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) mask |= leaf_node_cpu_bit(rnp, cpu); IMO, it would be nice to hide the iterator bit somehow, to match for_each_leaf_node_possible_cpu(), which this largely looks similar to otherwise. Thanks, Mark.
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 08:25:42PM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 07:21:48PM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > From: Colin Ian King> > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0x8000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > Hmm.. Seems using BIT() here is a good idea, and maybe rename bit as > grp_idx or something. Well, "bit" could be a bit, or it could be the number of the bit. Given that we have "mask", and given that we are shifting by it, it has to be the number of the bit. > Naming, naming, naming.. Often I think that the biggest problem with naming is putting too much time into worrying about it. ;-) But if you want to change the name from "bit" to "bitno" or "bitnum" as part of your updated patchset, I won't object. The reason for preferring either to "grp_idx" is that "bitno" goes well with "mask". But as a general rule, I must follow the usual practice of not favoring renaming patches. Thanx, Paul > Regards, > Boqun > > > > > > > if (mask != 0) { > > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > > -- > > > 2.10.2 > > > > >
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 08:25:42PM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 07:21:48PM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > > From: Colin Ian King > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0x8000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > Hmm.. Seems using BIT() here is a good idea, and maybe rename bit as > grp_idx or something. Well, "bit" could be a bit, or it could be the number of the bit. Given that we have "mask", and given that we are shifting by it, it has to be the number of the bit. > Naming, naming, naming.. Often I think that the biggest problem with naming is putting too much time into worrying about it. ;-) But if you want to change the name from "bit" to "bitno" or "bitnum" as part of your updated patchset, I won't object. The reason for preferring either to "grp_idx" is that "bitno" goes well with "mask". But as a general rule, I must follow the usual practice of not favoring renaming patches. Thanx, Paul > Regards, > Boqun > > > > > > > if (mask != 0) { > > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > > -- > > > 2.10.2 > > > > >
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 11:33:19AM +, Colin Ian King wrote: > On 13/12/16 11:21, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > >> From: Colin Ian King> >> > >> mask and bit are unsigned longs, so if bit is 31 we end up sign > >> extending the 1 and mask ends up as 0x8000. Fix this > >> by explicitly adding integer suffix UL ensure 1 is a unsigned long > >> rather than an signed int. > >> > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > >> Issue found with static analysis with CoverityScan, CID 1388564 > >> > >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() > >> in force_qs_rnp()") > >> Signed-off-by: Colin Ian King > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > Sure, that's good with me. And I have popped that series off of my queue. Thanx, Paul > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > >> --- > >> kernel/rcu/tree.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 10162ac..6ecedd8 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > >> > >>leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > >>if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > >> - mask |= 1 << bit; > >> + mask |= 1UL << bit; > >> > >>if (mask != 0) { > >>/* Idle/offline CPUs, report (releases rnp->lock. */ > >> -- > >> 2.10.2 > >> > >
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 11:33:19AM +, Colin Ian King wrote: > On 13/12/16 11:21, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > >> From: Colin Ian King > >> > >> mask and bit are unsigned longs, so if bit is 31 we end up sign > >> extending the 1 and mask ends up as 0x8000. Fix this > >> by explicitly adding integer suffix UL ensure 1 is a unsigned long > >> rather than an signed int. > >> > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > >> Issue found with static analysis with CoverityScan, CID 1388564 > >> > >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() > >> in force_qs_rnp()") > >> Signed-off-by: Colin Ian King > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > Sure, that's good with me. And I have popped that series off of my queue. Thanx, Paul > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > >> --- > >> kernel/rcu/tree.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 10162ac..6ecedd8 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > >> > >>leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > >>if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > >> - mask |= 1 << bit; > >> + mask |= 1UL << bit; > >> > >>if (mask != 0) { > >>/* Idle/offline CPUs, report (releases rnp->lock. */ > >> -- > >> 2.10.2 > >> > >
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 07:21:48PM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > From: Colin Ian King> > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > extending the 1 and mask ends up as 0x8000. Fix this > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > rather than an signed int. > > > > Right, you are, and the tool is ;-) > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > This is my careless mistake, thank you for finding it out and fix it! > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() > > in force_qs_rnp()") > > Signed-off-by: Colin Ian King > > I think Paul only queued that for running tests and I have almost > finished a v2. I will fold your fix in my patch and add your SoB along > with mine, does that work for you? > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > please let me know ;-) > > Regards, > Boqun > > > --- > > kernel/rcu/tree.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index 10162ac..6ecedd8 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > - mask |= 1 << bit; > > + mask |= 1UL << bit; Hmm.. Seems using BIT() here is a good idea, and maybe rename bit as grp_idx or something. Naming, naming, naming.. Regards, Boqun > > > > if (mask != 0) { > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > -- > > 2.10.2 > > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 07:21:48PM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > > From: Colin Ian King > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > extending the 1 and mask ends up as 0x8000. Fix this > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > rather than an signed int. > > > > Right, you are, and the tool is ;-) > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > This is my careless mistake, thank you for finding it out and fix it! > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() > > in force_qs_rnp()") > > Signed-off-by: Colin Ian King > > I think Paul only queued that for running tests and I have almost > finished a v2. I will fold your fix in my patch and add your SoB along > with mine, does that work for you? > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > please let me know ;-) > > Regards, > Boqun > > > --- > > kernel/rcu/tree.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index 10162ac..6ecedd8 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > - mask |= 1 << bit; > > + mask |= 1UL << bit; Hmm.. Seems using BIT() here is a good idea, and maybe rename bit as grp_idx or something. Naming, naming, naming.. Regards, Boqun > > > > if (mask != 0) { > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > -- > > 2.10.2 > > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On 13/12/16 11:21, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: >> From: Colin Ian King>> >> mask and bit are unsigned longs, so if bit is 31 we end up sign >> extending the 1 and mask ends up as 0x8000. Fix this >> by explicitly adding integer suffix UL ensure 1 is a unsigned long >> rather than an signed int. >> > > Right, you are, and the tool is ;-) > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > This is my careless mistake, thank you for finding it out and fix it! > >> Issue found with static analysis with CoverityScan, CID 1388564 >> >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() >> in force_qs_rnp()") >> Signed-off-by: Colin Ian King > > I think Paul only queued that for running tests and I have almost > finished a v2. I will fold your fix in my patch and add your SoB along > with mine, does that work for you? Sure, that's good with me. > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > please let me know ;-) > > Regards, > Boqun > >> --- >> kernel/rcu/tree.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >> index 10162ac..6ecedd8 100644 >> --- a/kernel/rcu/tree.c >> +++ b/kernel/rcu/tree.c >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, >> >> leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) >> if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) >> -mask |= 1 << bit; >> +mask |= 1UL << bit; >> >> if (mask != 0) { >> /* Idle/offline CPUs, report (releases rnp->lock. */ >> -- >> 2.10.2 >> signature.asc Description: OpenPGP digital signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On 13/12/16 11:21, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: >> From: Colin Ian King >> >> mask and bit are unsigned longs, so if bit is 31 we end up sign >> extending the 1 and mask ends up as 0x8000. Fix this >> by explicitly adding integer suffix UL ensure 1 is a unsigned long >> rather than an signed int. >> > > Right, you are, and the tool is ;-) > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > This is my careless mistake, thank you for finding it out and fix it! > >> Issue found with static analysis with CoverityScan, CID 1388564 >> >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() >> in force_qs_rnp()") >> Signed-off-by: Colin Ian King > > I think Paul only queued that for running tests and I have almost > finished a v2. I will fold your fix in my patch and add your SoB along > with mine, does that work for you? Sure, that's good with me. > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > please let me know ;-) > > Regards, > Boqun > >> --- >> kernel/rcu/tree.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >> index 10162ac..6ecedd8 100644 >> --- a/kernel/rcu/tree.c >> +++ b/kernel/rcu/tree.c >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, >> >> leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) >> if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) >> -mask |= 1 << bit; >> +mask |= 1UL << bit; >> >> if (mask != 0) { >> /* Idle/offline CPUs, report (releases rnp->lock. */ >> -- >> 2.10.2 >> signature.asc Description: OpenPGP digital signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > From: Colin Ian King> > mask and bit are unsigned longs, so if bit is 31 we end up sign > extending the 1 and mask ends up as 0x8000. Fix this > by explicitly adding integer suffix UL ensure 1 is a unsigned long > rather than an signed int. > Right, you are, and the tool is ;-) If @bit is greater than 32, we even got an undefined behavior in C ;-( This is my careless mistake, thank you for finding it out and fix it! > Issue found with static analysis with CoverityScan, CID 1388564 > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in > force_qs_rnp()") > Signed-off-by: Colin Ian King I think Paul only queued that for running tests and I have almost finished a v2. I will fold your fix in my patch and add your SoB along with mine, does that work for you? TBH, this situation is kinda new to me, so if anyone has any suggestion, please let me know ;-) Regards, Boqun > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 10162ac..6ecedd8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > - mask |= 1 << bit; > + mask |= 1UL << bit; > > if (mask != 0) { > /* Idle/offline CPUs, report (releases rnp->lock. */ > -- > 2.10.2 > signature.asc Description: PGP signature
Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
On Tue, Dec 13, 2016 at 10:56:46AM +, Colin King wrote: > From: Colin Ian King > > mask and bit are unsigned longs, so if bit is 31 we end up sign > extending the 1 and mask ends up as 0x8000. Fix this > by explicitly adding integer suffix UL ensure 1 is a unsigned long > rather than an signed int. > Right, you are, and the tool is ;-) If @bit is greater than 32, we even got an undefined behavior in C ;-( This is my careless mistake, thank you for finding it out and fix it! > Issue found with static analysis with CoverityScan, CID 1388564 > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in > force_qs_rnp()") > Signed-off-by: Colin Ian King I think Paul only queued that for running tests and I have almost finished a v2. I will fold your fix in my patch and add your SoB along with mine, does that work for you? TBH, this situation is kinda new to me, so if anyone has any suggestion, please let me know ;-) Regards, Boqun > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 10162ac..6ecedd8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > - mask |= 1 << bit; > + mask |= 1UL << bit; > > if (mask != 0) { > /* Idle/offline CPUs, report (releases rnp->lock. */ > -- > 2.10.2 > signature.asc Description: PGP signature
[PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
From: Colin Ian Kingmask and bit are unsigned longs, so if bit is 31 we end up sign extending the 1 and mask ends up as 0x8000. Fix this by explicitly adding integer suffix UL ensure 1 is a unsigned long rather than an signed int. Issue found with static analysis with CoverityScan, CID 1388564 Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") Signed-off-by: Colin Ian King --- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 10162ac..6ecedd8 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) - mask |= 1 << bit; + mask |= 1UL << bit; if (mask != 0) { /* Idle/offline CPUs, report (releases rnp->lock. */ -- 2.10.2
[PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error
From: Colin Ian King mask and bit are unsigned longs, so if bit is 31 we end up sign extending the 1 and mask ends up as 0x8000. Fix this by explicitly adding integer suffix UL ensure 1 is a unsigned long rather than an signed int. Issue found with static analysis with CoverityScan, CID 1388564 Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") Signed-off-by: Colin Ian King --- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 10162ac..6ecedd8 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) - mask |= 1 << bit; + mask |= 1UL << bit; if (mask != 0) { /* Idle/offline CPUs, report (releases rnp->lock. */ -- 2.10.2