| Message ID | 1472880809-29216-1-git-send-email-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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 <christophe.jaillet@wanadoo.fr> > --- > 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(&loop_conns_lock); > - list_splice(&loop_conns, &tmp_list); > - INIT_LIST_HEAD(&loop_conns); > + list_splice_init(&loop_conns, &tmp_list); > spin_unlock_irq(&loop_conns_lock); > > list_for_each_entry_safe(lc, _lc, &tmp_list, loop_node) { > -- > 2.7.4 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, &tmp_list, loop_node) { 175 WARN_ON(lc->conn->c_passive); 176 rds_conn_destroy(lc->conn); 177 } > > CJ >
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, &tmp_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. CJ -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, &tmp_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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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, &tmp_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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(&loop_conns_lock); - list_splice(&loop_conns, &tmp_list); - INIT_LIST_HEAD(&loop_conns); + list_splice_init(&loop_conns, &tmp_list); spin_unlock_irq(&loop_conns_lock); list_for_each_entry_safe(lc, _lc, &tmp_list, loop_node) {
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 <christophe.jaillet@wanadoo.fr> --- net/rds/loop.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)