| Message ID | 4b96ea99c3f0ccd5cc0683a5c944a1c4da41cc38.1586275373.git.ka-cheong.poon@oracle.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/2] net/rds: Replace direct refcount_inc() by inline function | expand |
On Tue, Apr 07, 2020 at 09:08:01AM -0700, Ka-Cheong Poon wrote: > Added rds_ib_dev_get() and rds_mr_get() to improve code readability. It is very hard to agree with this sentence. Hiding basic kernel primitives is very rare will improve code readability. It is definitely not the case here. Thanks > > Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> > --- > net/rds/ib.c | 8 ++++---- > net/rds/ib.h | 5 +++++ > net/rds/ib_rdma.c | 6 +++--- > net/rds/rdma.c | 8 ++++---- > net/rds/rds.h | 5 +++++ > 5 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/net/rds/ib.c b/net/rds/ib.c > index a792d8a..c16cb1a 100644 > --- a/net/rds/ib.c > +++ b/net/rds/ib.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2006, 2019 Oracle and/or its affiliates. All rights reserved. > + * Copyright (c) 2006, 2020 Oracle and/or its affiliates. All rights reserved. > * > * This software is available to you under a choice of one of two > * licenses. You may choose to be licensed under the terms of the GNU > @@ -224,10 +224,10 @@ static void rds_ib_add_one(struct ib_device *device) > down_write(&rds_ib_devices_lock); > list_add_tail_rcu(&rds_ibdev->list, &rds_ib_devices); > up_write(&rds_ib_devices_lock); > - refcount_inc(&rds_ibdev->refcount); > + rds_ib_dev_get(rds_ibdev); > > ib_set_client_data(device, &rds_ib_client, rds_ibdev); > - refcount_inc(&rds_ibdev->refcount); > + rds_ib_dev_get(rds_ibdev); > > rds_ib_nodev_connect(); > > @@ -258,7 +258,7 @@ struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device) > rcu_read_lock(); > rds_ibdev = ib_get_client_data(device, &rds_ib_client); > if (rds_ibdev) > - refcount_inc(&rds_ibdev->refcount); > + rds_ib_dev_get(rds_ibdev); > rcu_read_unlock(); > return rds_ibdev; > } > diff --git a/net/rds/ib.h b/net/rds/ib.h > index 0296f1f..fe7ea4e 100644 > --- a/net/rds/ib.h > +++ b/net/rds/ib.h > @@ -361,6 +361,11 @@ static inline void rds_ib_dma_sync_sg_for_device(struct ib_device *dev, > extern struct rds_transport rds_ib_transport; > struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device); > void rds_ib_dev_put(struct rds_ib_device *rds_ibdev); > +static inline void rds_ib_dev_get(struct rds_ib_device *rds_ibdev) > +{ > + refcount_inc(&rds_ibdev->refcount); > +} > + > extern struct ib_client rds_ib_client; > > extern unsigned int rds_ib_retry_count; > diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c > index b34b24e..1b942d80 100644 > --- a/net/rds/ib_rdma.c > +++ b/net/rds/ib_rdma.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved. > + * Copyright (c) 2006, 2020 Oracle and/or its affiliates. All rights reserved. > * > * This software is available to you under a choice of one of two > * licenses. You may choose to be licensed under the terms of the GNU > @@ -56,7 +56,7 @@ static struct rds_ib_device *rds_ib_get_device(__be32 ipaddr) > list_for_each_entry_rcu(rds_ibdev, &rds_ib_devices, list) { > list_for_each_entry_rcu(i_ipaddr, &rds_ibdev->ipaddr_list, list) { > if (i_ipaddr->ipaddr == ipaddr) { > - refcount_inc(&rds_ibdev->refcount); > + rds_ib_dev_get(rds_ibdev); > rcu_read_unlock(); > return rds_ibdev; > } > @@ -139,7 +139,7 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con > spin_unlock_irq(&ib_nodev_conns_lock); > > ic->rds_ibdev = rds_ibdev; > - refcount_inc(&rds_ibdev->refcount); > + rds_ib_dev_get(rds_ibdev); > } > > void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn) > diff --git a/net/rds/rdma.c b/net/rds/rdma.c > index 585e6b3..d5abe0e 100644 > --- a/net/rds/rdma.c > +++ b/net/rds/rdma.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2007, 2017 Oracle and/or its affiliates. All rights reserved. > + * Copyright (c) 2007, 2020 Oracle and/or its affiliates. All rights reserved. > * > * This software is available to you under a choice of one of two > * licenses. You may choose to be licensed under the terms of the GNU > @@ -84,7 +84,7 @@ static struct rds_mr *rds_mr_tree_walk(struct rb_root *root, u64 key, > if (insert) { > rb_link_node(&insert->r_rb_node, parent, p); > rb_insert_color(&insert->r_rb_node, root); > - refcount_inc(&insert->r_refcount); > + rds_mr_get(insert); > } > return NULL; > } > @@ -343,7 +343,7 @@ static int __rds_rdma_map(struct rds_sock *rs, struct rds_get_mr_args *args, > > rdsdebug("RDS: get_mr key is %x\n", mr->r_key); > if (mr_ret) { > - refcount_inc(&mr->r_refcount); > + rds_mr_get(mr); > *mr_ret = mr; > } > > @@ -827,7 +827,7 @@ int rds_cmsg_rdma_dest(struct rds_sock *rs, struct rds_message *rm, > if (!mr) > err = -EINVAL; /* invalid r_key */ > else > - refcount_inc(&mr->r_refcount); > + rds_mr_get(mr); > spin_unlock_irqrestore(&rs->rs_rdma_lock, flags); > > if (mr) { > diff --git a/net/rds/rds.h b/net/rds/rds.h > index e4a6035..6a665fa 100644 > --- a/net/rds/rds.h > +++ b/net/rds/rds.h > @@ -953,6 +953,11 @@ static inline void rds_mr_put(struct rds_mr *mr) > __rds_put_mr_final(mr); > } > > +static inline void rds_mr_get(struct rds_mr *mr) > +{ > + refcount_inc(&mr->r_refcount); > +} > + > static inline bool rds_destroy_pending(struct rds_connection *conn) > { > return !check_net(rds_conn_net(conn)) || > -- > 1.8.3.1 >
From: Leon Romanovsky <leon@kernel.org> Date: Tue, 7 Apr 2020 21:48:09 +0300 > On Tue, Apr 07, 2020 at 09:08:01AM -0700, Ka-Cheong Poon wrote: >> Added rds_ib_dev_get() and rds_mr_get() to improve code readability. > > It is very hard to agree with this sentence. > Hiding basic kernel primitives is very rare will improve code readability. > It is definitely not the case here. I completely agree.
On 4/8/20 2:48 AM, Leon Romanovsky wrote: > On Tue, Apr 07, 2020 at 09:08:01AM -0700, Ka-Cheong Poon wrote: >> Added rds_ib_dev_get() and rds_mr_get() to improve code readability. > > It is very hard to agree with this sentence. > Hiding basic kernel primitives is very rare will improve code readability. > It is definitely not the case here. This is to match the rds_ib_dev_put() and rds_mr_put() functions. Isn't it natural to have a pair of *_put()/*_get() functions? Thanks. > Thanks > >> >> Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> >> --- >> net/rds/ib.c | 8 ++++---- >> net/rds/ib.h | 5 +++++ >> net/rds/ib_rdma.c | 6 +++--- >> net/rds/rdma.c | 8 ++++---- >> net/rds/rds.h | 5 +++++ >> 5 files changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/net/rds/ib.c b/net/rds/ib.c >> index a792d8a..c16cb1a 100644 >> --- a/net/rds/ib.c >> +++ b/net/rds/ib.c >> @@ -1,5 +1,5 @@ >> /* >> - * Copyright (c) 2006, 2019 Oracle and/or its affiliates. All rights reserved. >> + * Copyright (c) 2006, 2020 Oracle and/or its affiliates. All rights reserved. >> * >> * This software is available to you under a choice of one of two >> * licenses. You may choose to be licensed under the terms of the GNU >> @@ -224,10 +224,10 @@ static void rds_ib_add_one(struct ib_device *device) >> down_write(&rds_ib_devices_lock); >> list_add_tail_rcu(&rds_ibdev->list, &rds_ib_devices); >> up_write(&rds_ib_devices_lock); >> - refcount_inc(&rds_ibdev->refcount); >> + rds_ib_dev_get(rds_ibdev); >> >> ib_set_client_data(device, &rds_ib_client, rds_ibdev); >> - refcount_inc(&rds_ibdev->refcount); >> + rds_ib_dev_get(rds_ibdev); >> >> rds_ib_nodev_connect(); >> >> @@ -258,7 +258,7 @@ struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device) >> rcu_read_lock(); >> rds_ibdev = ib_get_client_data(device, &rds_ib_client); >> if (rds_ibdev) >> - refcount_inc(&rds_ibdev->refcount); >> + rds_ib_dev_get(rds_ibdev); >> rcu_read_unlock(); >> return rds_ibdev; >> } >> diff --git a/net/rds/ib.h b/net/rds/ib.h >> index 0296f1f..fe7ea4e 100644 >> --- a/net/rds/ib.h >> +++ b/net/rds/ib.h >> @@ -361,6 +361,11 @@ static inline void rds_ib_dma_sync_sg_for_device(struct ib_device *dev, >> extern struct rds_transport rds_ib_transport; >> struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device); >> void rds_ib_dev_put(struct rds_ib_device *rds_ibdev); >> +static inline void rds_ib_dev_get(struct rds_ib_device *rds_ibdev) >> +{ >> + refcount_inc(&rds_ibdev->refcount); >> +} >> + >> extern struct ib_client rds_ib_client; >> >> extern unsigned int rds_ib_retry_count; >> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c >> index b34b24e..1b942d80 100644 >> --- a/net/rds/ib_rdma.c >> +++ b/net/rds/ib_rdma.c >> @@ -1,5 +1,5 @@ >> /* >> - * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved. >> + * Copyright (c) 2006, 2020 Oracle and/or its affiliates. All rights reserved. >> * >> * This software is available to you under a choice of one of two >> * licenses. You may choose to be licensed under the terms of the GNU >> @@ -56,7 +56,7 @@ static struct rds_ib_device *rds_ib_get_device(__be32 ipaddr) >> list_for_each_entry_rcu(rds_ibdev, &rds_ib_devices, list) { >> list_for_each_entry_rcu(i_ipaddr, &rds_ibdev->ipaddr_list, list) { >> if (i_ipaddr->ipaddr == ipaddr) { >> - refcount_inc(&rds_ibdev->refcount); >> + rds_ib_dev_get(rds_ibdev); >> rcu_read_unlock(); >> return rds_ibdev; >> } >> @@ -139,7 +139,7 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con >> spin_unlock_irq(&ib_nodev_conns_lock); >> >> ic->rds_ibdev = rds_ibdev; >> - refcount_inc(&rds_ibdev->refcount); >> + rds_ib_dev_get(rds_ibdev); >> } >> >> void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn) >> diff --git a/net/rds/rdma.c b/net/rds/rdma.c >> index 585e6b3..d5abe0e 100644 >> --- a/net/rds/rdma.c >> +++ b/net/rds/rdma.c >> @@ -1,5 +1,5 @@ >> /* >> - * Copyright (c) 2007, 2017 Oracle and/or its affiliates. All rights reserved. >> + * Copyright (c) 2007, 2020 Oracle and/or its affiliates. All rights reserved. >> * >> * This software is available to you under a choice of one of two >> * licenses. You may choose to be licensed under the terms of the GNU >> @@ -84,7 +84,7 @@ static struct rds_mr *rds_mr_tree_walk(struct rb_root *root, u64 key, >> if (insert) { >> rb_link_node(&insert->r_rb_node, parent, p); >> rb_insert_color(&insert->r_rb_node, root); >> - refcount_inc(&insert->r_refcount); >> + rds_mr_get(insert); >> } >> return NULL; >> } >> @@ -343,7 +343,7 @@ static int __rds_rdma_map(struct rds_sock *rs, struct rds_get_mr_args *args, >> >> rdsdebug("RDS: get_mr key is %x\n", mr->r_key); >> if (mr_ret) { >> - refcount_inc(&mr->r_refcount); >> + rds_mr_get(mr); >> *mr_ret = mr; >> } >> >> @@ -827,7 +827,7 @@ int rds_cmsg_rdma_dest(struct rds_sock *rs, struct rds_message *rm, >> if (!mr) >> err = -EINVAL; /* invalid r_key */ >> else >> - refcount_inc(&mr->r_refcount); >> + rds_mr_get(mr); >> spin_unlock_irqrestore(&rs->rs_rdma_lock, flags); >> >> if (mr) { >> diff --git a/net/rds/rds.h b/net/rds/rds.h >> index e4a6035..6a665fa 100644 >> --- a/net/rds/rds.h >> +++ b/net/rds/rds.h >> @@ -953,6 +953,11 @@ static inline void rds_mr_put(struct rds_mr *mr) >> __rds_put_mr_final(mr); >> } >> >> +static inline void rds_mr_get(struct rds_mr *mr) >> +{ >> + refcount_inc(&mr->r_refcount); >> +} >> + >> static inline bool rds_destroy_pending(struct rds_connection *conn) >> { >> return !check_net(rds_conn_net(conn)) || >> -- >> 1.8.3.1 >>
On Wed, Apr 08, 2020 at 12:15:51PM +0800, Ka-Cheong Poon wrote: > On 4/8/20 2:48 AM, Leon Romanovsky wrote: > > On Tue, Apr 07, 2020 at 09:08:01AM -0700, Ka-Cheong Poon wrote: > > > Added rds_ib_dev_get() and rds_mr_get() to improve code readability. > > > > It is very hard to agree with this sentence. > > Hiding basic kernel primitives is very rare will improve code readability. > > It is definitely not the case here. > > > This is to match the rds_ib_dev_put() and rds_mr_put() functions. > Isn't it natural to have a pair of *_put()/*_get() functions? Ohhh, thank you for pointing that. It is great example why hiding basic primitives is really bad idea. 123 void rds_ib_dev_put(struct rds_ib_device *rds_ibdev) 124 { 125 BUG_ON(refcount_read(&rds_ibdev->refcount) == 0); ^^^^^^ no to this 126 if (refcount_dec_and_test(&rds_ibdev->refcount)) 127 queue_work(rds_wq, &rds_ibdev->free_work); 128 } .... 300 rds_ib_dev_put(rds_ibdev); 301 rds_ib_dev_put(rds_ibdev); Double put -> you wrongly initialized/used refcount. So instead of hiding refcount_inc(), I would say delete your *_put() variants, fix reference counting and convert rds_mr_put() to be normal kref object instead of current implementation. Which does exactly the same like your custom *_put()/_get(), but better and with less errors. Thanks
diff --git a/net/rds/ib.c b/net/rds/ib.c index a792d8a..c16cb1a 100644 --- a/net/rds/ib.c +++ b/net/rds/ib.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2006, 2019 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2006, 2020 Oracle and/or its affiliates. All rights reserved. * * This software is available to you under a choice of one of two * licenses. You may choose to be licensed under the terms of the GNU @@ -224,10 +224,10 @@ static void rds_ib_add_one(struct ib_device *device) down_write(&rds_ib_devices_lock); list_add_tail_rcu(&rds_ibdev->list, &rds_ib_devices); up_write(&rds_ib_devices_lock); - refcount_inc(&rds_ibdev->refcount); + rds_ib_dev_get(rds_ibdev); ib_set_client_data(device, &rds_ib_client, rds_ibdev); - refcount_inc(&rds_ibdev->refcount); + rds_ib_dev_get(rds_ibdev); rds_ib_nodev_connect(); @@ -258,7 +258,7 @@ struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device) rcu_read_lock(); rds_ibdev = ib_get_client_data(device, &rds_ib_client); if (rds_ibdev) - refcount_inc(&rds_ibdev->refcount); + rds_ib_dev_get(rds_ibdev); rcu_read_unlock(); return rds_ibdev; } diff --git a/net/rds/ib.h b/net/rds/ib.h index 0296f1f..fe7ea4e 100644 --- a/net/rds/ib.h +++ b/net/rds/ib.h @@ -361,6 +361,11 @@ static inline void rds_ib_dma_sync_sg_for_device(struct ib_device *dev, extern struct rds_transport rds_ib_transport; struct rds_ib_device *rds_ib_get_client_data(struct ib_device *device); void rds_ib_dev_put(struct rds_ib_device *rds_ibdev); +static inline void rds_ib_dev_get(struct rds_ib_device *rds_ibdev) +{ + refcount_inc(&rds_ibdev->refcount); +} + extern struct ib_client rds_ib_client; extern unsigned int rds_ib_retry_count; diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c index b34b24e..1b942d80 100644 --- a/net/rds/ib_rdma.c +++ b/net/rds/ib_rdma.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2006, 2020 Oracle and/or its affiliates. All rights reserved. * * This software is available to you under a choice of one of two * licenses. You may choose to be licensed under the terms of the GNU @@ -56,7 +56,7 @@ static struct rds_ib_device *rds_ib_get_device(__be32 ipaddr) list_for_each_entry_rcu(rds_ibdev, &rds_ib_devices, list) { list_for_each_entry_rcu(i_ipaddr, &rds_ibdev->ipaddr_list, list) { if (i_ipaddr->ipaddr == ipaddr) { - refcount_inc(&rds_ibdev->refcount); + rds_ib_dev_get(rds_ibdev); rcu_read_unlock(); return rds_ibdev; } @@ -139,7 +139,7 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con spin_unlock_irq(&ib_nodev_conns_lock); ic->rds_ibdev = rds_ibdev; - refcount_inc(&rds_ibdev->refcount); + rds_ib_dev_get(rds_ibdev); } void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn) diff --git a/net/rds/rdma.c b/net/rds/rdma.c index 585e6b3..d5abe0e 100644 --- a/net/rds/rdma.c +++ b/net/rds/rdma.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007, 2017 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2007, 2020 Oracle and/or its affiliates. All rights reserved. * * This software is available to you under a choice of one of two * licenses. You may choose to be licensed under the terms of the GNU @@ -84,7 +84,7 @@ static struct rds_mr *rds_mr_tree_walk(struct rb_root *root, u64 key, if (insert) { rb_link_node(&insert->r_rb_node, parent, p); rb_insert_color(&insert->r_rb_node, root); - refcount_inc(&insert->r_refcount); + rds_mr_get(insert); } return NULL; } @@ -343,7 +343,7 @@ static int __rds_rdma_map(struct rds_sock *rs, struct rds_get_mr_args *args, rdsdebug("RDS: get_mr key is %x\n", mr->r_key); if (mr_ret) { - refcount_inc(&mr->r_refcount); + rds_mr_get(mr); *mr_ret = mr; } @@ -827,7 +827,7 @@ int rds_cmsg_rdma_dest(struct rds_sock *rs, struct rds_message *rm, if (!mr) err = -EINVAL; /* invalid r_key */ else - refcount_inc(&mr->r_refcount); + rds_mr_get(mr); spin_unlock_irqrestore(&rs->rs_rdma_lock, flags); if (mr) { diff --git a/net/rds/rds.h b/net/rds/rds.h index e4a6035..6a665fa 100644 --- a/net/rds/rds.h +++ b/net/rds/rds.h @@ -953,6 +953,11 @@ static inline void rds_mr_put(struct rds_mr *mr) __rds_put_mr_final(mr); } +static inline void rds_mr_get(struct rds_mr *mr) +{ + refcount_inc(&mr->r_refcount); +} + static inline bool rds_destroy_pending(struct rds_connection *conn) { return !check_net(rds_conn_net(conn)) ||
Added rds_ib_dev_get() and rds_mr_get() to improve code readability. Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> --- net/rds/ib.c | 8 ++++---- net/rds/ib.h | 5 +++++ net/rds/ib_rdma.c | 6 +++--- net/rds/rdma.c | 8 ++++---- net/rds/rds.h | 5 +++++ 5 files changed, 21 insertions(+), 11 deletions(-)