[PATCH] sched: Enhance readability of iterating wake_list

[PATCH] sched: Enhance readability of iterating wake_list

[Byungchul Park]

This is just for enhancing readability and a trivial one. Please take
this only if you agree.

-----8<-----
>From bb8cfcaa21ce157fb890a3e878fed2f2b7dadf81 Mon Sep 17 00:00:00 2001
From: Byungchul Park <byungchul.park@lge.com>
Date: Fri, 10 Feb 2017 12:53:14 +0900
Subject: [PATCH] sched: Enhance readability of iterating wake_list

Code for interating wake_list can be simpler so that it becomes more
readable and can be seen at a glance.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/core.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d01f9d0..66e7743 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1769,6 +1769,11 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
+#define for_each_wake_list(task, node) \
+	for ((task) = llist_entry((node), struct task_struct, wake_entry); \
+	     node; (node) = llist_next(node), \
+	     (task) = llist_entry((node), struct task_struct, wake_entry))
+
 void sched_ttwu_pending(void)
 {
 	struct rq *rq = this_rq();
@@ -1783,17 +1788,8 @@ void sched_ttwu_pending(void)
 	raw_spin_lock_irqsave(&rq->lock, flags);
 	rq_pin_lock(rq, &rf);
 
-	while (llist) {
-		int wake_flags = 0;
-
-		p = llist_entry(llist, struct task_struct, wake_entry);
-		llist = llist_next(llist);
-
-		if (p->sched_remote_wakeup)
-			wake_flags = WF_MIGRATED;
-
-		ttwu_do_activate(rq, p, wake_flags, &rf);
-	}
+	for_each_wake_list(p, llist)
+		ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : 0, &rf);
 
 	rq_unpin_lock(rq, &rf);
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
-- 
1.9.1

Re: [PATCH] sched: Enhance readability of iterating wake_list

[Peter Zijlstra]

On Fri, Feb 10, 2017 at 01:09:31PM +0900, Byungchul Park wrote:
> +#define for_each_wake_list(task, node) \
> +	for ((task) = llist_entry((node), struct task_struct, wake_entry); \
> +	     node; (node) = llist_next(node), \
> +	     (task) = llist_entry((node), struct task_struct, wake_entry))
> +

How about you make that llist_for_each(pos, member) or similar and
replace all while (foo) { foo = llist_next(foo); } instances?

Because most llist_next() users have the same pattern.

Re: [PATCH] sched: Enhance readability of iterating wake_list

[Byungchul Park]

On Fri, Feb 10, 2017 at 08:55:23AM +0100, Peter Zijlstra wrote:
> On Fri, Feb 10, 2017 at 01:09:31PM +0900, Byungchul Park wrote:
> > +#define for_each_wake_list(task, node) \
> > +	for ((task) = llist_entry((node), struct task_struct, wake_entry); \
> > +	     node; (node) = llist_next(node), \
> > +	     (task) = llist_entry((node), struct task_struct, wake_entry))
> > +
> 
> How about you make that llist_for_each(pos, member) or similar and
> replace all while (foo) { foo = llist_next(foo); } instances?
> 
> Because most llist_next() users have the same pattern.

Yes, it would be better. I will do it.

Thank you.

Re: [PATCH] sched: Enhance readability of iterating wake_list

[Byungchul Park]

On Fri, Feb 10, 2017 at 08:55:23AM +0100, Peter Zijlstra wrote:
> On Fri, Feb 10, 2017 at 01:09:31PM +0900, Byungchul Park wrote:
> > +#define for_each_wake_list(task, node) \
> > +	for ((task) = llist_entry((node), struct task_struct, wake_entry); \
> > +	     node; (node) = llist_next(node), \
> > +	     (task) = llist_entry((node), struct task_struct, wake_entry))
> > +
> 
> How about you make that llist_for_each(pos, member) or similar and
> replace all while (foo) { foo = llist_next(foo); } instances?
> 
> Because most llist_next() users have the same pattern.

I did what you recommand, like the following.

Would it be better if I split the patch into several ones?
Or keep it one patch?

-----8<-----
diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 864e673..7d5286b 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -70,21 +70,10 @@ void __closure_wake_up(struct closure_waitlist *wait_list)
 	list = llist_del_all(&wait_list->list);
 
 	/* We first reverse the list to preserve FIFO ordering and fairness */
-
-	while (list) {
-		struct llist_node *t = list;
-		list = llist_next(list);
-
-		t->next = reverse;
-		reverse = t;
-	}
+	reverse = llist_reverse_order(list);
 
 	/* Then do the wakeups */
