netdev - Re: [PATCH] ipvs: Remove unused variable ret from sync_thread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131112162136.GA29065@redhat.com>
Date:	Tue, 12 Nov 2013 17:21:36 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>, Tejun Heo <tj@...nel.org>
Cc:	David Laight <David.Laight@...LAB.COM>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Ingo Molnar <mingo@...nel.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipvs: Remove unused variable ret from
	sync_thread_master()

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);
 }
 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html