Re: [PATCH] sched/mmcid: Bound the CID allocation busy wait

On Fri, Sep 18, 2026 at 01:34:54AM +0000, Jiakai Xu wrote:
> mm_get_cid() spins forever when no CID is available. All callers hold
> either a runqueue lock or mm::mm_cid::lock with interrupts disabled,
> so the loop relies on another CPU releasing a CID within a short
> window.
> 
> That assumption fails in two ways:
> 
>  1) In steady state per task mode CIDs are owned by their tasks for
>     their whole lifetime and are only released on task exit or
>     execve(). When the CID bitmap is exhausted, the spinning task
>     blocks everything on its CPU with interrupts disabled, which
>     escalates to RCU stalls and can lock up the machine when e.g. a
>     text_poke IPI targets the spinning CPU.
> 
>  2) During a mode transition the fixup thread has to acquire the
>     runqueue lock of the spinning task's CPU to release per CPU owned
>     CIDs. That lock is held by the spinning task, so neither context
>     can make progress - a livelock.
> 
> Bound the retry loop and return MM_CID_UNSET on exhaustion. All call
> sites cope with that:
> 
>  - The schedule in paths (mm_cid_from_task()/mm_cid_from_cpu()) set
>    both the per CPU and the task storage to MM_CID_UNSET and retry on
>    the next schedule in. The plain per CPU value left behind by
>    mm_drop_cid_on_cpu() has no owner in the bitmap anymore, so the
>    task must not adopt it as its own CID. A task running with an
>    unset CID is an already established state for lazily assigned
>    tasks (see mm_cid_fixup_cpus_to_tasks()).
> 
>  - sched_mm_cid_fork() stores the unset CID in the task and the per
>    CPU storage, which the schedule in path handles the same way.
> 
> This also prevents an exhausted allocation from feeding MM_CID_UNSET
> into the transition bit handling, which would later hand MM_CID_UNSET
> as bit number to clear_bit().

This all sounds horribly wrong. It fails to explain why the transition
isn't happening, nor does it explain how it doesn't utterly
violate/break user space.

> Fixes: 9a723ed7facff ("sched/mmcid: Provide new scheduler CID mechanism")
> Signed-off-by: Jiakai Xu <[email protected]>
> ---
>  kernel/sched/sched.h | 51 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e656c7059bf86..6de25f546e3a6 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3964,11 +3964,27 @@ static inline unsigned int __mm_get_cid(struct mm_struct *mm, unsigned int max_c
>  	return cid;
>  }
>  
> +/*
> + * The retry loop covers the transient contention window where a CID is
> + * concurrently released. It must be bound because all callers hold a
> + * runqueue lock or mm::mm_cid::lock with interrupts disabled. An
> + * unbounded wait livelocks with the context which is expected to
> + * release a CID: in steady state per task mode CIDs are owned by their
> + * tasks until exit and during a mode transition the fixup thread needs
> + * the runqueue lock which the spinning task holds.
> + *
> + * On exhaustion MM_CID_UNSET is returned, which all callers handle by
> + * letting the task run without a CID. It retries on the next schedule
> + * in or fork.
> + */
> +#define MM_CID_GET_RETRIES	32
> +
>  static inline unsigned int mm_get_cid(struct mm_struct *mm)
>  {
>  	unsigned int cid = __mm_get_cid(mm, READ_ONCE(mm->mm_cid.max_cids));
> +	unsigned int tries = MM_CID_GET_RETRIES;
>  
> -	while (cid == MM_CID_UNSET) {
> +	while (cid == MM_CID_UNSET && tries--) {
>  		cpu_relax();
>  		cid = __mm_get_cid(mm, num_possible_cpus());
>  	}
> @@ -4030,9 +4046,22 @@ static __always_inline void mm_cid_from_cpu(struct task_struct *t, unsigned int
>  			else
>  				cpu_cid = cid_to_cpu_cid(tcid);
>  		}
> -		/* Still nothing, allocate a new one */
> -		if (!cid_on_cpu(cpu_cid))
> -			cpu_cid = cid_to_cpu_cid(mm_get_cid(mm));
> +		/* Still nothing, allocate a new one. On pool exhaustion
> +		 * set both storages to MM_CID_UNSET: the plain per CPU
> +		 * value left by mm_drop_cid_on_cpu() no longer has an
> +		 * owner in the bitmap and must not be adopted by the
> +		 * task. It will be retried on the next schedule in.
> +		 */

This comment style is broken and inconsistent with your earlier comment.

> +		if (!cid_on_cpu(cpu_cid)) {
> +			unsigned int ncid = mm_get_cid(mm);
> +
> +			if (ncid == MM_CID_UNSET) {
> +				mm_cid_update_pcpu_cid(mm, MM_CID_UNSET);
> +				mm_cid_update_task_cid(t, MM_CID_UNSET);
> +				return;
> +			}
> +			cpu_cid = cid_to_cpu_cid(ncid);
> +		}
>  
>  		/* Handle the transition mode flag if required */
>  		if (mode & MM_CID_TRANSIT)
> @@ -4065,9 +4094,19 @@ static __always_inline void mm_cid_from_task(struct task_struct *t, unsigned int
>  			else
>  				tcid = cpu_cid_to_cid(cpu_cid);
>  		}
> -		/* Still nothing, allocate a new one */
> -		if (!cid_on_task(tcid))
> +		/* Still nothing, allocate a new one. On pool exhaustion
> +		 * keep the CID unset. It will be retried on the next
> +		 * schedule in.
> +		 */

Again, broken comment style.

> +		if (!cid_on_task(tcid)) {
>  			tcid = mm_get_cid(mm);
> +
> +			if (tcid == MM_CID_UNSET) {
> +				mm_cid_update_pcpu_cid(mm, tcid);
> +				mm_cid_update_task_cid(t, tcid);
> +				return;
> +			}
> +		}
>  		/* Set the transition mode flag if required */
>  		tcid |= mode & MM_CID_TRANSIT;
>  	}