-
-	while (reverse) {
-		cl = container_of(reverse, struct closure, list);
-		reverse = llist_next(reverse);
-
+	llist_for_each_entry(cl, reverse, list) {
 		closure_set_waiting(cl, 0);
 		closure_sub(cl, CLOSURE_WAITING + 1);
 	}
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 36c13e4..c82243a 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -359,11 +359,9 @@ static int release_stripe_list(struct r5conf *conf,
 
 	head = llist_del_all(&conf->released_stripes);
 	head = llist_reverse_order(head);
-	while (head) {
+	llist_for_each_entry(sh, head, release_list) {
 		int hash;
 
-		sh = llist_entry(head, struct stripe_head, release_list);
-		head = llist_next(head);
 		/* sh could be readded after STRIPE_ON_RELEASE_LIST is cleard */
 		smp_mb();
 		clear_bit(STRIPE_ON_RELEASE_LIST, &sh->state);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 253310c..280b912 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -501,9 +501,7 @@ static void vhost_scsi_evt_work(struct vhost_work *work)
 
 	mutex_lock(&vq->mutex);
 	llnode = llist_del_all(&vs->vs_event_list);
-	while (llnode) {
-		evt = llist_entry(llnode, struct vhost_scsi_evt, list);
-		llnode = llist_next(llnode);
+	llist_for_each_entry(evt, llnode, list) {
 		vhost_scsi_do_evt_work(vs, evt);
 		vhost_scsi_free_evt(vs, evt);
 	}
@@ -529,10 +527,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
 
 	bitmap_zero(signal, VHOST_SCSI_MAX_VQ);
 	llnode = llist_del_all(&vs->vs_completion_list);
-	while (llnode) {
-		cmd = llist_entry(llnode, struct vhost_scsi_cmd,
-				     tvc_completion_list);
-		llnode = llist_next(llnode);
+	llist_for_each_entry(cmd, llnode, tvc_completion_list) {
 		se_cmd = &cmd->tvc_se_cmd;
 
 		pr_debug("%s tv_cmd %p resid %u status %#02x\n", __func__,
diff --git a/fs/file_table.c b/fs/file_table.c
index 6d982b5..8800153 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -232,11 +232,10 @@ static void delayed_fput(struct work_struct *unused)
 {
 	struct llist_node *node = llist_del_all(&delayed_fput_list);
 	struct llist_node *next;
+	struct file *f;
 
-	for (; node; node = next) {
-		next = llist_next(node);
-		__fput(llist_entry(node, struct file, f_u.fu_llist));
-	}
+	llist_for_each_entry(f, node, f_u.fu_llist)
+		__fput(f);
 }
 
 static void ____fput(struct callback_head *work)
@@ -310,7 +309,7 @@ void put_filp(struct file *file)
 }
 
 void __init files_init(void)
-{ 
+{
 	filp_cachep = kmem_cache_create("filp", sizeof(struct file), 0,
 			SLAB_HWCACHE_ALIGN | SLAB_PANIC, NULL);
 	percpu_counter_init(&nr_files, 0, GFP_KERNEL);
@@ -329,4 +328,4 @@ void __init files_maxfiles_init(void)
 	n = ((totalram_pages - memreserve) * (PAGE_SIZE / 1024)) / 10;
 
 	files_stat.max_files = max_t(unsigned long, n, NR_FILE);
-} 
+}
diff --git a/fs/namespace.c b/fs/namespace.c
index b5b1259..0caf954 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1083,11 +1083,10 @@ static void delayed_mntput(struct work_struct *unused)
 {
 	struct llist_node *node = llist_del_all(&delayed_mntput_list);
 	struct llist_node *next;
+	struct mount *m;
 
-	for (; node; node = next) {
-		next = llist_next(node);
-		cleanup_mnt(llist_entry(node, struct mount, mnt_llist));
-	}
+	llist_for_each_entry(m, node, mnt_llist)
+		cleanup_mnt(m);
 }
 static DECLARE_DELAYED_WORK(delayed_mntput_work, delayed_mntput);
 
@@ -1615,7 +1614,7 @@ void __detach_mounts(struct dentry *dentry)
 	namespace_unlock();
 }
 
-/* 
+/*
  * Is the caller allowed to modify his namespace?
  */
 static inline bool may_mount(void)
@@ -2159,7 +2158,7 @@ static int do_loopback(struct path *path, const char *old_name,
 
 	err = -EINVAL;
 	if (mnt_ns_loop(old_path.dentry))
-		goto out; 
+		goto out;
 
 	mp = lock_mount(path);
 	err = PTR_ERR(mp);
diff --git a/include/linux/llist.h b/include/linux/llist.h
index fd4ca0b..418f566 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -160,11 +160,6 @@ static inline bool llist_empty(const struct llist_head *head)
 	return ACCESS_ONCE(head->first) == NULL;
 }
 
-static inline struct llist_node *llist_next(struct llist_node *node)
-{
-	return node->next;
-}
-
 extern bool llist_add_batch(struct llist_node *new_first,
 			    struct llist_node *new_last,
 			    struct llist_head *head);
diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index bcf107c..e2ebe8c 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -138,11 +138,7 @@ static void irq_work_run_list(struct llist_head *list)
 		return;
 
 	llnode = llist_del_all(list);
-	while (llnode != NULL) {
-		work = llist_entry(llnode, struct irq_work, llnode);
-
-		llnode = llist_next(llnode);
-
+	llist_for_each_entry(work, llnode, llnode) {
 		/*
 		 * Clear the PENDING bit, after this point the @work
 		 * can be re-used.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d01f9d0..417060b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void)
 	raw_spin_lock_irqsave(&rq->lock, flags);
 	rq_pin_lock(rq, &rf);
 
-	while (llist) {
-		int wake_flags = 0;
-
-		p = llist_entry(llist, struct task_struct, wake_entry);
-		llist = llist_next(llist);
-
-		if (p->sched_remote_wakeup)
-			wake_flags = WF_MIGRATED;
-
-		ttwu_do_activate(rq, p, wake_flags, &rf);
-	}
+	llist_for_each_entry(p, llist, wake_entry)
+		ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : 0, &rf);
 
 	rq_unpin_lock(rq, &rf);
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 3ca82d4..829de9e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -50,11 +50,9 @@ static void free_work(struct work_struct *w)
 {
 	struct vfree_deferred *p = container_of(w, struct vfree_deferred, wq);
 	struct llist_node *llnode = llist_del_all(&p->list);
-	while (llnode) {
-		void *p = llnode;
-		llnode = llist_next(llnode);
-		__vunmap(p, 1);
-	}
+
+	llist_for_each(llnode, llnode)
+		__vunmap((void *)llnode, 1);
 }
 
 /*** Page table manipulation functions ***/