Re: [PATCH] RDS: Simplify code
On 9/4/16 11:23 AM, Leon Romanovsky wrote: On Sun, Sep 04, 2016 at 05:57:20PM +0200, Christophe JAILLET wrote: Le 04/09/2016 à 14:20, Leon Romanovsky a écrit : On Sat, Sep 03, 2016 at 07:33:29AM +0200, Christophe JAILLET wrote: Calling 'list_splice' followed by 'INIT_LIST_HEAD' is equivalent to 'list_splice_init'. It is not 100% accurate list_splice(y, z) INIT_LIST_HEAD(y) ==> if (!list_empty(y)) __list_splice(y, z, z>next); INIT_LIST_HEAD(y) and not if (!list_empty(y)) { __list_splice(y, z, z>next); INIT_LIST_HEAD(y) } as list_splice_init will do. You are right but if you dig further you will see that calling INIT_LIST_HEAD on an empty list is a no-op (AFAIK). And if this list was not already correctly initialized, then you would have some other troubles. Thank you for the suggestion, It looks like the code after that can be skipped in case of loop_conns list is empty, the tmp_list will be empty too. 174 list_for_each_entry_safe(lc, _lc, _list, loop_node) { 175 WARN_ON(lc->conn->c_passive); 176 rds_conn_destroy(lc->conn); 177 } Thanks for trying. As already pointed, your change doesn't simplify much rather change the behavior. The loop cursor already takes care of list empty case. I don't see any reason to change that code. Regards, Santosh
Re: [PATCH] RDS: Simplify code
On Mon, Sep 05, 2016 at 06:38:21AM +0200, Christophe JAILLET wrote: > Le 04/09/2016 à 20:23, Leon Romanovsky a écrit : > >On Sun, Sep 04, 2016 at 05:57:20PM +0200, Christophe JAILLET wrote: > >>Le 04/09/2016 à 14:20, Leon Romanovsky a écrit : > >>>On Sat, Sep 03, 2016 at 07:33:29AM +0200, Christophe JAILLET wrote: > Calling 'list_splice' followed by 'INIT_LIST_HEAD' is equivalent to > 'list_splice_init'. > >>>It is not 100% accurate > >>> > >>>list_splice(y, z) > >>>INIT_LIST_HEAD(y) > >>> > >>>==> > >>> > >>>if (!list_empty(y)) > >>> __list_splice(y, z, z>next); > >>>INIT_LIST_HEAD(y) > >>> > >>>and not > >>> > >>>if (!list_empty(y)) { > >>> __list_splice(y, z, z>next); > >>> INIT_LIST_HEAD(y) > >>>} > >>> > >>>as list_splice_init will do. > >>> > >>You are right but if you dig further you will see that calling > >>INIT_LIST_HEAD on an empty list is a no-op (AFAIK). > >>And if this list was not already correctly initialized, then you would have > >>some other troubles. > >Thank you for the suggestion, > >It looks like the code after that can be skipped in case of loop_conns > >list is empty, the tmp_list will be empty too. > > > >174 list_for_each_entry_safe(lc, _lc, _list, loop_node) { > >175 WARN_ON(lc->conn->c_passive); > >176 rds_conn_destroy(lc->conn); > >177 } > Yes, but this would require some more code and test. This function doesn't > seem to be in a hot path, so I'm not sure that the added complexity would > worth it. > It would require a new 'list_empty()' test and some code rearrangement. > > I suppose that testing for emptiness at the beginning or going through a > list_for_each_entry_safe on a empty list (which will exit immediately and do > nothing) is more or less the same in term of speed. So keep the code simple > and readable. I would expect one list_empty check at the beginning and return immediately, but anyway it doesn't matter. > > CJ > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html signature.asc Description: PGP signature
Re: [PATCH] RDS: Simplify code
On Sun, Sep 04, 2016 at 05:57:20PM +0200, Christophe JAILLET wrote: > Le 04/09/2016 à 14:20, Leon Romanovsky a écrit : > >On Sat, Sep 03, 2016 at 07:33:29AM +0200, Christophe JAILLET wrote: > >>Calling 'list_splice' followed by 'INIT_LIST_HEAD' is equivalent to > >>'list_splice_init'. > >It is not 100% accurate > > > >list_splice(y, z) > >INIT_LIST_HEAD(y) > > > >==> > > > >if (!list_empty(y)) > > __list_splice(y, z, z>next); > >INIT_LIST_HEAD(y) > > > >and not > > > >if (!list_empty(y)) { > > __list_splice(y, z, z>next); > > INIT_LIST_HEAD(y) > >} > > > >as list_splice_init will do. > > > You are right but if you dig further you will see that calling > INIT_LIST_HEAD on an empty list is a no-op (AFAIK). > And if this list was not already correctly initialized, then you would have > some other troubles. Thank you for the suggestion, It looks like the code after that can be skipped in case of loop_conns list is empty, the tmp_list will be empty too. 174 list_for_each_entry_safe(lc, _lc, _list, loop_node) { 175 WARN_ON(lc->conn->c_passive); 176 rds_conn_destroy(lc->conn); 177 } > > CJ > signature.asc Description: PGP signature
Re: [PATCH] RDS: Simplify code
Le 04/09/2016 à 14:20, Leon Romanovsky a écrit : On Sat, Sep 03, 2016 at 07:33:29AM +0200, Christophe JAILLET wrote: Calling 'list_splice' followed by 'INIT_LIST_HEAD' is equivalent to 'list_splice_init'. It is not 100% accurate list_splice(y, z) INIT_LIST_HEAD(y) ==> if (!list_empty(y)) __list_splice(y, z, z>next); INIT_LIST_HEAD(y) and not if (!list_empty(y)) { __list_splice(y, z, z>next); INIT_LIST_HEAD(y) } as list_splice_init will do. You are right but if you dig further you will see that calling INIT_LIST_HEAD on an empty list is a no-op (AFAIK). And if this list was not already correctly initialized, then you would have some other troubles. CJ
Re: [PATCH] RDS: Simplify code
On Sat, Sep 03, 2016 at 07:33:29AM +0200, Christophe JAILLET wrote: > Calling 'list_splice' followed by 'INIT_LIST_HEAD' is equivalent to > 'list_splice_init'. It is not 100% accurate list_splice(y, z) INIT_LIST_HEAD(y) ==> if (!list_empty(y)) __list_splice(y, z, z>next); INIT_LIST_HEAD(y) and not if (!list_empty(y)) { __list_splice(y, z, z>next); INIT_LIST_HEAD(y) } as list_splice_init will do. > > This has been spotted with the following coccinelle script: > / > @@ > expression y,z; > @@ > > - list_splice(y,z); > - INIT_LIST_HEAD(y); > + list_splice_init(y,z); > > Signed-off-by: Christophe JAILLET> --- > net/rds/loop.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/rds/loop.c b/net/rds/loop.c > index f2bf78de5688..c3e6da4fdf97 100644 > --- a/net/rds/loop.c > +++ b/net/rds/loop.c > @@ -167,8 +167,7 @@ void rds_loop_exit(void) > > /* avoid calling conn_destroy with irqs off */ > spin_lock_irq(_conns_lock); > - list_splice(_conns, _list); > - INIT_LIST_HEAD(_conns); > + list_splice_init(_conns, _list); > spin_unlock_irq(_conns_lock); > > list_for_each_entry_safe(lc, _lc, _list, loop_node) { > -- > 2.7.4 > signature.asc Description: PGP signature
[PATCH] RDS: Simplify code
Calling 'list_splice' followed by 'INIT_LIST_HEAD' is equivalent to 'list_splice_init'. This has been spotted with the following coccinelle script: / @@ expression y,z; @@ - list_splice(y,z); - INIT_LIST_HEAD(y); + list_splice_init(y,z); Signed-off-by: Christophe JAILLET--- net/rds/loop.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/rds/loop.c b/net/rds/loop.c index f2bf78de5688..c3e6da4fdf97 100644 --- a/net/rds/loop.c +++ b/net/rds/loop.c @@ -167,8 +167,7 @@ void rds_loop_exit(void) /* avoid calling conn_destroy with irqs off */ spin_lock_irq(_conns_lock); - list_splice(_conns, _list); - INIT_LIST_HEAD(_conns); + list_splice_init(_conns, _list); spin_unlock_irq(_conns_lock); list_for_each_entry_safe(lc, _lc, _list, loop_node) { -- 2.7.4