Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Peter Zijlstra]

On Mon, Sep 26, 2022 at 12:55:21PM +0200, Christian Borntraeger wrote:
> 
> 
> Am 26.09.22 um 10:06 schrieb Christian Borntraeger:
> > 
> > 
> > Am 23.09.22 um 09:53 schrieb Christian Borntraeger:
> > > Am 23.09.22 um 09:21 schrieb Christian Borntraeger:
> > > > Peter,
> > > > 
> > > > as a heads-up. This commit (bisected and verified) triggers a
> > > > regression in our KVM on s390x CI. The symptom is that a specific
> > > > testcase (start a guest with next kernel and a poky ramdisk,
> > > > then ssh via vsock into the guest and run the reboot command) now
> > > > takes much longer (300 instead of 20 seconds). From a first look
> > > > it seems that the sshd takes very long to end during shutdown
> > > > but I have not looked into that yet.
> > > > Any quick idea?
> > > > 
> > > > Christian
> > > 
> > > the sshd seems to hang in virtio-serial (not vsock).
> > 
> > FWIW, sshd does not seem to hang, instead it seems to busy loop in
> > wait_port_writable calling into the scheduler over and over again.
> 
> -#define TASK_FREEZABLE                 0x00002000
> +#define TASK_FREEZABLE                 0x00000000
> 
> "Fixes" the issue. Just have to find out which of users is responsible.

Since it's not the wait_port_writable() one -- we already tested that by
virtue of 's/wait_event_freezable/wait_event/' there, it must be on the
producing side of that port. But I'm having a wee bit of trouble
following that code.

Is there a task stuck in FROZEN state? -- then again, I thought you said
there was no actual suspend involved, so that should not be it either.

I'm curious though -- how far does it get into the scheduler? It should
call schedule() with __state == TASK_INTERRUPTIBLE|TASK_FREEZABLE, which
is quite sufficient to get it off the runqueue, who then puts it back?
Or is it bailing early in the wait_event loop?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Christian Borntraeger]

Am 26.09.22 um 12:55 schrieb Christian Borntraeger:
> 
> 
> Am 26.09.22 um 10:06 schrieb Christian Borntraeger:
>>
>>
>> Am 23.09.22 um 09:53 schrieb Christian Borntraeger:
>>> Am 23.09.22 um 09:21 schrieb Christian Borntraeger:
>>>> Peter,
>>>>
>>>> as a heads-up. This commit (bisected and verified) triggers a
>>>> regression in our KVM on s390x CI. The symptom is that a specific
>>>> testcase (start a guest with next kernel and a poky ramdisk,
>>>> then ssh via vsock into the guest and run the reboot command) now
>>>> takes much longer (300 instead of 20 seconds). From a first look
>>>> it seems that the sshd takes very long to end during shutdown
>>>> but I have not looked into that yet.
>>>> Any quick idea?
>>>>
>>>> Christian
>>>
>>> the sshd seems to hang in virtio-serial (not vsock).
>>
>> FWIW, sshd does not seem to hang, instead it seems to busy loop in
>> wait_port_writable calling into the scheduler over and over again.
> 
> -#define TASK_FREEZABLE                 0x00002000
> +#define TASK_FREEZABLE                 0x00000000
> 
> "Fixes" the issue. Just have to find out which of users is responsible.

So it seems that my initial test was not good enough.

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9fa3c76a267f..e93df4f735fe 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -790,7 +790,7 @@ static int wait_port_writable(struct port *port, bool nonblock)
                 if (nonblock)
                         return -EAGAIN;
  
-               ret = wait_event_freezable(port->waitqueue,
+               ret = wait_event_interruptible(port->waitqueue,
                                            !will_write_block(port));
                 if (ret < 0)
                         return ret;

Does fix the problem.
My initial test was the following

--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -790,10 +790,8 @@ static int wait_port_writable(struct port *port, bool nonblock)
                 if (nonblock)
                         return -EAGAIN;
  
-               ret = wait_event_freezable(port->waitqueue,
+               wait_event(port->waitqueue,
                                            !will_write_block(port));
-               if (ret < 0)
-                       return ret;
         }
         /* Port got hot-unplugged. */
         if (!port->guest_connected)

and obviously it did not provide an exit path.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Peter Zijlstra]

