Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1579394 > unrolled thread

[PATCH] llist: Don't reinvent the wheel but use existing llist API

Back to article view | Back to linux.kernel

Contents

  [PATCH] llist: Don't reinvent the wheel but use existing llist API Byungchul Park <byungchul.park@lge.com> - 2017-02-13 05:20 +0100
    Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API NeilBrown <neilb@suse.com> - 2017-02-13 06:00 +0100
      Re: [PATCH] llist: Don't reinvent the wheel but use existing llist  API Byungchul Park <byungchul.park@lge.com> - 2017-02-13 06:20 +0100
    Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API "Huang\, Ying" <ying.huang@intel.com> - 2017-02-13 06:40 +0100
      Re: [PATCH] llist: Don't reinvent the wheel but use existing llist  API Byungchul Park <byungchul.park@lge.com> - 2017-02-13 07:00 +0100
    Re: [PATCH] llist: Don't reinvent the wheel but use existing llist  API Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-13 06:50 +0100
      Re: [PATCH] llist: Don't reinvent the wheel but use existing llist  API Byungchul Park <byungchul.park@lge.com> - 2017-02-13 07:10 +0100

#1579394 — [PATCH] llist: Don't reinvent the wheel but use existing llist API

Although llist provides proper APIs, they are not used. Make them used.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 drivers/md/bcache/closure.c | 15 ++-------------
 drivers/md/raid5.c          |  4 +---
 drivers/vhost/scsi.c        |  9 ++-------
 fs/file_table.c             | 12 +++++-------
 fs/namespace.c              | 12 +++++-------
 include/linux/llist.h       |  3 +++
 kernel/irq_work.c           |  6 +-----
 kernel/sched/core.c         | 13 ++-----------
 mm/vmalloc.c                |  8 +++-----
 9 files changed, 24 insertions(+), 58 deletions(-)

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..69bf625 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -231,12 +231,10 @@ static void __fput(struct file *file)
 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 +308,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 +327,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..69e1b07 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1082,12 +1082,10 @@ static void __cleanup_mnt(struct rcu_head *head)
 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 +1613,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 +2157,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..3fc8ac5 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -160,6 +160,9 @@ static inline bool llist_empty(const struct llist_head *head)
 	return ACCESS_ONCE(head->first) == NULL;
 }
 
