On 2021/5/27 21:16, Jason Gunthorpe wrote:

On Thu, May 27, 2021 at 01:08:37PM +0000, Marciniszyn, Mike wrote:

quoted

quoted

+++ b/drivers/infiniband/hw/hfi1/verbs.c

@@ -534,7 +534,8 @@ static inline void hfi1_handle_packet(struct

hfi1_packet *packet,
 		 * Notify rvt_multicast_detach() if it is waiting for us
 		 * to finish.
 		 */
-		if (atomic_dec_return(&mcast->refcount) <= 1)
+		refcount_dec(&mcast->refcount);
+		if (refcount_read(&mcast->refcount) <= 1)
 			wake_up(&mcast->wait);

Is there refcount_ that preserves the atomic characteristics of the single call?

You are supposed to us refcount_dec_and_test() for patterns like this,
this hunk looks wrong to me

Jason

I made a mistake, thank you.

refcount_dec_and_test() test only if the refcount is 0, so I couldn't find a
good way to replace the atomic_t with refcount_t here.

And there is no refcount_dec/inc_return() for a refcount_t, I found some
previous disscussdion:

https://www.openwall.com/lists/kernel-hardening/2016/11/28/8
https://www.openwall.com/lists/kernel-hardening/2016/12/01/1
https://lkml.org/lkml/2017/9/1/389
https://yhbt.net/lore/all/20200921112218.GB2139@willie-the-truck/t/#md37f8b6084bccbc0ebf76ba249c069372b4d8497

I think such functions undermines the design of refcount. But to be honest, I
didn't find a clear reason.

Peter, could you please explain why you said "add_return and sub_return are
horrible interface for refcount"?

You can review the whole patch at:

https://patchwork.kernel.org/project/linux-rdma/patch/1621925504-33019-14-git-send-email-liweihang@huawei.com/

Thanks
Weihang