On Mon, Sep 26, 2022 at 02:32:24PM +0200, Christian Borntraeger wrote:
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 9fa3c76a267f..e93df4f735fe 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -790,7 +790,7 @@ static int wait_port_writable(struct port *port, bool nonblock)
>                 if (nonblock)
>                         return -EAGAIN;
> -               ret = wait_event_freezable(port->waitqueue,
> +               ret = wait_event_interruptible(port->waitqueue,
>                                            !will_write_block(port));
>                 if (ret < 0)
>                         return ret;
> 
> Does fix the problem.

It's almost as if someone does try_to_wake_up(.state = TASK_FREEZABLE)
-- which would be quite insane.

Could you please test with something like the below on? I can boot that
with KVM, but obviously I didn't suffer any weirdness to begin with :/

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4e6a6417211f..ef9ccfc3a8c0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4051,6 +4051,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
 	unsigned long flags;
 	int cpu, success = 0;
 
+	WARN_ON_ONCE(state & TASK_FREEZABLE);
+
 	preempt_disable();
 	if (p == current) {
 		/*
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Christian Borntraeger]

Am 26.09.22 um 14:55 schrieb Peter Zijlstra:

> Could you please test with something like the below on? I can boot that
> with KVM, but obviously I didn't suffer any weirdness to begin with :/
> 
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 4e6a6417211f..ef9ccfc3a8c0 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4051,6 +4051,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
>   	unsigned long flags;
>   	int cpu, success = 0;
>   
> +	WARN_ON_ONCE(state & TASK_FREEZABLE);
> +
>   	preempt_disable();
>   	if (p == current) {
>   		/*

Does not seem to trigger.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Peter Zijlstra]

On Mon, Sep 26, 2022 at 03:23:10PM +0200, Christian Borntraeger wrote:
> Am 26.09.22 um 14:55 schrieb Peter Zijlstra:
> 
> > Could you please test with something like the below on? I can boot that
> > with KVM, but obviously I didn't suffer any weirdness to begin with :/
> > 
> > ---
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 4e6a6417211f..ef9ccfc3a8c0 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4051,6 +4051,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> >   	unsigned long flags;
> >   	int cpu, success = 0;
> > +	WARN_ON_ONCE(state & TASK_FREEZABLE);
> > +
> >   	preempt_disable();
> >   	if (p == current) {
> >   		/*
> 
> Does not seem to trigger.

Moo -- quite the puzzle this :/ I'll go stare at it more then.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Christian Borntraeger]

Am 26.09.22 um 15:37 schrieb Peter Zijlstra:
> On Mon, Sep 26, 2022 at 03:23:10PM +0200, Christian Borntraeger wrote:
>> Am 26.09.22 um 14:55 schrieb Peter Zijlstra:
>>
>>> Could you please test with something like the below on? I can boot that
>>> with KVM, but obviously I didn't suffer any weirdness to begin with :/
>>>
>>> ---
>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> index 4e6a6417211f..ef9ccfc3a8c0 100644
>>> --- a/kernel/sched/core.c
>>> +++ b/kernel/sched/core.c
>>> @@ -4051,6 +4051,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
>>>    	unsigned long flags;
>>>    	int cpu, success = 0;
>>> +	WARN_ON_ONCE(state & TASK_FREEZABLE);
>>> +
>>>    	preempt_disable();
>>>    	if (p == current) {
>>>    		/*
>>
>> Does not seem to trigger.
> 
> Moo -- quite the puzzle this :/ I'll go stare at it more then.

In the end this is about the end of the sshd process (shutting it down).
I can also trigger the problem by sending a SIGTERM so its not about
the shutdown itself.
Pofiling the guest I see scheduler functions like sched_clock, pick_next_entity,
update_min_vruntime and so on with 100% system time.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Christian Borntraeger]

Am 26.09.22 um 15:37 schrieb Peter Zijlstra:
> On Mon, Sep 26, 2022 at 03:23:10PM +0200, Christian Borntraeger wrote:
>> Am 26.09.22 um 14:55 schrieb Peter Zijlstra:
>>
>>> Could you please test with something like the below on? I can boot that
>>> with KVM, but obviously I didn't suffer any weirdness to begin with :/
>>>
>>> ---
>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> index 4e6a6417211f..ef9ccfc3a8c0 100644
>>> --- a/kernel/sched/core.c
>>> +++ b/kernel/sched/core.c
>>> @@ -4051,6 +4051,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
>>>    	unsigned long flags;
>>>    	int cpu, success = 0;
>>> +	WARN_ON_ONCE(state & TASK_FREEZABLE);
>>> +
>>>    	preempt_disable();
>>>    	if (p == current) {
>>>    		/*
>>
>> Does not seem to trigger.
> 
> Moo -- quite the puzzle this :/ I'll go stare at it more then.

Hmm,

#define ___wait_is_interruptible(state)                                         \
         (!__builtin_constant_p(state) ||                                        \
                 state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)          \

That would not trigger when state is also TASK_FREEZABLE, no?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Peter Zijlstra]

On Mon, Sep 26, 2022 at 05:49:16PM +0200, Christian Borntraeger wrote:

> Hmm,
> 
> #define ___wait_is_interruptible(state)                                         \
>         (!__builtin_constant_p(state) ||                                        \
>                 state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)          \
> 
> That would not trigger when state is also TASK_FREEZABLE, no?

Spot on!

signal_pending_state() writes that as:

	state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)

which is the correct form.

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 14ad8a0e9fac..7f5a51aae0a7 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -281,7 +281,7 @@ static inline void wake_up_pollfree(struct wait_queue_head *wq_head)
 
 #define ___wait_is_interruptible(state)						\
 	(!__builtin_constant_p(state) ||					\
-		state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)		\
+	 (state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
 
 extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
 

Let me go git-grep some to see if there's more similar fail.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Peter Zijlstra]

On Mon, Sep 26, 2022 at 08:06:46PM +0200, Peter Zijlstra wrote:

> Let me go git-grep some to see if there's more similar fail.

I've ended up with the below...

---
 include/linux/wait.h | 2 +-
 kernel/hung_task.c   | 8 ++++++--
 kernel/sched/core.c  | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 14ad8a0e9fac..7f5a51aae0a7 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -281,7 +281,7 @@ static inline void wake_up_pollfree(struct wait_queue_head *wq_head)
 
 #define ___wait_is_interruptible(state)						\
 	(!__builtin_constant_p(state) ||					\
-		state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)		\
+	 (state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
 
 extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
 
diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index f1321c03c32a..4a8a713fd67b 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -191,6 +191,8 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
 	hung_task_show_lock = false;
 	rcu_read_lock();
 	for_each_process_thread(g, t) {
+		unsigned int state;
+
 		if (!max_count--)
 			goto unlock;
 		if (time_after(jiffies, last_break + HUNG_TASK_LOCK_BREAK)) {
@@ -198,8 +200,10 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
 				goto unlock;
 			last_break = jiffies;
 		}
-		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
-		if (READ_ONCE(t->__state) == TASK_UNINTERRUPTIBLE)
+		/* skip the TASK_KILLABLE tasks -- these can be killed */
+		state == READ_ONCE(t->__state);
+		if ((state & TASK_UNINTERRUPTIBLE) &&
+		    !(state & TASK_WAKEKILL))
 			check_hung_task(t, timeout);
 	}
  unlock:
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 1095917ed048..12ee5b98e2c4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8885,7 +8885,7 @@ state_filter_match(unsigned long state_filter, struct task_struct *p)
 	 * When looking for TASK_UNINTERRUPTIBLE skip TASK_IDLE (allows
 	 * TASK_KILLABLE).
 	 */
-	if (state_filter == TASK_UNINTERRUPTIBLE && state == TASK_IDLE)
+	if (state_filter == TASK_UNINTERRUPTIBLE && state & TASK_NOLOAD)
 		return false;
 
 	return true;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Christian Borntraeger]