+/*
+ * Is this necessary? This should be obsolete unless users exist.
+ */
 static inline struct llist_node *llist_next(struct llist_node *node)
 {
 	return node->next;
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 ***/
-- 
1.9.1

[toc] | [next] | [standalone]

#1579405

[Multipart message — attachments visible in raw view] — view raw

On Mon, Feb 13 2017, Byungchul Park wrote:

> Although llist provides proper APIs, they are not used. Make them used.
>
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  drivers/md/bcache/closure.c | 15 ++-------------
>  drivers/md/raid5.c          |  4 +---
>  drivers/vhost/scsi.c        |  9 ++-------
>  fs/file_table.c             | 12 +++++-------
>  fs/namespace.c              | 12 +++++-------
>  include/linux/llist.h       |  3 +++
>  kernel/irq_work.c           |  6 +-----
>  kernel/sched/core.c         | 13 ++-----------
>  mm/vmalloc.c                |  8 +++-----
>  9 files changed, 24 insertions(+), 58 deletions(-)
>
...

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

This one is wrong (I haven't checked the rest).
As soon a STRIPE_ON_RELEASE_LIST is cleared, the llist_node can be
reused, so we need to call llist_next *before* the rest of the code.
You have moved the call to afterwards.

You could possibly change it to use llist_for_each_entry_safe()

NeilBrown

[toc] | [prev] | [next] | [standalone]

#1579412 — Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API

On Mon, Feb 13, 2017 at 03:56:08PM +1100, NeilBrown wrote:
> On Mon, Feb 13 2017, Byungchul Park wrote:
> 
> > Although llist provides proper APIs, they are not used. Make them used.
> >
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  drivers/md/bcache/closure.c | 15 ++-------------
> >  drivers/md/raid5.c          |  4 +---
> >  drivers/vhost/scsi.c        |  9 ++-------
> >  fs/file_table.c             | 12 +++++-------
> >  fs/namespace.c              | 12 +++++-------
> >  include/linux/llist.h       |  3 +++
> >  kernel/irq_work.c           |  6 +-----
> >  kernel/sched/core.c         | 13 ++-----------
> >  mm/vmalloc.c                |  8 +++-----
> >  9 files changed, 24 insertions(+), 58 deletions(-)
> >
> ...
> 
> > 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);
> 
> This one is wrong (I haven't checked the rest).
> As soon a STRIPE_ON_RELEASE_LIST is cleared, the llist_node can be
> reused, so we need to call llist_next *before* the rest of the code.
> You have moved the call to afterwards.
> 
> You could possibly change it to use llist_for_each_entry_safe()

Yes right. I also found that I should have used the safe version. :(

Thank you very much.

> 
> NeilBrown

[toc] | [prev] | [next] | [standalone]

#1579416

Hi, Byungchul,

Byungchul Park <byungchul.park@lge.com> writes:

> Although llist provides proper APIs, they are not used. Make them used.
>
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  drivers/md/bcache/closure.c | 15 ++-------------
>  drivers/md/raid5.c          |  4 +---
>  drivers/vhost/scsi.c        |  9 ++-------
>  fs/file_table.c             | 12 +++++-------
>  fs/namespace.c              | 12 +++++-------
>  include/linux/llist.h       |  3 +++
>  kernel/irq_work.c           |  6 +-----
>  kernel/sched/core.c         | 13 ++-----------
>  mm/vmalloc.c                |  8 +++-----

I think you need to split the patch.  One for each subsystem, this makes
it easier to be reviewed and merged.

Best Regards,
Huang, Ying

[snip]

[toc] | [prev] | [next] | [standalone]

#1579450 — Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API

On Mon, Feb 13, 2017 at 01:38:36PM +0800, Huang, Ying wrote:
> Hi, Byungchul,
> 
> Byungchul Park <byungchul.park@lge.com> writes:
> 
> > Although llist provides proper APIs, they are not used. Make them used.
> >
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  drivers/md/bcache/closure.c | 15 ++-------------
> >  drivers/md/raid5.c          |  4 +---
> >  drivers/vhost/scsi.c        |  9 ++-------
> >  fs/file_table.c             | 12 +++++-------
> >  fs/namespace.c              | 12 +++++-------
> >  include/linux/llist.h       |  3 +++
> >  kernel/irq_work.c           |  6 +-----
> >  kernel/sched/core.c         | 13 ++-----------
> >  mm/vmalloc.c                |  8 +++-----
> 
> I think you need to split the patch.  One for each subsystem, this makes
> it easier to be reviewed and merged.

Thank you. I will do it.

> 
> Best Regards,
> Huang, Ying
> 
> [snip]

[toc] | [prev] | [next] | [standalone]

#1579435 — Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API

On Mon, Feb 13, 2017 at 01:10:13PM +0900, Byungchul Park wrote:
> Although llist provides proper APIs, they are not used. Make them used.

> @@ -231,12 +231,10 @@ static void __fput(struct file *file)
>  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);
>  }

#define llist_for_each_entry(pos, node, member)                         \
        for ((pos) = llist_entry((node), typeof(*(pos)), member);       \
             &(pos)->member != NULL;                                    \
             (pos) = llist_entry((pos)->member.next, typeof(*(pos)), member))

Now, think what happens after __fput() frees the damn thing.  In the
step of that loop, that is.

That kind of pattern (find next, do something with the current, proceed to
the next we'd found before) is a strong hint that this "do something"
might remove the thing from the list, or outright destroy it.  Both
file_table.c and namespace.c chunks are breaking exactly that kind of
places.

Please, don't do this kind of conversions blindly.  There _is_ another
iterating primitive for such places, but figuring out which one is right
is not something you can do without understanding what the code is doing.
And no, blind replacement of all such loops with llist_for_each_entry_safe,
just in case, is not a good idea either.

[toc] | [prev] | [next] | [standalone]

#1579455 — Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API

On Mon, Feb 13, 2017 at 05:43:26AM +0000, Al Viro wrote:
> On Mon, Feb 13, 2017 at 01:10:13PM +0900, Byungchul Park wrote:
> > Although llist provides proper APIs, they are not used. Make them used.
> 
> > @@ -231,12 +231,10 @@ static void __fput(struct file *file)
> >  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);
> >  }
> 
> #define llist_for_each_entry(pos, node, member)                         \
>         for ((pos) = llist_entry((node), typeof(*(pos)), member);       \
>              &(pos)->member != NULL;                                    \
>              (pos) = llist_entry((pos)->member.next, typeof(*(pos)), member))
> 
> Now, think what happens after __fput() frees the damn thing.  In the
> step of that loop, that is.
> 
> That kind of pattern (find next, do something with the current, proceed to
> the next we'd found before) is a strong hint that this "do something"
> might remove the thing from the list, or outright destroy it.  Both
> file_table.c and namespace.c chunks are breaking exactly that kind of
> places.
> 
> Please, don't do this kind of conversions blindly.  There _is_ another
> iterating primitive for such places, but figuring out which one is right
> is not something you can do without understanding what the code is doing.

I'm really sorry. I made a mistake. I should have used the safe version in
the case.

> And no, blind replacement of all such loops with llist_for_each_entry_safe,
> just in case, is not a good idea either.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web