Re: [PATCH v6 2/5] sched: Use user_cpus_ptr for saving user provided cpumask in sched_setaffinity()

[Waiman Long]

On 8/31/22 05:12, Peter Zijlstra wrote:
> On Thu, Aug 25, 2022 at 09:01:16PM -0400, Waiman Long wrote:
>
>
> >   void relax_compatible_cpus_allowed_ptr(struct task_struct *p)
> >   {
> > -	struct cpumask *user_mask = p->user_cpus_ptr;
> > -	unsigned long flags;
> > -
> >   	/*
> > -	 * Try to restore the old affinity mask. If this fails, then
> > -	 * we free the mask explicitly to avoid it being inherited across
> > -	 * a subsequent fork().
> > +	 * Try to restore the old affinity mask with __sched_setaffinity().
> > +	 * Cpuset masking will be done there too.
> >   	 */
> > -	if (!user_mask || !__sched_setaffinity(p, user_mask))
> > -		return;
> > -
> > -	raw_spin_lock_irqsave(&p->pi_lock, flags);
> > -	user_mask = clear_user_cpus_ptr(p);
> > -	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> > -
> > -	kfree(user_mask);
> > +	__sched_setaffinity(p, task_user_cpus(p), false);
> >   }
> We have an issue with __sched_setaffinity() failing here. I'm not sure
> ignoring the failure is the right thing -- but I'm also not enturely
> sure what is.
I am not sure what we can do in case __sched_setaffinity() fails. Maybe 
we can print a warning when this happen. What do you think?
> >   void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
> > @@ -8081,10 +8046,11 @@ int dl_task_check_affinity(struct task_struct *p, const struct cpumask *mask)
> >   #endif
> >   
> >   static int
> > -__sched_setaffinity(struct task_struct *p, const struct cpumask *mask)
> > +__sched_setaffinity(struct task_struct *p, const struct cpumask *mask, bool save_mask)
> >   {
> >   	int retval;
> >   	cpumask_var_t cpus_allowed, new_mask;
> > +	struct cpumask *user_mask = NULL;
> >   
> >   	if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL))
> >   		return -ENOMEM;
> > @@ -8100,8 +8066,22 @@ __sched_setaffinity(struct task_struct *p, const struct cpumask *mask)
> >   	retval = dl_task_check_affinity(p, new_mask);
> >   	if (retval)
> >   		goto out_free_new_mask;
> > +
> > +	/*
> > +	 * Save the user requested mask internally now and then update
> > +	 * user_cpus_ptr later after making sure this call will be
> > +	 * successful, i.e. retval == 0.
> > +	 */
> > +	if (save_mask) {
> > +		user_mask = kmalloc(cpumask_size(), GFP_KERNEL);
> > +		if (!user_mask) {
> > +			retval = -ENOMEM;
> > +			goto out_free_new_mask;
> > +		}
> > +		cpumask_copy(user_mask, mask);
> > +	}
> >   again:
> > -	retval = __set_cpus_allowed_ptr(p, new_mask, SCA_CHECK | SCA_USER);
> > +	retval = __set_cpus_allowed_ptr(p, new_mask, SCA_CHECK);
> >   	if (retval)
> >   		goto out_free_new_mask;
> >   
> > @@ -8115,7 +8095,16 @@ __sched_setaffinity(struct task_struct *p, const struct cpumask *mask)
> >   		goto again;
> >   	}
> >   
> > +	if (save_mask) {
> > +		unsigned long flags;
> > +
> > +		/* Use pi_lock to synchronize changes to user_cpus_ptr */
> > +		raw_spin_lock_irqsave(&p->pi_lock, flags);
> > +		swap(p->user_cpus_ptr, user_mask);
> > +		raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> > +	}
> >   out_free_new_mask:
> > +	kfree(user_mask);
> >   	free_cpumask_var(new_mask);
> >   out_free_cpus_allowed:
> >   	free_cpumask_var(cpus_allowed);
> I'm confused as to why it's put in this function and not in the one
> caller that actually sets the new @save_mask true, here:

Looking at this patch alone, we can certainly put mask saving in 
sched_setaffinity(). In later patches, however, I have to make 
user_cpus_ptr update in the same lock critical section as cpus_mask. 
That is the reason why it is done this way here. I can certainly make 
your suggested change in this patch and then move the saving inside in a 
later patch.

Cheers,
Longman