Re: [PATCH] cgroup: Disable preemption for cgrp->freezer.freeze_seq when CONFIG_PREEMPT_RT=y.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-10-02 09:22:25 [-0700], Kuniyuki Iwashima wrote:
> On Thu, Oct 2, 2025 at 1:28 AM Michal Koutný <mkoutny@suse.com> wrote:
> >
> > Hello.
> >
> > Thanks for looking into this Kuniyuki.
> >
> > On Thu, Oct 02, 2025 at 05:22:07AM +0000, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > The writer side is under spin_lock_irq(), but the section is still
> > > preemptible with CONFIG_PREEMPT_RT=y.
> >
> > I see similar construction in other places, e.g.
> >         mems_allowed_seq in set_mems_allowed
> 
> IIUC, local_irq_save() or local_irq_disable() is used
> for the writer of mems_allowed_seq, so there should
> be not preemptible.

That local_irq_disable() looks odd.
mems_allowed_seq is different, it is associated with
task_struct::alloc_lock. This lock is acquired by set_mems_allowed() so
it is enough. That local_irq_disable() is there because seqcount
read side can be used from softirq.

> 
> >         period_seqcount in ioc_start_period
> >         pidmap_lock_seq in alloc_pid/pidfs_add_pid
> 
> These two seem to have the same problem.

Nope. period_seqcount is a seqcount_spinlock_t. So is pidmap_lock_seq.

> > (where their outer lock becomes preemptible on PREEMPT_RT.)
> >
> > > Let's wrap the section with preempt_{disable,enable}_nested().
> >
> > Is it better to wrap them all (for CONFIG_PREEMPT_RT=y) or should they
> > become seqlock_t on CONFIG_PREEMPT_RT=y?
> 
> I think wrapping them would be better as the wrapper is just
> an lockdep assertion when PREEMPT_RT=n

Now that I swap in in everything.

If you have a "naked" seqcount_t then you need manually ensure that
there can be only one writer. And then need to disable preemption on top
of it in order to ensure that the writer makes always progress.

In the freezer case, may I suggest the following instead:

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 539c64eeef38f..933c4487a8462 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -435,7 +435,7 @@ struct cgroup_freezer_state {
 	int nr_frozen_tasks;
 
 	/* Freeze time data consistency protection */
-	seqcount_t freeze_seq;
+	seqcount_spinlock_t freeze_seq;
 
 	/*
 	 * Most recent time the cgroup was requested to freeze.
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index eb9fc7ae65b08..c0215e7de3666 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5813,7 +5813,7 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
 	 * if the parent has to be frozen, the child has too.
 	 */
 	cgrp->freezer.e_freeze = parent->freezer.e_freeze;
-	seqcount_init(&cgrp->freezer.freeze_seq);
+	seqcount_spinlock_init(&cgrp->freezer.freeze_seq, &css_set_lock);
 	if (cgrp->freezer.e_freeze) {
 		/*
 		 * Set the CGRP_FREEZE flag, so when a process will be

While former works, too this is way nicer. Not sure if it compiles but
it should.

> Thanks!

Sebastian