2020-11-30 08:58:05

by Yejune Deng

Subject: [PATCH] infiniband: i40iw: replace atomic_add_return()

atomic_inc_return() is a little neater

Signed-off-by: Yejune Deng <[email protected]>
---
drivers/infiniband/hw/i40iw/i40iw_cm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c b/drivers/infiniband/hw/i40iw/i40iw_cm.c
index 3053c345..26e92ae 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
@@ -2426,7 +2426,7 @@ static void i40iw_handle_rst_pkt(struct i40iw_cm_node *cm_node,
}
break;
case I40IW_CM_STATE_MPAREQ_RCVD:
- atomic_add_return(1, &cm_node->passive_state);
+ atomic_inc_return(&cm_node->passive_state);
break;
case I40IW_CM_STATE_ESTABLISHED:
case I40IW_CM_STATE_SYN_RCVD:
@@ -3020,7 +3020,7 @@ static int i40iw_cm_reject(struct i40iw_cm_node *cm_node, const void *pdata, u8
i40iw_cleanup_retrans_entry(cm_node);

if (!loopback) {
- passive_state = atomic_add_return(1, &cm_node->passive_state);
+ passive_state = atomic_inc_return(&cm_node->passive_state);
if (passive_state == I40IW_SEND_RESET_EVENT) {
cm_node->state = I40IW_CM_STATE_CLOSED;
i40iw_rem_ref_cm_node(cm_node);
@@ -3678,7 +3678,7 @@ int i40iw_accept(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
return -EINVAL;
}

- passive_state = atomic_add_return(1, &cm_node->passive_state);
+ passive_state = atomic_inc_return(&cm_node->passive_state);
if (passive_state == I40IW_SEND_RESET_EVENT) {
i40iw_rem_ref_cm_node(cm_node);
return -ECONNRESET;
--
1.9.1

2020-12-01 14:45:43

by Shiraz Saleem

Subject: RE: [PATCH] infiniband: i40iw: replace atomic_add_return()

> Subject: [PATCH] infiniband: i40iw: replace atomic_add_return()
>
> atomic_inc_return() is a little neater
>
> Signed-off-by: Yejune Deng <[email protected]>
> ---
> drivers/infiniband/hw/i40iw/i40iw_cm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c
> b/drivers/infiniband/hw/i40iw/i40iw_cm.c
> index 3053c345..26e92ae 100644
> --- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
> +++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
> @@ -2426,7 +2426,7 @@ static void i40iw_handle_rst_pkt(struct i40iw_cm_node
> *cm_node,
> }
> break;
> case I40IW_CM_STATE_MPAREQ_RCVD:
> - atomic_add_return(1, &cm_node->passive_state);
> + atomic_inc_return(&cm_node->passive_state);

Just an atomic_inc would suffice here.

> break;
> case I40IW_CM_STATE_ESTABLISHED:
> case I40IW_CM_STATE_SYN_RCVD:
> @@ -3020,7 +3020,7 @@ static int i40iw_cm_reject(struct i40iw_cm_node
> *cm_node, const void *pdata, u8
> i40iw_cleanup_retrans_entry(cm_node);
>
> if (!loopback) {
> - passive_state = atomic_add_return(1, &cm_node->passive_state);
> + passive_state = atomic_inc_return(&cm_node->passive_state);

Fine with it as its consistent across i40iw. But aren't there many more instances of this across the tree?
Isn't this a choice best left to the developer?

2020-12-01 22:39:04

by Jason Gunthorpe

Subject: Re: [PATCH] infiniband: i40iw: replace atomic_add_return()

On Tue, Dec 01, 2020 at 02:42:25PM +0000, Saleem, Shiraz wrote:

> > case I40IW_CM_STATE_ESTABLISHED:
> > case I40IW_CM_STATE_SYN_RCVD:
> > @@ -3020,7 +3020,7 @@ static int i40iw_cm_reject(struct i40iw_cm_node
> > *cm_node, const void *pdata, u8
> > i40iw_cleanup_retrans_entry(cm_node);
> >
> > if (!loopback) {
> > - passive_state = atomic_add_return(1, &cm_node->passive_state);
> > + passive_state = atomic_inc_return(&cm_node->passive_state);
>
> Fine with it as its consistent across i40iw. But aren't there many
> more instances of this across the tree? Isn't this a choice best
> left to the developer?

I don't think it is a style choice, the narrowest applicable atomic
should always be used as a matter of clarity and performance.

Jason

2020-12-07 19:46:59

by Jason Gunthorpe

Subject: Re: [PATCH] infiniband: i40iw: replace atomic_add_return()

On Mon, Nov 30, 2020 at 04:52:56PM +0800, Yejune Deng wrote:
> atomic_inc_return() is a little neater
>
> Signed-off-by: Yejune Deng <[email protected]>
> ---
> drivers/infiniband/hw/i40iw/i40iw_cm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)

Applied to for-next with the note from Shiraz

Thanks,
Jason