From: Xuewen Yan <xuewen.yan94@gmail.com>
To: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Xuewen Yan <xuewen.yan@unisoc.com>,
akpm@linux-foundation.org, oleg@redhat.com,
dylanbhatch@google.com, rick.p.edgecombe@intel.com,
ke.wang@unisoc.com, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] sched/proc: Print user_cpus_ptr for task status
Date: Tue, 7 May 2024 14:57:35 +0800 [thread overview]
Message-ID: <CAB8ipk-yz+6X2E7BsJmNqVgZDjE8NkJFNdqFU+WLieKVhFaCuA@mail.gmail.com> (raw)
In-Reply-To: <e402d623-1875-47a2-9db3-8299a54502ef@redhat.com>
Hi Waiman
On Tue, May 7, 2024 at 2:04 AM Waiman Long <longman@redhat.com> wrote:
>
> On 5/6/24 04:04, Xuewen Yan wrote:
> > Hi Peter
> >
> > On Mon, Apr 29, 2024 at 8:10 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >> On Mon, Apr 29, 2024 at 04:46:33PM +0800, Xuewen Yan wrote:
> >>> The commit 851a723e45d1c("sched: Always clear user_cpus_ptr in do_set_cpus_allowed()")
> >>> would clear the user_cpus_ptr when call the do_set_cpus_allowed.
> >>>
> >>> In order to determine whether the user_cpus_ptr is taking effect,
> >>> it is better to print the task's user_cpus_ptr.
> >> This is an ABI change and would mandate we forever more have this
> >> distinction. I don't think your changes justifies things sufficiently
> >> for this.
> > I added this mainly because online/offline cpu will produce different
> > results for the !top-cpuset task.
> >
> > For example:
> >
> > If the task was running, then offline task's cpus, would lead to clear
> > its user-mask.
> >
> > unisoc:/ # while true; do sleep 600; done&
> > [1] 6786
> > unisoc:/ # echo 6786 > /dev/cpuset/top-app/tasks
> > unisoc:/ # cat /dev/cpuset/top-app/cpus
> > 0-7
> > unisoc:/ # cat /proc/6786/status | grep Cpus
> > Cpus_allowed: ff
> > Cpus_allowed_list: 0-7
> > Cpus_user_allowed: (null)
> > Cpus_user_allowed_list: (null)
> >
> > unisoc:/ # taskset -p c0 6786
> > pid 6786's current affinity mask: ff
> > pid 6786's new affinity mask: c0
> > unisoc:/ # cat /proc/6786/status | grep Cpus
> > Cpus_allowed: c0
> > Cpus_allowed_list: 6-7
> > Cpus_user_allowed: c0
> > Cpus_user_allowed_list: 6-7
> >
> > After offline the cpu6 and cpu7, the user-mask would be cleared:
> >
> > unisoc:/ # echo 0 > /sys/devices/system/cpu/cpu7/online
> > unisoc:/ # cat /proc/6786/status | grep Cpus
> > Cpus_allowed: 40
> > Cpus_allowed_list: 6
> > Cpus_user_allowed: c0
> > Cpus_user_allowed_list: 6-7
> > ums9621_1h10:/ # echo 0 > /sys/devices/system/cpu/cpu6/online
> > ums9621_1h10:/ # cat /proc/6786/status | grep Cpus
> > Cpus_allowed: 3f
> > Cpus_allowed_list: 0-5
> > Cpus_user_allowed: (null)
> > Cpus_user_allowed_list: (null)
> >
> > When online the cpu6/7, the user-mask can not bring back:
> >
> > unisoc:/ # echo 1 > /sys/devices/system/cpu/cpu6/online
> > unisoc:/ # cat /proc/6786/status | grep Cpus
> > Cpus_allowed: 7f
> > Cpus_allowed_list: 0-6
> > Cpus_user_allowed: (null)
> > Cpus_user_allowed_list: (null)
> > unisoc:/ # echo 1 > /sys/devices/system/cpu/cpu7/online
> > unisoc:/ # cat /proc/6786/status | grep Cpus
> > Cpus_allowed: ff
> > Cpus_allowed_list: 0-7
> > Cpus_user_allowed: (null)
> > Cpus_user_allowed_list: (null)
> >
> > However, if we offline the cpu when the task is sleeping, at this
> > time, because would not call the fallback_cpu(), its user-mask will
> > not be cleared.
> >
> > unisoc:/ # while true; do sleep 600; done&
> > [1] 5990
> > unisoc:/ # echo 5990 > /dev/cpuset/top-app/tasks
> > unisoc:/ # cat /proc/5990/status | grep Cpus
> > Cpus_allowed: ff
> > Cpus_allowed_list: 0-7
> > Cpus_user_allowed: (null)
> > Cpus_user_allowed_list: (null)
> >
> > unisoc:/ # taskset -p c0 5990
> > pid 5990's current affinity mask: ff
> > pid 5990's new affinity mask: c0
> > unisoc:/ # cat /proc/5990/status | grep Cpus
> > Cpus_allowed: c0
> > Cpus_allowed_list: 6-7
> > Cpus_user_allowed: c0
> > Cpus_user_allowed_list: 6-7
> >
> > unisoc:/ # echo 0 > /sys/devices/system/cpu/cpu6/online
> > unisoc:/ # cat /proc/5990/status | grep Cpus
> > Cpus_allowed: 80
> > Cpus_allowed_list: 7
> > Cpus_user_allowed: c0
> > Cpus_user_allowed_list: 6-7
> > unisoc:/ # echo 0 > /sys/devices/system/cpu/cpu7/online
> > unisoc:/ # cat /proc/5990/status | grep Cpus
> > Cpus_allowed: 3f
> > Cpus_allowed_list: 0-5
> > Cpus_user_allowed: c0
> > Cpus_user_allowed_list: 6-7
> >
> >
> > After 10 minutes, it was waked up, it can also keep its user-mask:
> > ums9621_1h10:/ # cat /proc/5990/status | grep Cpus
> > Cpus_allowed: 3f
> > Cpus_allowed_list: 0-5
> > Cpus_user_allowed: c0
> > Cpus_user_allowed_list: 6-7
> >
> > In order to solve the above problem, I modified the following patch.
> > At this time, for !top-cpuset, regardless of whether the task is in
> > the running state when offline cpu, its cpu-mask can be maintained.
> > However, this patch may not be perfect yet, so I send the "Print
> > user_cpus_ptr for task status" patch first to debug more conveniently.
> >
> > --->
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 68cfa656b9b1..00879b6de8d4 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1870,7 +1870,7 @@ extern void dl_bw_free(int cpu, u64 dl_bw);
> > #ifdef CONFIG_SMP
> >
> > /* do_set_cpus_allowed() - consider using set_cpus_allowed_ptr() instead */
> > -extern void do_set_cpus_allowed(struct task_struct *p, const struct
> > cpumask *new_mask);
> > +extern void do_set_cpus_allowed(struct task_struct *p, const struct
> > cpumask *new_mask, bool keep_user);
> >
> > /**
> > * set_cpus_allowed_ptr - set CPU affinity mask of a task
> > @@ -1886,7 +1886,7 @@ extern int dl_task_check_affinity(struct
> > task_struct *p, const struct cpumask *m
> > extern void force_compatible_cpus_allowed_ptr(struct task_struct *p);
> > extern void relax_compatible_cpus_allowed_ptr(struct task_struct *p);
> > #else
> > -static inline void do_set_cpus_allowed(struct task_struct *p, const
> > struct cpumask *new_mask)
> > +static inline void do_set_cpus_allowed(struct task_struct *p, const
> > struct cpumask *new_mask, bool keep_user)
> > {
> > }
> > static inline int set_cpus_allowed_ptr(struct task_struct *p, const
> > struct cpumask *new_mask)
> > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> > index 7ee9994aee40..0c448f8a3829 100644
> > --- a/kernel/cgroup/cpuset.c
> > +++ b/kernel/cgroup/cpuset.c
> > @@ -4005,9 +4005,14 @@ bool cpuset_cpus_allowed_fallback(struct
> > task_struct *tsk)
> >
> > rcu_read_lock();
> > cs_mask = task_cs(tsk)->cpus_allowed;
> > - if (is_in_v2_mode() && cpumask_subset(cs_mask, possible_mask)) {
> > - do_set_cpus_allowed(tsk, cs_mask);
> > - changed = true;
> > + if (cpumask_subset(cs_mask, possible_mask)) {
> > + if (is_in_v2_mode()) {
> > + do_set_cpus_allowed(tsk, cs_mask, false);
> > + changed = true;
> > + } else if (task_cs(tsk) != &top_cpuset) {
> > + do_set_cpus_allowed(tsk, cs_mask, true);
> > + changed = true;
> > + }
> > }
> > rcu_read_unlock();
> >
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 7a7aa5f93c0c..7ede27630088 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -527,7 +527,7 @@ static void __kthread_bind_mask(struct task_struct
> > *p, const struct cpumask *mas
> >
> > /* It's safe because the task is inactive. */
> > raw_spin_lock_irqsave(&p->pi_lock, flags);
> > - do_set_cpus_allowed(p, mask);
> > + do_set_cpus_allowed(p, mask, false);
> > p->flags |= PF_NO_SETAFFINITY;
> > raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> > }
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 33cfd522fc7c..623f89e65e6c 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -2855,18 +2855,21 @@ __do_set_cpus_allowed(struct task_struct *p,
> > struct affinity_context *ctx)
> > * Used for kthread_bind() and select_fallback_rq(), in both cases the user
> > * affinity (if any) should be destroyed too.
> > */
> > -void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
> > +void do_set_cpus_allowed(struct task_struct *p, const struct cpumask
> > *new_mask, bool keep_user)
> > {
> > struct affinity_context ac = {
> > .new_mask = new_mask,
> > .user_mask = NULL,
> > - .flags = SCA_USER, /* clear the user requested mask */
> > + .flags = 0, /* clear the user requested mask */
> > };
> > union cpumask_rcuhead {
> > cpumask_t cpumask;
> > struct rcu_head rcu;
> > };
> >
> > + if (!keep_user)
> > + ac.flags = SCA_USER;
> > +
> > __do_set_cpus_allowed(p, &ac);
> >
> > /*
> > @@ -2874,7 +2877,8 @@ void do_set_cpus_allowed(struct task_struct *p,
> > const struct cpumask *new_mask)
> > * to use kfree() here (when PREEMPT_RT=y), therefore punt to using
> > * kfree_rcu().
> > */
> > - kfree_rcu((union cpumask_rcuhead *)ac.user_mask, rcu);
> > + if (!keep_user)
> > + kfree_rcu((union cpumask_rcuhead *)ac.user_mask, rcu);
> > }
> >
> > static cpumask_t *alloc_user_cpus_ptr(int node)
> > @@ -3664,7 +3668,7 @@ int select_fallback_rq(int cpu, struct task_struct *p)
> > *
> > * More yuck to audit.
> > */
> > - do_set_cpus_allowed(p, task_cpu_possible_mask(p));
> > + do_set_cpus_allowed(p,
> > task_cpu_possible_mask(p), false);
> > state = fail;
> > break;
> > case fail:
> >
> These changes essentially reverts commit 851a723e45d1c("sched: Always
> clear user_cpus_ptr in do_set_cpus_allowed()") except the additional
> caller in the cpuset code.
>
> How about the following less invasive change?
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 7019a40457a6..646837eab70c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2796,21 +2796,24 @@ __do_set_cpus_allowed(struct task_struct *p,
> struct affinity_context *ctx)
> }
>
> /*
> - * Used for kthread_bind() and select_fallback_rq(), in both cases the user
> - * affinity (if any) should be destroyed too.
> + * Used for kthread_bind() and select_fallback_rq(). Destroy user affinity
> + * if no intersection with the new mask.
> */
> void do_set_cpus_allowed(struct task_struct *p, const struct cpumask
> *new_mask)
> {
> struct affinity_context ac = {
> .new_mask = new_mask,
> .user_mask = NULL,
> - .flags = SCA_USER, /* clear the user requested mask */
> + .flags = 0,
> };
> union cpumask_rcuhead {
> cpumask_t cpumask;
> struct rcu_head rcu;
> };
>
> + if (current->user_cpus_ptr &&
> !cpumask_intersects(current->user_cpus_ptr, new_mask))
Thanks for your suggestion, and I try it and as for me, it works well,
but I change the "current" to p.
I think “current” is inappropriate because what is changed here is the
mask of p.
It is possible that “p” and “current” are not equal.
I would send the next patch later and add your Suggested-by. Thanks
again for your advice!
BR
---
xuewen
> + ac.flags = SCA_USER; /* clear the user requested mask */
> +
> __do_set_cpus_allowed(p, &ac);
>
> /*
>
> No compilation test done. Note that there is a null check inside
> kfree_rcu() with no need for additional check.
>
> Regards,
> Longman
>
>
next prev parent reply other threads:[~2024-05-07 6:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 8:46 [PATCH] sched/proc: Print user_cpus_ptr for task status Xuewen Yan
2024-04-29 12:10 ` Peter Zijlstra
2024-05-06 8:04 ` Xuewen Yan
2024-05-06 18:03 ` Waiman Long
2024-05-07 6:57 ` Xuewen Yan [this message]
2024-05-08 17:01 ` Waiman Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAB8ipk-yz+6X2E7BsJmNqVgZDjE8NkJFNdqFU+WLieKVhFaCuA@mail.gmail.com \
--to=xuewen.yan94@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=dylanbhatch@google.com \
--cc=ke.wang@unisoc.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=longman@redhat.com \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=rick.p.edgecombe@intel.com \
--cc=xuewen.yan@unisoc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.