Re: [PATCH v4 10/17] preempt: Introduce HAS_SEPARATE_PREEMPT_RESCHED_BITS

[Shrikanth Hegde <sshegde@linux.ibm.com>]

Hi.

> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 33fc4c814a9f..8299657f0f86 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -34,14 +34,31 @@
>    *         SOFTIRQ_MASK:	0x0000ff00
>    * HARDIRQ_DISABLE_MASK:	0x00ff0000
>    *         HARDIRQ_MASK:	0x0f000000
> + *
> + * When HAS_SEPARATE_PREEMPT_RESCHED_BITS=y, PREEMPT_NEED_RESCHED is put in a
> + * separate word and that allows 64bit load-store architectures to 'set'
> + * PREEMPT_NEED_RESCHED without messing up the otherwise symmetric
> + * modifications used on preempt_count and still load the whole thing
> + * (single-copy) atomically, without having to resort to full atomic
> + * operations.
> + *
> + * Because of the above, NMI_MASK bits are different depending on
> + * HAS_SEPARATE_PREEMPT_RESCHED_BITS:
> + *
> + * - HAS_SEPARATE_PREEMPT_RESCHED_BITS=n:
> + *
>    *             NMI_MASK:	0x10000000
>    * PREEMPT_NEED_RESCHED:	0x80000000
> + *
> + * - HAS_SEPARATE_PREEMPT_RESCHED_BITS=y:
> + *             NMI_MASK:	0xf0000000
> + * (PREEMPT_NEED_RESCHED is in a different word)
>    */
>   #define PREEMPT_BITS	8
>   #define SOFTIRQ_BITS	8
>   #define HARDIRQ_DISABLE_BITS	8
>   #define HARDIRQ_BITS	4
> -#define NMI_BITS	1
> +#define NMI_BITS	(1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
>  


Shouldn't testing/selftests/bpf/bpf_experimental.h also be updated with 
same?


>   #define PREEMPT_SHIFT	0
>   #define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
> @@ -116,8 +133,8 @@ static __always_inline unsigned char interrupt_context_level(void)
>    * preempt_count() is commonly implemented with READ_ONCE().
>    */
>   
> -#define nmi_count()	(preempt_count() & NMI_MASK)
> -#define hardirq_count()	(preempt_count() & HARDIRQ_MASK)
> +#define nmi_count()		(preempt_count() & NMI_MASK)
> +#define hardirq_count()		(preempt_count() & HARDIRQ_MASK)
>   #ifdef CONFIG_PREEMPT_RT
>   # define softirq_count()	(current->softirq_disable_cnt & SOFTIRQ_MASK)
>   # define irq_count()		((preempt_count() & (NMI_MASK | HARDIRQ_MASK)) | softirq_count())
> diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
> index 88c594c6d7fc..35f546a042b1 100644
> --- a/kernel/Kconfig.preempt
> +++ b/kernel/Kconfig.preempt
> @@ -122,6 +122,10 @@ config PREEMPT_RT_NEEDS_BH_LOCK
>   config PREEMPT_COUNT
>          bool
>   
> +config HAS_SEPARATE_PREEMPT_RESCHED_BITS
> +	bool
> +	depends on PREEMPT_COUNT && 64BIT
> +
>   config PREEMPTION
>          bool
>          select PREEMPT_COUNT
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9b3f1764fa9e..6d88343c3bad 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5973,8 +5973,13 @@ void preempt_count_add(int val)
>   #ifdef CONFIG_DEBUG_PREEMPT
>   	/*
>   	 * Underflow?
> +	 *
> +	 * Cannot detect underflow based on the current preempt_count() value
> +	 * if using HAS_SEPARATE_PREEMPT_RESCHED_BITS because preempt count takes all 32
> +	 * bits.
>   	 */
> -	if (DEBUG_LOCKS_WARN_ON((preempt_count() < 0)))
> +	if (!IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS) &&
> +	    DEBUG_LOCKS_WARN_ON((preempt_count() < 0)))
>   		return;
>   #endif
>   	__preempt_count_add(val);
> @@ -6006,7 +6011,10 @@ void preempt_count_sub(int val)
>   	/*
>   	 * Underflow?
>   	 */
> -	if (DEBUG_LOCKS_WARN_ON(val > preempt_count()))
> +	unsigned int uval = val;
> +	unsigned int pc = preempt_count();
> +
> +	if (DEBUG_LOCKS_WARN_ON(pc - uval > pc))
>   		return;
>   	/*
>   	 * Is the spinlock portion underflowing?
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 0c9b2269a8d6..7980a4a232f9 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -103,7 +103,13 @@ void _local_interrupt_enable(void)
>   }
>   EXPORT_SYMBOL(_local_interrupt_enable);
>   
> +#ifndef CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS
> +/*
> + * Any 32bit architecture that still cares about performance should
> + * probably ensure this is near preempt_count.
> + */
>   DEFINE_PER_CPU(unsigned int, nmi_nesting);
> +#endif
>   
>   /*
>    * SOFTIRQ_OFFSET usage:
> diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
> index bfafe1204c7b..c3d976c801bb 100644
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -1429,7 +1429,7 @@ static int unexpected_testcase_failures;
>   
>   static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
>   {
> -	int saved_preempt_count = preempt_count();
> +	long saved_preempt_count = preempt_count();

nit: Isn't preempt_count still returns int?

>   #ifdef CONFIG_PREEMPT_RT
>   	int saved_mgd_count = current->migration_disabled;
>   	int saved_rcu_count = current->rcu_read_lock_nesting;