| Message ID | 20231118155105.25678-30-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | biops: add atomig find_bit() operations | expand |
On 18.11.23 16:51, Yury Norov wrote: > The function opencodes find_and_set_bit() with a for_each() loop. Fix > it, and make the whole function a simple almost one-liner. > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > --- > net/smc/smc_wr.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c > index 0021065a600a..b6f0cfc52788 100644 > --- a/net/smc/smc_wr.c > +++ b/net/smc/smc_wr.c > @@ -170,15 +170,11 @@ void smc_wr_tx_cq_handler(struct ib_cq *ib_cq, void *cq_context) > > static inline int smc_wr_tx_get_free_slot_index(struct smc_link *link, u32 *idx) > { > - *idx = link->wr_tx_cnt; > if (!smc_link_sendable(link)) > return -ENOLINK; > - for_each_clear_bit(*idx, link->wr_tx_mask, link->wr_tx_cnt) { > - if (!test_and_set_bit(*idx, link->wr_tx_mask)) > - return 0; > - } > - *idx = link->wr_tx_cnt; > - return -EBUSY; > + > + *idx = find_and_set_bit(link->wr_tx_mask, link->wr_tx_cnt); > + return *idx < link->wr_tx_cnt ? 0 : -EBUSY; > } > > /** My understanding is that you can omit the lines with > - *idx = link->wr_tx_cnt; because they only apply to the error paths and you checked that the calling function does not use the idx variable in the error cases. Do I understand this correct? If so the removal of these 2 lines is not related to your change of using find_and_set_bit(), do I understand that correctly? If so, it may be worth mentioning that in the commit message.
The prefix tag and subject imply that it is a bugfix. I think, first, it should be a new feature with net-next tag. Also please use net/smc as prefix. Thanks, Tony Lu On Sat, Nov 18, 2023 at 07:51:00AM -0800, Yury Norov wrote: > The function opencodes find_and_set_bit() with a for_each() loop. Fix > it, and make the whole function a simple almost one-liner. > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > --- > net/smc/smc_wr.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c > index 0021065a600a..b6f0cfc52788 100644 > --- a/net/smc/smc_wr.c > +++ b/net/smc/smc_wr.c > @@ -170,15 +170,11 @@ void smc_wr_tx_cq_handler(struct ib_cq *ib_cq, void *cq_context) > > static inline int smc_wr_tx_get_free_slot_index(struct smc_link *link, u32 *idx) > { > - *idx = link->wr_tx_cnt; > if (!smc_link_sendable(link)) > return -ENOLINK; > - for_each_clear_bit(*idx, link->wr_tx_mask, link->wr_tx_cnt) { > - if (!test_and_set_bit(*idx, link->wr_tx_mask)) > - return 0; > - } > - *idx = link->wr_tx_cnt; > - return -EBUSY; > + > + *idx = find_and_set_bit(link->wr_tx_mask, link->wr_tx_cnt); > + return *idx < link->wr_tx_cnt ? 0 : -EBUSY; > } > > /** > -- > 2.39.2
On Mon, Nov 20, 2023 at 09:43:54AM +0100, Alexandra Winter wrote: > > > On 18.11.23 16:51, Yury Norov wrote: > > The function opencodes find_and_set_bit() with a for_each() loop. Fix > > it, and make the whole function a simple almost one-liner. > > > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > > --- > > net/smc/smc_wr.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c > > index 0021065a600a..b6f0cfc52788 100644 > > --- a/net/smc/smc_wr.c > > +++ b/net/smc/smc_wr.c > > @@ -170,15 +170,11 @@ void smc_wr_tx_cq_handler(struct ib_cq *ib_cq, void *cq_context) > > > > static inline int smc_wr_tx_get_free_slot_index(struct smc_link *link, u32 *idx) > > { > > - *idx = link->wr_tx_cnt; > > if (!smc_link_sendable(link)) > > return -ENOLINK; > > - for_each_clear_bit(*idx, link->wr_tx_mask, link->wr_tx_cnt) { > > - if (!test_and_set_bit(*idx, link->wr_tx_mask)) > > - return 0; > > - } > > - *idx = link->wr_tx_cnt; > > - return -EBUSY; > > + > > + *idx = find_and_set_bit(link->wr_tx_mask, link->wr_tx_cnt); > > + return *idx < link->wr_tx_cnt ? 0 : -EBUSY; > > } > > > > /** > > > My understanding is that you can omit the lines with > > - *idx = link->wr_tx_cnt; > because they only apply to the error paths and you checked that the calling function > does not use the idx variable in the error cases. Do I understand this correct? > > If so the removal of these 2 lines is not related to your change of using find_and_set_bit(), > do I understand that correctly? > > If so, it may be worth mentioning that in the commit message. I'll add: If find_and_set_bit() doesn't acquire a bit, it returns ->wr_tx_cnt, and so explicit initialization of *idx with the same value is unneeded. Makes sense?
On 21.11.23 14:41, Yury Norov wrote: > On Mon, Nov 20, 2023 at 09:43:54AM +0100, Alexandra Winter wrote: >> >> >> On 18.11.23 16:51, Yury Norov wrote: >>> The function opencodes find_and_set_bit() with a for_each() loop. Fix >>> it, and make the whole function a simple almost one-liner. >>> >>> Signed-off-by: Yury Norov <yury.norov@gmail.com> >>> --- >>> net/smc/smc_wr.c | 10 +++------- >>> 1 file changed, 3 insertions(+), 7 deletions(-) >>> >>> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c >>> index 0021065a600a..b6f0cfc52788 100644 >>> --- a/net/smc/smc_wr.c >>> +++ b/net/smc/smc_wr.c >>> @@ -170,15 +170,11 @@ void smc_wr_tx_cq_handler(struct ib_cq *ib_cq, void *cq_context) >>> >>> static inline int smc_wr_tx_get_free_slot_index(struct smc_link *link, u32 *idx) >>> { >>> - *idx = link->wr_tx_cnt; >>> if (!smc_link_sendable(link)) >>> return -ENOLINK; >>> - for_each_clear_bit(*idx, link->wr_tx_mask, link->wr_tx_cnt) { >>> - if (!test_and_set_bit(*idx, link->wr_tx_mask)) >>> - return 0; >>> - } >>> - *idx = link->wr_tx_cnt; >>> - return -EBUSY; >>> + >>> + *idx = find_and_set_bit(link->wr_tx_mask, link->wr_tx_cnt); >>> + return *idx < link->wr_tx_cnt ? 0 : -EBUSY; >>> } >>> >>> /** >> >> >> My understanding is that you can omit the lines with >>> - *idx = link->wr_tx_cnt; >> because they only apply to the error paths and you checked that the calling function >> does not use the idx variable in the error cases. Do I understand this correct? >> >> If so the removal of these 2 lines is not related to your change of using find_and_set_bit(), >> do I understand that correctly? >> >> If so, it may be worth mentioning that in the commit message. > > I'll add: > > If find_and_set_bit() doesn't acquire a bit, it returns > ->wr_tx_cnt, and so explicit initialization of *idx with > the same value is unneeded. > > Makes sense? > Makes sense for the -EBUSY case, thank you. It does not explain that you also removed the line for the -ENOLINK case (which is ok, because the caller has also initialized it to link->wr_tx_cnt)
diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c index 0021065a600a..b6f0cfc52788 100644 --- a/net/smc/smc_wr.c +++ b/net/smc/smc_wr.c @@ -170,15 +170,11 @@ void smc_wr_tx_cq_handler(struct ib_cq *ib_cq, void *cq_context) static inline int smc_wr_tx_get_free_slot_index(struct smc_link *link, u32 *idx) { - *idx = link->wr_tx_cnt; if (!smc_link_sendable(link)) return -ENOLINK; - for_each_clear_bit(*idx, link->wr_tx_mask, link->wr_tx_cnt) { - if (!test_and_set_bit(*idx, link->wr_tx_mask)) - return 0; - } - *idx = link->wr_tx_cnt; - return -EBUSY; + + *idx = find_and_set_bit(link->wr_tx_mask, link->wr_tx_cnt); + return *idx < link->wr_tx_cnt ? 0 : -EBUSY; } /**
The function opencodes find_and_set_bit() with a for_each() loop. Fix it, and make the whole function a simple almost one-liner. Signed-off-by: Yury Norov <yury.norov@gmail.com> --- net/smc/smc_wr.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)