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

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

[Byungchul Park]

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 | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 864e673..1841d03 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
 void __closure_wake_up(struct closure_waitlist *wait_list)
 {
 	struct llist_node *list;
-	struct closure *cl;
+	struct closure *cl, *t;
 	struct llist_node *reverse = NULL;
 
 	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_safe(cl, t, reverse, list) {
 		closure_set_waiting(cl, 0);
 		closure_sub(cl, CLOSURE_WAITING + 1);
 	}
-- 
1.9.1

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

[Coly Li]

On 2017/8/7 下午4:38, Byungchul Park wrote:
> Although llist provides proper APIs, they are not used. Make them used.
> 
> Signed-off-by: Byungchul Park <byungchul.park@lge.com
Only have a question about why not using llist_for_each_entry(), it's
still OK with llist_for_each_entry_safe(). The rested part is good to me.

Acked-by: Coly Li <colyli@suse.de>

> ---
>  drivers/md/bcache/closure.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
> index 864e673..1841d03 100644
> --- a/drivers/md/bcache/closure.c
> +++ b/drivers/md/bcache/closure.c
> @@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
>  void __closure_wake_up(struct closure_waitlist *wait_list)
>  {
>  	struct llist_node *list;
> -	struct closure *cl;
> +	struct closure *cl, *t;
>  	struct llist_node *reverse = NULL;
>  
>  	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_safe(cl, t, reverse, list) {

Just wondering why not using llist_for_each_entry(), or you use the
_safe version on purpose ?


>  		closure_set_waiting(cl, 0);
>  		closure_sub(cl, CLOSURE_WAITING + 1);
>  	}
> 


-- 
Coly Li

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

[Byungchul Park]

On Mon, Aug 07, 2017 at 06:18:35PM +0800, Coly Li wrote:
> On 2017/8/7 下午4:38, Byungchul Park wrote:
> > Although llist provides proper APIs, they are not used. Make them used.
> > 
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com
> Only have a question about why not using llist_for_each_entry(), it's

Hello,

The reason is to keep the original logic unchanged. The logic already
does as if it's the safe version against removal.

> still OK with llist_for_each_entry_safe(). The rested part is good to me.
> 
> Acked-by: Coly Li <colyli@suse.de>
> 
> > ---
> >  drivers/md/bcache/closure.c | 17 +++--------------
> >  1 file changed, 3 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
> > index 864e673..1841d03 100644
> > --- a/drivers/md/bcache/closure.c
> > +++ b/drivers/md/bcache/closure.c
> > @@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
> >  void __closure_wake_up(struct closure_waitlist *wait_list)
> >  {
> >  	struct llist_node *list;
> > -	struct closure *cl;
> > +	struct closure *cl, *t;
> >  	struct llist_node *reverse = NULL;
> >  
> >  	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_safe(cl, t, reverse, list) {
> 
> Just wondering why not using llist_for_each_entry(), or you use the
> _safe version on purpose ?

If I use llist_for_each_entry(), then it would change the original
behavior. Is it ok?

Thank you,
Byungchul

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

[Coly Li]

On 2017/8/8 下午12:12, Byungchul Park wrote:
> On Mon, Aug 07, 2017 at 06:18:35PM +0800, Coly Li wrote:
>> On 2017/8/7 下午4:38, Byungchul Park wrote:
>>> Although llist provides proper APIs, they are not used. Make them used.
>>>
>>> Signed-off-by: Byungchul Park <byungchul.park@lge.com
>> Only have a question about why not using llist_for_each_entry(), it's
> 
> Hello,
> 
> The reason is to keep the original logic unchanged. The logic already
> does as if it's the safe version against removal.
> 
>> still OK with llist_for_each_entry_safe(). The rested part is good to me.
>>
>> Acked-by: Coly Li <colyli@suse.de>
>>
>>> ---
>>>  drivers/md/bcache/closure.c | 17 +++--------------
>>>  1 file changed, 3 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
>>> index 864e673..1841d03 100644
>>> --- a/drivers/md/bcache/closure.c
>>> +++ b/drivers/md/bcache/closure.c
>>> @@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
>>>  void __closure_wake_up(struct closure_waitlist *wait_list)
>>>  {
>>>  	struct llist_node *list;
>>> -	struct closure *cl;
>>> +	struct closure *cl, *t;
>>>  	struct llist_node *reverse = NULL;
>>>  
>>>  	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_safe(cl, t, reverse, list) {
>>
>> Just wondering why not using llist_for_each_entry(), or you use the
>> _safe version on purpose ?
> 
> If I use llist_for_each_entry(), then it would change the original
> behavior. Is it ok?
> 

I feel llist_for_each_entry() keeps the original behavior, and variable
't' can be removed. Anyway, either llist_for_each_entry() or
llist_for_each_entry_safe() works correctly and well here. Any one you
use is OK to me, thanks for your informative reply :-)



-- 
Coly Li

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

[Byungchul Park]

On Tue, Aug 08, 2017 at 01:28:39PM +0800, Coly Li wrote:
> >>> +	llist_for_each_entry_safe(cl, t, reverse, list) {
> >>
> >> Just wondering why not using llist_for_each_entry(), or you use the
> >> _safe version on purpose ?
> > 
> > If I use llist_for_each_entry(), then it would change the original
> > behavior. Is it ok?
> > 
> 
> I feel llist_for_each_entry() keeps the original behavior, and variable

Ah.. I see. Then.. Can I change it into non-safe version? Is it still ok
with non-safe one? I will change it at the next spin, if yes.

> 't' can be removed. Anyway, either llist_for_each_entry() or
> llist_for_each_entry_safe() works correctly and well here. Any one you
> use is OK to me, thanks for your informative reply :-)

I rather appriciate it.

Thank you,
Byungchul

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

[Coly Li]

On 2017/8/8 下午2:00, Byungchul Park wrote:
> On Tue, Aug 08, 2017 at 01:28:39PM +0800, Coly Li wrote:
>>>>> +	llist_for_each_entry_safe(cl, t, reverse, list) {
>>>>
>>>> Just wondering why not using llist_for_each_entry(), or you use the
>>>> _safe version on purpose ?
>>>
>>> If I use llist_for_each_entry(), then it would change the original
>>> behavior. Is it ok?
>>>
>>
>> I feel llist_for_each_entry() keeps the original behavior, and variable
> 
> Ah.. I see. Then.. Can I change it into non-safe version? Is it still ok
> with non-safe one? I will change it at the next spin, if yes.
> 
>> 't' can be removed. Anyway, either llist_for_each_entry() or
>> llist_for_each_entry_safe() works correctly and well here. Any one you
>> use is OK to me, thanks for your informative reply :-)
> 
> I rather appriciate it.
> 

Yes, please. And you have my Acked-by :-)


-- 
Coly Li

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

[Nikolay Borisov]

On  8.08.2017 09:00, Byungchul Park wrote:
> On Tue, Aug 08, 2017 at 01:28:39PM +0800, Coly Li wrote:
>>>>> +	llist_for_each_entry_safe(cl, t, reverse, list) {
>>>>
>>>> Just wondering why not using llist_for_each_entry(), or you use the
>>>> _safe version on purpose ?
>>>
>>> If I use llist_for_each_entry(), then it would change the original
>>> behavior. Is it ok?

Generally, _safe versions of list primitives is used when you are going
to perform removal in the iteration. I haven't looked at the code in
bcache but if it's removing entries from the list then _safe version is
required. If you are only iterating - then non-safe version is fine.

>>>
>>
>> I feel llist_for_each_entry() keeps the original behavior, and variable
> 
> Ah.. I see. Then.. Can I change it into non-safe version? Is it still ok
> with non-safe one? I will change it at the next spin, if yes.
> 
>> 't' can be removed. Anyway, either llist_for_each_entry() or
>> llist_for_each_entry_safe() works correctly and well here. Any one you
>> use is OK to me, thanks for your informative reply :-)
> 
> I rather appriciate it.
> 
> Thank you,
> Byungchul
> 

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

[Byungchul Park]

On Wed, Aug 09, 2017 at 09:39:09AM +0300, Nikolay Borisov wrote:
> 
> 
> On  8.08.2017 09:00, Byungchul Park wrote:
> > On Tue, Aug 08, 2017 at 01:28:39PM +0800, Coly Li wrote:
> >>>>> +	llist_for_each_entry_safe(cl, t, reverse, list) {
> >>>>
> >>>> Just wondering why not using llist_for_each_entry(), or you use the
> >>>> _safe version on purpose ?
> >>>
> >>> If I use llist_for_each_entry(), then it would change the original
> >>> behavior. Is it ok?
> 
> Generally, _safe versions of list primitives is used when you are going
> to perform removal in the iteration. I haven't looked at the code in
> bcache but if it's removing entries from the list then _safe version is
> required. If you are only iterating - then non-safe version is fine.

Thank you~ :)

[RESEND PATCH] Don't reinvent the wheel but use existing llilst API

[Byungchul Park]

Hello Linus,

Even though llist APIs exist, serveral code in kernel reinvent the
implementation again and again. Since I think it's *worth* making it use
existing APIs, I submit patches doing it.

Actually I've submitted them to maintainers and commiters about 10 times
for 4 months, but they seem to be too busy to review or take them. Of
course you might be even busier, but it would be appriciated if you do
that instead. It might not take your time much since they are simple.

Patches worked on scheduler, irq_work, vhost/scsi and raid are already
taken by proper maintainers. For now, only ones for bcache, mm, fput and
namespace remain.

Thank you,
Byungchul

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

[Byungchul Park]

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 | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 864e673..1841d03 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
 void __closure_wake_up(struct closure_waitlist *wait_list)
 {
 	struct llist_node *list;
-	struct closure *cl;
+	struct closure *cl, *t;
 	struct llist_node *reverse = NULL;
 
 	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_safe(cl, t, reverse, list) {
 		closure_set_waiting(cl, 0);
 		closure_sub(cl, CLOSURE_WAITING + 1);
 	}
-- 
1.9.1

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

[Byungchul Park]

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 | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 864e673..1841d03 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
 void __closure_wake_up(struct closure_waitlist *wait_list)
 {
 	struct llist_node *list;
-	struct closure *cl;
+	struct closure *cl, *t;
 	struct llist_node *reverse = NULL;
 
 	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_safe(cl, t, reverse, list) {
 		closure_set_waiting(cl, 0);
 		closure_sub(cl, CLOSURE_WAITING + 1);
 	}
-- 
1.9.1

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

[Byungchul Park]

On Fri, May 12, 2017 at 09:35:02AM +0900, Byungchul Park wrote:
> Although llist provides proper APIs, they are not used. Make them used.

+to axboe@kernel.dk

I didn't know your e-mail address has been changed.
Could you give your opinion about this patch?

> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  drivers/md/bcache/closure.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
> index 864e673..1841d03 100644
> --- a/drivers/md/bcache/closure.c
> +++ b/drivers/md/bcache/closure.c
> @@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
>  void __closure_wake_up(struct closure_waitlist *wait_list)
>  {
>  	struct llist_node *list;
> -	struct closure *cl;
> +	struct closure *cl, *t;
>  	struct llist_node *reverse = NULL;
>  
>  	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_safe(cl, t, reverse, list) {
>  		closure_set_waiting(cl, 0);
>  		closure_sub(cl, CLOSURE_WAITING + 1);
>  	}
> -- 
> 1.9.1

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

[Byungchul Park]

On Fri, Jun 02, 2017 at 11:08:48AM +0900, Byungchul Park wrote:
> On Fri, May 12, 2017 at 09:35:02AM +0900, Byungchul Park wrote:
> > Although llist provides proper APIs, they are not used. Make them used.
> 
> +to axboe@kernel.dk
> 
> I didn't know your e-mail address has been changed.
> Could you give your opinion about this patch?

I am sorry if I sent the patch to a wrong person. But otherwise, it
would be appriciated if you give your opinions on it, or let me know
the right person.

Thank you,
Byungchul

> 
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  drivers/md/bcache/closure.c | 17 +++--------------
> >  1 file changed, 3 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
> > index 864e673..1841d03 100644
> > --- a/drivers/md/bcache/closure.c
> > +++ b/drivers/md/bcache/closure.c
> > @@ -64,27 +64,16 @@ void closure_put(struct closure *cl)
> >  void __closure_wake_up(struct closure_waitlist *wait_list)
> >  {
> >  	struct llist_node *list;
> > -	struct closure *cl;
> > +	struct closure *cl, *t;
> >  	struct llist_node *reverse = NULL;
> >  
> >  	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_safe(cl, t, reverse, list) {
> >  		closure_set_waiting(cl, 0);
> >  		closure_sub(cl, CLOSURE_WAITING + 1);
> >  	}
> > -- 
> > 1.9.1