Am 26.09.22 um 20:22 schrieb Peter Zijlstra:
> On Mon, Sep 26, 2022 at 08:06:46PM +0200, Peter Zijlstra wrote:
> 
>> Let me go git-grep some to see if there's more similar fail.
> 
> I've ended up with the below...

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

Kind of scary that nobody else has reported any regression. I guess the freezable variant is just not used widely.
> 
> ---
>   include/linux/wait.h | 2 +-
>   kernel/hung_task.c   | 8 ++++++--
>   kernel/sched/core.c  | 2 +-
>   3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 14ad8a0e9fac..7f5a51aae0a7 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -281,7 +281,7 @@ static inline void wake_up_pollfree(struct wait_queue_head *wq_head)
>   
>   #define ___wait_is_interruptible(state)						\
>   	(!__builtin_constant_p(state) ||					\
> -		state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)		\
> +	 (state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>   
>   extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
>   
> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
> index f1321c03c32a..4a8a713fd67b 100644
> --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -191,6 +191,8 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>   	hung_task_show_lock = false;
>   	rcu_read_lock();
>   	for_each_process_thread(g, t) {
> +		unsigned int state;
> +
>   		if (!max_count--)
>   			goto unlock;
>   		if (time_after(jiffies, last_break + HUNG_TASK_LOCK_BREAK)) {
> @@ -198,8 +200,10 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>   				goto unlock;
>   			last_break = jiffies;
>   		}
> -		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
> -		if (READ_ONCE(t->__state) == TASK_UNINTERRUPTIBLE)
> +		/* skip the TASK_KILLABLE tasks -- these can be killed */
> +		state == READ_ONCE(t->__state);
> +		if ((state & TASK_UNINTERRUPTIBLE) &&
> +		    !(state & TASK_WAKEKILL))
>   			check_hung_task(t, timeout);
>   	}
>    unlock:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 1095917ed048..12ee5b98e2c4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8885,7 +8885,7 @@ state_filter_match(unsigned long state_filter, struct task_struct *p)
>   	 * When looking for TASK_UNINTERRUPTIBLE skip TASK_IDLE (allows
>   	 * TASK_KILLABLE).
>   	 */
> -	if (state_filter == TASK_UNINTERRUPTIBLE && state == TASK_IDLE)
> +	if (state_filter == TASK_UNINTERRUPTIBLE && state & TASK_NOLOAD)
>   		return false;
>   
>   	return true;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

[Christian Borntraeger]

Am 27.09.22 um 07:35 schrieb Christian Borntraeger:
> 
> 
> Am 26.09.22 um 20:22 schrieb Peter Zijlstra:
>> On Mon, Sep 26, 2022 at 08:06:46PM +0200, Peter Zijlstra wrote:
>>
>>> Let me go git-grep some to see if there's more similar fail.
>>
>> I've ended up with the below...
> 
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Kind of scary that nobody else has reported any regression. I guess the freezable variant is just not used widely.

Will you queue this fix for next soon?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization