[PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Geert Uytterhoeven]

net/netfilter/ipvs/ip_vs_sync.c: In function 'sync_thread_master':
net/netfilter/ipvs/ip_vs_sync.c:1640:8: warning: unused variable 'ret' [-Wunused-variable]

Introduced by commit 35a2af94c7ce7130ca292c68b1d27fcfdb648f6b ("sched/wait:
Make the __wait_event*() interface more friendly")

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 net/netfilter/ipvs/ip_vs_sync.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index f63c2388f38d..d258125c4202 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1637,7 +1637,7 @@ static int sync_thread_master(void *data)
 			continue;
 		}
 		while (ip_vs_send_sync_msg(tinfo->sock, sb->mesg) < 0) {
-			int ret = __wait_event_interruptible(*sk_sleep(sk),
+			__wait_event_interruptible(*sk_sleep(sk),
 						   sock_writeable(sk) ||
 						   kthread_should_stop());
 			if (unlikely(kthread_should_stop()))
-- 
1.7.9.5

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Peter Zijlstra]

On Tue, Nov 12, 2013 at 02:53:16PM +0100, Geert Uytterhoeven wrote:
> net/netfilter/ipvs/ip_vs_sync.c: In function 'sync_thread_master':
> net/netfilter/ipvs/ip_vs_sync.c:1640:8: warning: unused variable 'ret' [-Wunused-variable]
> 
> Introduced by commit 35a2af94c7ce7130ca292c68b1d27fcfdb648f6b ("sched/wait:
> Make the __wait_event*() interface more friendly")
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  net/netfilter/ipvs/ip_vs_sync.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index f63c2388f38d..d258125c4202 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -1637,7 +1637,7 @@ static int sync_thread_master(void *data)
>  			continue;
>  		}
>  		while (ip_vs_send_sync_msg(tinfo->sock, sb->mesg) < 0) {
> -			int ret = __wait_event_interruptible(*sk_sleep(sk),

So ideally there's be a comment here why we're using interruptible but
then ignore interruptions.

Julian said (
http://lkml.kernel.org/r/alpine.LFD.2.00.1310012245020.1782@ja.ssi.bg ):

" Yes, your patch looks ok to me. In the past
we used ssleep() but IPVS users were confused why
IPVS threads increase the load average. So, we
switched to _interruptible calls and later the socket
polling was added.  "

So maybe add something like

			/*
			 * (Ab)use interruptible sleep to avoid increasing
			 * the load avg.
			 */

> +			__wait_event_interruptible(*sk_sleep(sk),
>  						   sock_writeable(sk) ||
>  						   kthread_should_stop());
>  			if (unlikely(kthread_should_stop()))
> -- 
> 1.7.9.5
> 

RE: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[David Laight]

> > @@ -1637,7 +1637,7 @@ static int sync_thread_master(void *data)
> >  			continue;
> >  		}
> >  		while (ip_vs_send_sync_msg(tinfo->sock, sb->mesg) < 0) {
> > -			int ret = __wait_event_interruptible(*sk_sleep(sk),
> 
> So ideally there's be a comment here why we're using interruptible but
> then ignore interruptions.
> 
> Julian said (
> http://lkml.kernel.org/r/alpine.LFD.2.00.1310012245020.1782@ja.ssi.bg ):
> 
> " Yes, your patch looks ok to me. In the past
> we used ssleep() but IPVS users were confused why
> IPVS threads increase the load average. So, we
> switched to _interruptible calls and later the socket
> polling was added.  "

I've done this in the past so that the code sleeps interruptibly
unless there is a signal pending - which would cause it to return
early.

    /* Tell scheduler we are going to sleep... */
    if (signal_pending(current))
        /* We don't want waking immediately (again) */
        sleep_state = TASK_UNINTERRUPTIBLE;
    else
        sleep_state = TASK_INTERRUPTIBLE;
    set_current_state(sleep_state);

Shame there isn't a process flag to indicate that the process
will sleep uninterruptibly and that it doesn't matter.
So don't count to the load average and don't emit a warning
if it has been sleeping for a long time.

	David

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Peter Zijlstra]

On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> > > @@ -1637,7 +1637,7 @@ static int sync_thread_master(void *data)
> > >  			continue;
> > >  		}
> > >  		while (ip_vs_send_sync_msg(tinfo->sock, sb->mesg) < 0) {
> > > -			int ret = __wait_event_interruptible(*sk_sleep(sk),
> > 
> > So ideally there's be a comment here why we're using interruptible but
> > then ignore interruptions.
> > 
> > Julian said (
> > http://lkml.kernel.org/r/alpine.LFD.2.00.1310012245020.1782@ja.ssi.bg ):
> > 
> > " Yes, your patch looks ok to me. In the past
> > we used ssleep() but IPVS users were confused why
> > IPVS threads increase the load average. So, we
> > switched to _interruptible calls and later the socket
> > polling was added.  "
> 
> I've done this in the past so that the code sleeps interruptibly
> unless there is a signal pending - which would cause it to return
> early.
> 
>     /* Tell scheduler we are going to sleep... */
>     if (signal_pending(current))
>         /* We don't want waking immediately (again) */
>         sleep_state = TASK_UNINTERRUPTIBLE;
>     else
>         sleep_state = TASK_INTERRUPTIBLE;
>     set_current_state(sleep_state);

If this is for kernel threads, I think you can wipe the pending state;
not entirely sure how signals interact with kthreads; Oleg will know.

RE: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[David Laight]

> > I've done this in the past so that the code sleeps interruptibly
> > unless there is a signal pending - which would cause it to return
> > early.
> >
> >     /* Tell scheduler we are going to sleep... */
> >     if (signal_pending(current))
> >         /* We don't want waking immediately (again) */
> >         sleep_state = TASK_UNINTERRUPTIBLE;
> >     else
> >         sleep_state = TASK_INTERRUPTIBLE;
> >     set_current_state(sleep_state);
> 
> If this is for kernel threads, I think you can wipe the pending state;
> not entirely sure how signals interact with kthreads; Oleg will know.

I did take me a while to manage to use kthreads, and we probably still
have customers who insist on using pre 2.6.18 kernels!
(In which case the processes are children of a daemon that only
return to userspace in order to exit.)

We also need to send signals (to get the process out of blocking
socket calls), so I have to use force_sig() to get the signal noticed.

The other issue was tidyup - I had to find module_put_and_exit().

	David

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Peter Zijlstra]

On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> Shame there isn't a process flag to indicate that the process
> will sleep uninterruptibly and that it doesn't matter.
> So don't count to the load average and don't emit a warning
> if it has been sleeping for a long time.

A process flag wouldn't work, because the task could block waiting for
actual work to complete in other sleeps.

However, we could do something like the below; which would allow us
writing things like:

	(void)___wait_event(*sk_sleep(sk),
			    sock_writeable(sk) || kthread_should_stop(),
			    TASK_UNINTERRUPTIBLE | TASK_IDLE, 0, 0,
			    schedule());

Marking the one wait-for-more-work as TASK_IDLE such that it doesn't
contribute to the load avg.

---
 fs/proc/array.c              | 23 ++++++++++++-----------
 include/linux/sched.h        |  8 +++++---
 include/trace/events/sched.h |  3 ++-
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 1bd2077187fd..da9a9c8b5bba 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -133,17 +133,18 @@ static inline void task_name(struct seq_file *m, struct task_struct *p)
  * simple bit tests.
  */
 static const char * const task_state_array[] = {
-	"R (running)",		/*   0 */
-	"S (sleeping)",		/*   1 */
-	"D (disk sleep)",	/*   2 */
-	"T (stopped)",		/*   4 */
-	"t (tracing stop)",	/*   8 */
-	"Z (zombie)",		/*  16 */
-	"X (dead)",		/*  32 */
-	"x (dead)",		/*  64 */
-	"K (wakekill)",		/* 128 */
-	"W (waking)",		/* 256 */
-	"P (parked)",		/* 512 */
+	"R (running)",		/*    0 */
+	"S (sleeping)",		/*    1 */
+	"D (disk sleep)",	/*    2 */
+	"T (stopped)",		/*    4 */
+	"t (tracing stop)",	/*    8 */
+	"Z (zombie)",		/*   16 */
+	"X (dead)",		/*   32 */
+	"x (dead)",		/*   64 */
+	"K (wakekill)",		/*  128 */
+	"W (waking)",		/*  256 */
+	"P (parked)",		/*  512 */
+	"I (idle)",		/* 1024 */
 };
 
 static inline const char *get_task_state(struct task_struct *tsk)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 045b0d227846..a4a9e80688d8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -145,9 +145,10 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq);
 #define TASK_WAKEKILL		128
 #define TASK_WAKING		256
 #define TASK_PARKED		512
-#define TASK_STATE_MAX		1024
+#define TASK_IDLE		1024
+#define TASK_STATE_MAX		2048
 
-#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKWP"
+#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKWPI"
 
 extern char ___assert_task_state[1 - 2*!!(
 		sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
@@ -173,7 +174,8 @@ extern char ___assert_task_state[1 - 2*!!(
 			((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
 #define task_contributes_to_load(task)	\
 				((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
-				 (task->flags & PF_FROZEN) == 0)
+				 (task->flags & PF_FROZEN) == 0 && \
+				 (task->state & TASK_IDLE) == 0)
 
 #define __set_task_state(tsk, state_value)		\
 	do { (tsk)->state = (state_value); } while (0)
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 04c308413a5d..4553a2495c25 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -144,7 +144,8 @@ TRACE_EVENT(sched_switch,
 		  __print_flags(__entry->prev_state & (TASK_STATE_MAX-1), "|",
 				{ 1, "S"} , { 2, "D" }, { 4, "T" }, { 8, "t" },
 				{ 16, "Z" }, { 32, "X" }, { 64, "x" },
-				{ 128, "K" }, { 256, "W" }, { 512, "P" }) : "R",
+				{ 128, "K" }, { 256, "W" }, { 512, "P" },
+				{ 1024, "I" }) : "R",
 		__entry->prev_state & TASK_STATE_MAX ? "+" : "",
 		__entry->next_comm, __entry->next_pid, __entry->next_prio)
 );

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Oleg Nesterov]

On 11/12, Peter Zijlstra wrote:
>
> On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> > Shame there isn't a process flag to indicate that the process
> > will sleep uninterruptibly and that it doesn't matter.
> > So don't count to the load average and don't emit a warning
> > if it has been sleeping for a long time.
>
> A process flag wouldn't work, because the task could block waiting for
> actual work to complete in other sleeps.
>
> However, we could do something like the below; which would allow us
> writing things like:
>
> 	(void)___wait_event(*sk_sleep(sk),
> 			    sock_writeable(sk) || kthread_should_stop(),
> 			    TASK_UNINTERRUPTIBLE | TASK_IDLE, 0, 0,
> 			    schedule());
>
> Marking the one wait-for-more-work as TASK_IDLE such that it doesn't
> contribute to the load avg.

Agreed, I thought about additional bit too.

>  static const char * const task_state_array[] = {
> -	"R (running)",		/*   0 */
> -	"S (sleeping)",		/*   1 */
> -	"D (disk sleep)",	/*   2 */
> -	"T (stopped)",		/*   4 */
> -	"t (tracing stop)",	/*   8 */
> -	"Z (zombie)",		/*  16 */
> -	"X (dead)",		/*  32 */
> -	"x (dead)",		/*  64 */
> -	"K (wakekill)",		/* 128 */
> -	"W (waking)",		/* 256 */
> -	"P (parked)",		/* 512 */
> +	"R (running)",		/*    0 */
> +	"S (sleeping)",		/*    1 */
> +	"D (disk sleep)",	/*    2 */
> +	"T (stopped)",		/*    4 */
> +	"t (tracing stop)",	/*    8 */
> +	"Z (zombie)",		/*   16 */
> +	"X (dead)",		/*   32 */
> +	"x (dead)",		/*   64 */
> +	"K (wakekill)",		/*  128 */
> +	"W (waking)",		/*  256 */
> +	"P (parked)",		/*  512 */
> +	"I (idle)",		/* 1024 */
>  };

but I am not sure about what /proc/ should report in this case...

>  #define task_contributes_to_load(task)	\
>  				((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
> -				 (task->flags & PF_FROZEN) == 0)
> +				 (task->flags & PF_FROZEN) == 0 && \
> +				 (task->state & TASK_IDLE) == 0)

perhaps

	(task->state & (TASK_UNINTERRUPTIBLE | TASK_IDLE)) == TASK_UNINTERRUPTIBLE

can save an insn.

I am also wondering if it makes any sense to turn PF_FROZEN into
TASK_FROZEN, something like (incomplete, probably racy) patch below.
Note that it actually adds the new state, not the the qualifier.

Oleg.

--- x/include/linux/freezer.h
+++ x/include/linux/freezer.h
@@ -23,7 +23,7 @@ extern unsigned int freeze_timeout_msecs
  */
 static inline bool frozen(struct task_struct *p)
 {
-	return p->flags & PF_FROZEN;
+	return p->state & TASK_FROZEN;
 }
 
 extern bool freezing_slow_path(struct task_struct *p);
--- x/kernel/freezer.c
+++ x/kernel/freezer.c
@@ -57,16 +57,13 @@ bool __refrigerator(bool check_kthr_stop
 	pr_debug("%s entered refrigerator\n", current->comm);
 
 	for (;;) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-
 		spin_lock_irq(&freezer_lock);
-		current->flags |= PF_FROZEN;
-		if (!freezing(current) ||
-		    (check_kthr_stop && kthread_should_stop()))
-			current->flags &= ~PF_FROZEN;
+		if (freezing(current) &&
+		    !(check_kthr_stop && kthread_should_stop()))
+			set_current_state(TASK_FROZEN);
 		spin_unlock_irq(&freezer_lock);
 
-		if (!(current->flags & PF_FROZEN))
+		if (!(current->state & TASK_FROZEN))
 			break;
 		was_frozen = true;
 		schedule();
@@ -148,8 +145,7 @@ void __thaw_task(struct task_struct *p)
 	 * refrigerator.
 	 */
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p))
-		wake_up_process(p);
+	try_to_wake_up(p, TASK_FROZEN, 0);
 	spin_unlock_irqrestore(&freezer_lock, flags);
 }
 

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Oleg Nesterov]

On 11/12, Peter Zijlstra wrote:
>
> On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> >
> >     /* Tell scheduler we are going to sleep... */
> >     if (signal_pending(current))
> >         /* We don't want waking immediately (again) */
> >         sleep_state = TASK_UNINTERRUPTIBLE;
> >     else
> >         sleep_state = TASK_INTERRUPTIBLE;
> >     set_current_state(sleep_state);
>
> If this is for kernel threads, I think you can wipe the pending state;

Yes, unless this kthread does allow_signal() signal_pending() can't be
true.

Oleg.

oom-kill && frozen()

[Oleg Nesterov]

On 11/12, Oleg Nesterov wrote:
>
> I am also wondering if it makes any sense to turn PF_FROZEN into
> TASK_FROZEN, something like (incomplete, probably racy) patch below.
> Note that it actually adds the new state, not the the qualifier.

As for the current usage of PF_FROZEN... David, it seems that
oom_scan_process_thread()->__thaw_task() is dead? Probably this
was fine before, when __thaw_task() cleared the "need to freeze"
condition, iirc it was PF_FROZEN.

But today __thaw_task() can't help, no? the task will simply
schedule() in D state again.

Oleg.

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Peter Zijlstra]

On Tue, Nov 12, 2013 at 05:21:36PM +0100, Oleg Nesterov wrote:
> On 11/12, Peter Zijlstra wrote:
> >
> > On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> > > Shame there isn't a process flag to indicate that the process
> > > will sleep uninterruptibly and that it doesn't matter.
> > > So don't count to the load average and don't emit a warning
> > > if it has been sleeping for a long time.
> >
> > A process flag wouldn't work, because the task could block waiting for
> > actual work to complete in other sleeps.
> >
> > However, we could do something like the below; which would allow us
> > writing things like:
> >
> > 	(void)___wait_event(*sk_sleep(sk),
> > 			    sock_writeable(sk) || kthread_should_stop(),
> > 			    TASK_UNINTERRUPTIBLE | TASK_IDLE, 0, 0,
> > 			    schedule());
> >
> > Marking the one wait-for-more-work as TASK_IDLE such that it doesn't
> > contribute to the load avg.
> 
> Agreed, I thought about additional bit too.
> 
> >  static const char * const task_state_array[] = {
> > -	"R (running)",		/*   0 */
> > -	"S (sleeping)",		/*   1 */
> > -	"D (disk sleep)",	/*   2 */
> > -	"T (stopped)",		/*   4 */
> > -	"t (tracing stop)",	/*   8 */
> > -	"Z (zombie)",		/*  16 */
> > -	"X (dead)",		/*  32 */
> > -	"x (dead)",		/*  64 */
> > -	"K (wakekill)",		/* 128 */
> > -	"W (waking)",		/* 256 */
> > -	"P (parked)",		/* 512 */
> > +	"R (running)",		/*    0 */
> > +	"S (sleeping)",		/*    1 */
> > +	"D (disk sleep)",	/*    2 */
> > +	"T (stopped)",		/*    4 */
> > +	"t (tracing stop)",	/*    8 */
> > +	"Z (zombie)",		/*   16 */
> > +	"X (dead)",		/*   32 */
> > +	"x (dead)",		/*   64 */
> > +	"K (wakekill)",		/*  128 */
> > +	"W (waking)",		/*  256 */
> > +	"P (parked)",		/*  512 */
> > +	"I (idle)",		/* 1024 */
> >  };
> 
> but I am not sure about what /proc/ should report in this case...

We have to put in something...

	BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));

However, since we always set it together with TASK_UNINTERUPTIBLE
userspace shouldn't actually ever see the I thing.

> >  #define task_contributes_to_load(task)	\
> >  				((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
> > -				 (task->flags & PF_FROZEN) == 0)
> > +				 (task->flags & PF_FROZEN) == 0 && \
> > +				 (task->state & TASK_IDLE) == 0)
> 
> perhaps
> 
> 	(task->state & (TASK_UNINTERRUPTIBLE | TASK_IDLE)) == TASK_UNINTERRUPTIBLE
> 
> can save an insn.

Fair enough.

> I am also wondering if it makes any sense to turn PF_FROZEN into
> TASK_FROZEN, something like (incomplete, probably racy) patch below.
> Note that it actually adds the new state, not the the qualifier.
> 
> --- x/include/linux/freezer.h
> +++ x/include/linux/freezer.h
> @@ -23,7 +23,7 @@ extern unsigned int freeze_timeout_msecs
>   */
>  static inline bool frozen(struct task_struct *p)
>  {
> -	return p->flags & PF_FROZEN;
> +	return p->state & TASK_FROZEN;

do we want == there? Does it make sense to allow it be set with other
state flags?

>  }
>  
>  extern bool freezing_slow_path(struct task_struct *p);
> --- x/kernel/freezer.c
> +++ x/kernel/freezer.c
> @@ -57,16 +57,13 @@ bool __refrigerator(bool check_kthr_stop
>  	pr_debug("%s entered refrigerator\n", current->comm);
>  
>  	for (;;) {
> -		set_current_state(TASK_UNINTERRUPTIBLE);
> -
>  		spin_lock_irq(&freezer_lock);
> -		current->flags |= PF_FROZEN;
> -		if (!freezing(current) ||
> -		    (check_kthr_stop && kthread_should_stop()))
> -			current->flags &= ~PF_FROZEN;
> +		if (freezing(current) &&
> +		    !(check_kthr_stop && kthread_should_stop()))
> +			set_current_state(TASK_FROZEN);
>  		spin_unlock_irq(&freezer_lock);
>  
> -		if (!(current->flags & PF_FROZEN))
> +		if (!(current->state & TASK_FROZEN))
>  			break;
>  		was_frozen = true;
>  		schedule();
> @@ -148,8 +145,7 @@ void __thaw_task(struct task_struct *p)
>  	 * refrigerator.
>  	 */
>  	spin_lock_irqsave(&freezer_lock, flags);
> -	if (frozen(p))
> -		wake_up_process(p);
> +	try_to_wake_up(p, TASK_FROZEN, 0);
>  	spin_unlock_irqrestore(&freezer_lock, flags);
>  }

Should work I suppose... I'm not entirely sure why that's a PF to begin
with.

Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()

[Oleg Nesterov]

On 11/12, Peter Zijlstra wrote:
>
> On Tue, Nov 12, 2013 at 05:21:36PM +0100, Oleg Nesterov wrote:
> > On 11/12, Peter Zijlstra wrote:
> > >
> > >  static const char * const task_state_array[] = {
> > >  ...
> > > +	"I (idle)",		/* 1024 */
> > >  };
> >
> > but I am not sure about what /proc/ should report in this case...
>
> We have to put in something...
>
> 	BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
>
> However, since we always set it together with TASK_UNINTERUPTIBLE
> userspace shouldn't actually ever see the I thing.

OOPS. I didn't know that get_task_state() does &= TASK_REPORT. So it
can never report anything > EXIT_DEAD.

Perhaps we should change BUILD_BUG_ON() and shrink task_state_array?

	--- x/include/linux/sched.h
	+++ x/include/linux/sched.h
	@@ -163,7 +163,7 @@ extern char ___assert_task_state[1 - 2*!
	 /* get_task_state() */
	 #define TASK_REPORT		(TASK_RUNNING | TASK_INTERRUPTIBLE | \
					 TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
	-				 __TASK_TRACED)
	+				 __TASK_TRACED | EXIT_ZOMBIE | EXIT_DEAD)
	 
	 #define task_is_traced(task)	((task->state & __TASK_TRACED) != 0)
	 #define task_is_stopped(task)	((task->state & __TASK_STOPPED) != 0)
	--- x/fs/proc/array.c
	+++ x/fs/proc/array.c
	@@ -148,16 +148,12 @@ static const char * const task_state_arr
	 
	 static inline const char *get_task_state(struct task_struct *tsk)
	 {
	-	unsigned int state = (tsk->state & TASK_REPORT) | tsk->exit_state;
	+	unsigned int state = (tsk->state | tsk->exit_state) & TASK_REPORT;
		const char * const *p = &task_state_array[0];
	 
	-	BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
	+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array));
	 
	-	while (state) {
	-		p++;
	-		state >>= 1;
	-	}
	-	return *p;
	+	return task_state_array[fls(state)];
	 }
	 
	 static inline void task_state(struct seq_file *m, struct pid_namespace *ns,

> > I am also wondering if it makes any sense to turn PF_FROZEN into
> > TASK_FROZEN, something like (incomplete, probably racy) patch below.
> > Note that it actually adds the new state, not the the qualifier.
> >
> > --- x/include/linux/freezer.h
> > +++ x/include/linux/freezer.h
> > @@ -23,7 +23,7 @@ extern unsigned int freeze_timeout_msecs
> >   */
> >  static inline bool frozen(struct task_struct *p)
> >  {
> > -	return p->flags & PF_FROZEN;
> > +	return p->state & TASK_FROZEN;
>
> do we want == there?

Not really, but if we add TASK_FROZEN then we should probably
add task_is_frozen() just for consistency (frozen() should use
this helper) and all task_is_* helpers do "&".

And,

> Does it make sense to allow it be set with other
> state flags?

Not sure, but _perhaps_ TASK_FROZEN | TASK_KILLABLE can be used
too, say, we can add CGROUP_FROZEN_KILLABLE. Probably makes no
sense, I dunno.

> > --- x/kernel/freezer.c
> > +++ x/kernel/freezer.c
> > @@ -57,16 +57,13 @@ bool __refrigerator(bool check_kthr_stop
> >  	pr_debug("%s entered refrigerator\n", current->comm);
> >
> >  	for (;;) {
> > -		set_current_state(TASK_UNINTERRUPTIBLE);
> > -
> >  		spin_lock_irq(&freezer_lock);
> > -		current->flags |= PF_FROZEN;
> > -		if (!freezing(current) ||
> > -		    (check_kthr_stop && kthread_should_stop()))
> > -			current->flags &= ~PF_FROZEN;
> > +		if (freezing(current) &&
> > +		    !(check_kthr_stop && kthread_should_stop()))
> > +			set_current_state(TASK_FROZEN);
> >  		spin_unlock_irq(&freezer_lock);
> >
> > -		if (!(current->flags & PF_FROZEN))
> > +		if (!(current->state & TASK_FROZEN))
> >  			break;
> >  		was_frozen = true;
> >  		schedule();
> > @@ -148,8 +145,7 @@ void __thaw_task(struct task_struct *p)
> >  	 * refrigerator.
> >  	 */
> >  	spin_lock_irqsave(&freezer_lock, flags);
> > -	if (frozen(p))
> > -		wake_up_process(p);
> > +	try_to_wake_up(p, TASK_FROZEN, 0);
> >  	spin_unlock_irqrestore(&freezer_lock, flags);
> >  }
>
> Should work I suppose...

And perhaps we can simplify this a bit. Not sure we can kill freezer_lock
altogether, but at least __thaw_task() can avoid it.

But I am still not sure the new TASK_ state really makes sense, even if
it looks a bit more clean to me.

OTOH, personally I think that /proc/pid/status should report "F (frozen)"
if frozen(), but this doesn't need TASK_FROZEN of course.

> I'm not entirely sure why that's a PF to begin
> with.

Oh, it had more PF_'s until tj improved (and cleanuped) this code ;)

Oleg.

Re: oom-kill && frozen()

[Tejun Heo]

Hello,

On Tue, Nov 12, 2013 at 05:56:43PM +0100, Oleg Nesterov wrote:
> On 11/12, Oleg Nesterov wrote:
> > I am also wondering if it makes any sense to turn PF_FROZEN into
> > TASK_FROZEN, something like (incomplete, probably racy) patch below.
> > Note that it actually adds the new state, not the the qualifier.
> 
> As for the current usage of PF_FROZEN... David, it seems that
> oom_scan_process_thread()->__thaw_task() is dead? Probably this
> was fine before, when __thaw_task() cleared the "need to freeze"
> condition, iirc it was PF_FROZEN.
> 
> But today __thaw_task() can't help, no? the task will simply
> schedule() in D state again.

Yeah, it'll have to be actively excluded using e.g. PF_FREEZER_SKIP,
which, BTW, can usually only be manipulated by the task itself.  I've
been saying this multiple times but this is yet another cost of having
"frozen" as a separate completely alien task state, which is
essentially close to being undefined when viewed from userland.  We're
spreading broken behaviors and complexity throughout the kernel by
making this half broken state visible.  :(

Thanks.

-- 
tejun

Re: oom-kill && frozen()

[Oleg Nesterov]

On 11/13, Tejun Heo wrote:
>
> Hello,
>
> On Tue, Nov 12, 2013 at 05:56:43PM +0100, Oleg Nesterov wrote:
> > On 11/12, Oleg Nesterov wrote:
> > > I am also wondering if it makes any sense to turn PF_FROZEN into
> > > TASK_FROZEN, something like (incomplete, probably racy) patch below.
> > > Note that it actually adds the new state, not the the qualifier.
> >
> > As for the current usage of PF_FROZEN... David, it seems that
> > oom_scan_process_thread()->__thaw_task() is dead? Probably this
> > was fine before, when __thaw_task() cleared the "need to freeze"
> > condition, iirc it was PF_FROZEN.
> >
> > But today __thaw_task() can't help, no? the task will simply
> > schedule() in D state again.
>
> Yeah, it'll have to be actively excluded using e.g. PF_FREEZER_SKIP,
> which, BTW, can usually only be manipulated by the task itself.

Oh, yes, yes, yes, I agree. PF_FREEZER_SKIP and the growing number of
freezable_schedule() makes this all more confusing.

In fact I was think about something like

	1. Add the new __TASK_FREEZABLE qualifier

	2. Turn freezable_schedule() into

		void freezable_schedule(void)
		{
			spin_lock_irq(&current->pi_lock);
			if (current->state)
				current->state |= __TASK_FREEZABLE
			spin_unlock_irq(&current->pi_lock);
			
			schedule();

			try_to_freeze();
		}

	3. Kill PF_FREEZER_SKIP/freezer_do_not_count/count/should_skip

	4. Change freeze_task() and fake_signal_wake_up()

		-	wake_up_state(p, TASK_INTERRUPTIBLE);
		+	wake_up_state(p, TASK_INTERRUPTIBLE | __TASK_FREEZABLE);

Unfortunately, this can only work if the caller can tolerate the
false wakeup. We can even fix wait_for_vfork_done(), but say
ptrace_stop() can't work this way.

And even if we can make this work, the very fact that freezable_schedule()
does schedule() twice does not look right.

_Perhaps_ we can do something like "selective wakeup"? IOW, ignoring the
races/details,

	1. Add __TASK_FROZEN qualifier _and_ state

	2. Change frozen(),

		static inline bool frozen(struct task_struct *p)
		{
			return p->state & __TASK_FROZEN;
		}

	2. Change freezable_schedule(),

		void freezable_schedule(void)
		{
			spin_lock_irq(&current->pi_lock);
			if (current->state)
				current->state |= __TASK_FROZEN;
			spin_unlock_irq(&current->pi_lock);
			
			schedule();
		}

	3. Change __refrigerator() to use saved_state | __TASK_FROZEN
	   too.

	4. Finally, change try_to_wake_up() path to do

		-	p->state = TASK_WAKING;
		+	p->state &= ~state;
		+	if (p->state & ~(TASK_DEAD | TASK_WAKEKILL | TASK_PARKED))
		+		return;
		+	else
		+		p->state = TASK_WAKING;

	   IOW, if the task sleeps in, say, TASK_INTERRUPTIBLE | __TASK_FROZEN
	   then it need both try_to_wake_up(TASK_INTERRUPTIBLE) and
	   try_to_wake_up(__TASK_FROZEN) to wake up.

	 5. Kill PF_FREEZER_SKIP / etc.

Unfortunately, 4. is obviously needs more changes, although at first glance
nothing really nontrivial... we need a common helper for try_to_wake_up()
and ttwu_remote() which checks/changes ->state and we need to avoid "stat"
if we do not actually wake up.

Hmm. and this all makes me think that at least s/PF_FROZEN/TASK_FROZEN/ as
a first step actually makes some sense... Note the "qualifier _and_ state"
above.

Tejun, Peter, do you think this makes any sense? I am just curious, but
"selective wakeup" looks potentially useful.



And what about oom_scan_process_thread() ? should we simply kill this
dead frozen/__thaw_task code or should we change freezing() to respect
TIF_MEMDIE?

Oleg.

Re: oom-kill && frozen()

[Peter Zijlstra]

On Wed, Nov 13, 2013 at 06:07:24PM +0100, Oleg Nesterov wrote:
> 	4. Finally, change try_to_wake_up() path to do
> 
> 		-	p->state = TASK_WAKING;
> 		+	p->state &= ~state;
> 		+	if (p->state & ~(TASK_DEAD | TASK_WAKEKILL | TASK_PARKED))
> 		+		return;
> 		+	else
> 		+		p->state = TASK_WAKING;
> 
> 	   IOW, if the task sleeps in, say, TASK_INTERRUPTIBLE | __TASK_FROZEN
> 	   then it need both try_to_wake_up(TASK_INTERRUPTIBLE) and
> 	   try_to_wake_up(__TASK_FROZEN) to wake up.

> Tejun, Peter, do you think this makes any sense? I am just curious, but
> "selective wakeup" looks potentially useful.

I've never looked at any of this freeze stuff, so I cannot comment too
much. However we should be very careful not to add too much to relative
hot paths for the relative rare case of freezing stuff.

Re: oom-kill && frozen()

[Oleg Nesterov]

On 11/13, Peter Zijlstra wrote:
>
> On Wed, Nov 13, 2013 at 06:07:24PM +0100, Oleg Nesterov wrote:
> > 	4. Finally, change try_to_wake_up() path to do
> >
> > 		-	p->state = TASK_WAKING;
> > 		+	p->state &= ~state;
> > 		+	if (p->state & ~(TASK_DEAD | TASK_WAKEKILL | TASK_PARKED))
> > 		+		return;
> > 		+	else
> > 		+		p->state = TASK_WAKING;
> >
> > 	   IOW, if the task sleeps in, say, TASK_INTERRUPTIBLE | __TASK_FROZEN
> > 	   then it need both try_to_wake_up(TASK_INTERRUPTIBLE) and
> > 	   try_to_wake_up(__TASK_FROZEN) to wake up.
>
> > Tejun, Peter, do you think this makes any sense? I am just curious, but
> > "selective wakeup" looks potentially useful.
>
> I've never looked at any of this freeze stuff, so I cannot comment too
> much. However we should be very careful not to add too much to relative
> hot paths for the relative rare case of freezing stuff.

Yes, yes, sure.

Plus my description was confusing and incomplete (and I am sure I missed
something more). I forgot to mention that freeze_task() should also add
the FROZEN state if it sees FROZEN qualifier, this needs more changes.

So please forget. May be I'll _try_ to make something working (at least
for discussion), but I am not sure.

Oleg.

__refrigerator() && saved task->state

[Oleg Nesterov]

Sorry for noise, but I am totally confused.

Could you please remind why __refrigerator() saves/restores
task->state?

I can see only one reason: set_freezable() kernel threads which
check kthread_should_stop() and do try_to_freeze() by hand.

But does this save/restore actually help?

For example kauditd_thread() looks obviously racy exactly because
try_to_freeze() can return in TASK_INTERRUPTIBLE state after
wake_up(kauditd_wait) was already called, we can miss an event.

At first glance it would be better to simply kill this logic? If
it was called with ->state != 0, the caller is going to schedule()
and it probably executes the wait_event-like code, in this case
it would me more safe to pretend the task got a spurious wakeup?

(as for kauditd_thread() in particular, it looks wrong in any
 case, even kthread_should_stop() check doesn't look right, it
 needs kthread_freezable_should_stop() afaics).

But I guess I missed something else...

Oleg.

Re: __refrigerator() && saved task->state

[Peter Zijlstra]

On Wed, Nov 13, 2013 at 08:11:43PM +0100, Oleg Nesterov wrote:
> At first glance it would be better to simply kill this logic? If
> it was called with ->state != 0, the caller is going to schedule()
> and it probably executes the wait_event-like code, in this case
> it would me more safe to pretend the task got a spurious wakeup?

Note that in general the kernel cannot deal with spurious wakeups :/

Most proper locks and wait primitives can, but there's enough open-coded
crap out there that can not.

I tried..

Re: __refrigerator() && saved task->state

[Oleg Nesterov]

On 11/13, Peter Zijlstra wrote:
>
> On Wed, Nov 13, 2013 at 08:11:43PM +0100, Oleg Nesterov wrote:
> > At first glance it would be better to simply kill this logic? If
> > it was called with ->state != 0, the caller is going to schedule()
> > and it probably executes the wait_event-like code, in this case
> > it would me more safe to pretend the task got a spurious wakeup?
>
> Note that in general the kernel cannot deal with spurious wakeups :/
>
> Most proper locks and wait primitives can, but there's enough open-coded
> crap out there that can not.

Oh yes, I understand.

My point is, "restore the old state" in this case looks worse simply
because you miss any wakeup in between which was going to clear that
state. And afaics only kthreads can call __refrigerator() in !RUNNING.

But let me repeat, I am almost sure I missed something else.

Oleg.