[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~ :)