Re: [PATCH v8 -tip 20/26] sched: Release references to the per-task cookie on exit

[Joel Fernandes <joel@joelfernandes.org>]

On Wed, Nov 04, 2020 at 04:50:42PM -0500, chris hyser wrote:
> On 10/19/20 9:43 PM, Joel Fernandes (Google) wrote:
> > During exit, we have to free the references to a cookie that might be shared by
> > many tasks. This commit therefore ensures when the task_struct is released, any
> > references to cookies that it holds are also released.
> > 
> > Tested-by: Julien Desfossez <jdesfossez@digitalocean.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >   include/linux/sched.h | 2 ++
> >   kernel/fork.c         | 1 +
> >   kernel/sched/core.c   | 8 ++++++++
> >   3 files changed, 11 insertions(+)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 4cb76575afa8..eabd96beff92 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -2079,12 +2079,14 @@ void sched_core_unsafe_exit(void);
> >   bool sched_core_wait_till_safe(unsigned long ti_check);
> >   bool sched_core_kernel_protected(void);
> >   int sched_core_share_pid(pid_t pid);
> > +void sched_tsk_free(struct task_struct *tsk);
> >   #else
> >   #define sched_core_unsafe_enter(ignore) do { } while (0)
> >   #define sched_core_unsafe_exit(ignore) do { } while (0)
> >   #define sched_core_wait_till_safe(ignore) do { } while (0)
> >   #define sched_core_kernel_protected(ignore) do { } while (0)
> >   #define sched_core_share_pid(pid_t pid) do { } while (0)
> > +#define sched_tsk_free(tsk) do { } while (0)
> >   #endif
> >   #endif
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index b9c289d0f4ef..a39248a02fdd 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -736,6 +736,7 @@ void __put_task_struct(struct task_struct *tsk)
> >   	exit_creds(tsk);
> >   	delayacct_tsk_free(tsk);
> >   	put_signal_struct(tsk->signal);
> > +	sched_tsk_free(tsk);
> >   	if (!profile_handoff_task(tsk))
> >   		free_task(tsk);
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 42aa811eab14..61e1dcf11000 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -9631,6 +9631,14 @@ static int cpu_core_tag_color_write_u64(struct cgroup_subsys_state *css,
> >   	return 0;
> >   }
> > +
> > +void sched_tsk_free(struct task_struct *tsk)
> > +{
> > +	if (!tsk->core_task_cookie)
> > +		return;
> > +	sched_core_put_task_cookie(tsk->core_task_cookie);
> > +	sched_core_put();
> 
> 
> sched_tsk_free() can be called under softirq. sched_core_put() is riddled with things that may want to sleep.

Right, that breaks. Can you try the diff I attached below and see if it does
not crash now?

> I tried a number of things like replacing the mutexes in put/get with spin_lock_bh() and just kept finding things like:
> 
> 
> [root@chyser-vm5 bin]# [ 1123.516209] BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49

Indeed, stop machine cannot be called from an atomic context, which is what
spin_lock_bh gives you.

---8<-----------------------

diff --git a/kernel/sched/coretag.c b/kernel/sched/coretag.c
index 6c008e5471d7..9967f37c5df0 100644
--- a/kernel/sched/coretag.c
+++ b/kernel/sched/coretag.c
@@ -16,6 +16,7 @@
  */
 struct sched_core_cookie {
 	refcount_t refcnt;
+	struct work_struct work; /* to free in WQ context. */;
 };
 
 static DEFINE_MUTEX(sched_core_tasks_mutex);
@@ -54,21 +55,24 @@ void sched_core_tag_requeue(struct task_struct *p, unsigned long cookie, bool gr
 }
 
 /* Per-task interface: Used by fork(2) and prctl(2). */
+static void sched_core_put_cookie_work(struct work_struct *ws);
+
 static unsigned long sched_core_alloc_task_cookie(void)
 {
-	struct sched_core_cookie *ptr =
+	struct sched_core_cookie *ck =
 		kmalloc(sizeof(struct sched_core_cookie), GFP_KERNEL);
 
-	if (!ptr)
+	if (!ck)
 		return 0;
-	refcount_set(&ptr->refcnt, 1);
+	refcount_set(&ck->refcnt, 1);
+	INIT_WORK(&ck->work, sched_core_put_cookie_work);
 
 	/*
 	 * NOTE: sched_core_put() is not done by put_task_cookie(). Instead, it
 	 * is done after the stopper runs.
 	 */
 	sched_core_get();
-	return (unsigned long)ptr;
+	return (unsigned long)ck;
 }
 
 static bool sched_core_get_task_cookie(unsigned long cookie)
@@ -91,6 +95,15 @@ static void sched_core_put_task_cookie(unsigned long cookie)
 		kfree(ptr);
 }
 
+static void sched_core_put_cookie_work(struct work_struct *ws)
+{
+	struct sched_core_cookie *ck =
+		container_of(ws, struct sched_core_cookie, work);
+
+	sched_core_put_task_cookie((unsigned long)ck);
+	sched_core_put();
+}
+
 struct sched_core_task_write_tag {
 	struct task_struct *tasks[2];
 	unsigned long cookies[2];
@@ -461,8 +474,11 @@ int cpu_core_tag_color_write_u64(struct cgroup_subsys_state *css,
 
 void sched_tsk_free(struct task_struct *tsk)
 {
+	struct sched_core_cookie *ck;
+
 	if (!tsk->core_task_cookie)
 		return;
-	sched_core_put_task_cookie(tsk->core_task_cookie);
-	sched_core_put();
+
+	ck = (struct sched_core_cookie *)tsk->core_task_cookie;
+	queue_work(system_wq, &ck->work);
 }