Re: [PATCH rdma-next 01/10] RDMA/umem: Add reference counting to ib_umem

[Leon Romanovsky <leon@kernel.org>]

On Tue, Feb 03, 2026 at 11:11:39AM +0100, Jiri Pirko wrote:
> Tue, Feb 03, 2026 at 11:03:27AM +0100, leon@kernel.org wrote:
> >On Tue, Feb 03, 2026 at 09:49:53AM +0100, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> Introduce reference counting for ib_umem objects to simplify memory
> >> lifecycle management when umem buffers are shared between the core
> >> layer and device drivers.

The lifecycle should be confined either to the core or to the driver,
but it should not mix responsibilities across both.

> >> 
> >> When the core RDMA layer allocates an ib_umem and passes it to a driver
> >> (e.g., for CQ or QP creation with external buffers), both layers need
> >> to manage the umem lifecycle. Without reference counting, this requires
> >> complex conditional release logic to avoid double-frees on error paths
> >> and leaks on success paths.
> >
> >This sentence doesn't align with the proposed change.
> 
> Hmm, I'm not sure why you think it does not align. It exactly describes
> the code I had it originally without this path in place :)

There is no complex conditional release logic which this reference
counting solves. That counting allows delayed, semi-random release,
nothing more.

> 
> >
> >> 
> >> With reference counting:
> >> - Core allocates umem with refcount=1
> >> - Driver calls ib_umem_get_ref() to take a reference
> >> - Both layers can unconditionally call ib_umem_release()
> >> - The umem is freed only when the last reference is dropped
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> Change-Id: Ifb1765ea3b14dab3329294633ea5df063c74420d
> >
> >Please remove the Change-Ids and write the commit message yourself,
> >without relying on AI. The current message provides no meaningful
> 
> I'm new in RDMA. Not sure what you mean by meaningful information :)

This part of commit message is clearly generated by Cursor:
With reference counting:
- Core allocates umem with refcount=1
- Driver calls ib_umem_get_ref() to take a reference
- Both layers can unconditionally call ib_umem_release()
- The umem is freed only when the last reference is dropped

The above paragraphs have clear AI pattern as they don't explain why,
but only how.

I'm seeing the SAME commit messages in many internals and externals
patches.

Even my AI review is agreed with me :)
...
"AI-authorship-score": "medium"
...

> I'm always trying to provide it.
> 
> >information, particularly the auto‑generated summary at the end.
> 
> Doh, the changeIDs :) Sorry about that.
> 
> 
> >
> >Thanks
> >
> >> ---
> >>  drivers/infiniband/core/umem.c        | 5 +++++
> >>  drivers/infiniband/core/umem_dmabuf.c | 1 +
> >>  drivers/infiniband/core/umem_odp.c    | 3 +++
> >>  include/rdma/ib_umem.h                | 9 +++++++++
> >>  4 files changed, 18 insertions(+)
> >> 
> >> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> >> index 8137031c2a65..09ce694d66ea 100644
> >> --- a/drivers/infiniband/core/umem.c
> >> +++ b/drivers/infiniband/core/umem.c
> >> @@ -192,6 +192,7 @@ struct ib_umem *ib_umem_get(struct ib_device *device, unsigned long addr,
> >>  	umem = kzalloc(sizeof(*umem), GFP_KERNEL);
> >>  	if (!umem)
> >>  		return ERR_PTR(-ENOMEM);
> >> +	refcount_set(&umem->refcount, 1);
> >>  	umem->ibdev      = device;
> >>  	umem->length     = size;
> >>  	umem->address    = addr;
> >> @@ -280,11 +281,15 @@ EXPORT_SYMBOL(ib_umem_get);
> >>  /**
> >>   * ib_umem_release - release memory pinned with ib_umem_get
> >>   * @umem: umem struct to release
> >> + *
> >> + * Decrement the reference count and free the umem when it reaches zero.
> >>   */
> >>  void ib_umem_release(struct ib_umem *umem)
> >>  {
> >>  	if (!umem)
> >>  		return;
> >> +	if (!refcount_dec_and_test(&umem->refcount))
> >> +		return;
> >>  	if (umem->is_dmabuf)
> >>  		return ib_umem_dmabuf_release(to_ib_umem_dmabuf(umem));
> >>  	if (umem->is_odp)
> >> diff --git a/drivers/infiniband/core/umem_dmabuf.c b/drivers/infiniband/core/umem_dmabuf.c
> >> index 939da49b0dcc..5c5286092fca 100644
> >> --- a/drivers/infiniband/core/umem_dmabuf.c
> >> +++ b/drivers/infiniband/core/umem_dmabuf.c
> >> @@ -143,6 +143,7 @@ ib_umem_dmabuf_get_with_dma_device(struct ib_device *device,
> >>  	}
> >>  
> >>  	umem = &umem_dmabuf->umem;
> >> +	refcount_set(&umem->refcount, 1);
> >>  	umem->ibdev = device;
> >>  	umem->length = size;
> >>  	umem->address = offset;
> >> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> >> index 572a91a62a7b..7be30fda57e3 100644
> >> --- a/drivers/infiniband/core/umem_odp.c
> >> +++ b/drivers/infiniband/core/umem_odp.c
> >> @@ -144,6 +144,7 @@ struct ib_umem_odp *ib_umem_odp_alloc_implicit(struct ib_device *device,
> >>  	if (!umem_odp)
> >>  		return ERR_PTR(-ENOMEM);
> >>  	umem = &umem_odp->umem;
> >> +	refcount_set(&umem->refcount, 1);
> >>  	umem->ibdev = device;
> >>  	umem->writable = ib_access_writable(access);
> >>  	umem->owning_mm = current->mm;
> >> @@ -185,6 +186,7 @@ ib_umem_odp_alloc_child(struct ib_umem_odp *root, unsigned long addr,
> >>  	if (!odp_data)
> >>  		return ERR_PTR(-ENOMEM);
> >>  	umem = &odp_data->umem;
> >> +	refcount_set(&umem->refcount, 1);
> >>  	umem->ibdev = root->umem.ibdev;
> >>  	umem->length     = size;
> >>  	umem->address    = addr;
> >> @@ -245,6 +247,7 @@ struct ib_umem_odp *ib_umem_odp_get(struct ib_device *device,
> >>  	if (!umem_odp)
> >>  		return ERR_PTR(-ENOMEM);
> >>  
> >> +	refcount_set(&umem_odp->umem.refcount, 1);
> >>  	umem_odp->umem.ibdev = device;
> >>  	umem_odp->umem.length = size;
> >>  	umem_odp->umem.address = addr;
> >> diff --git a/include/rdma/ib_umem.h b/include/rdma/ib_umem.h
> >> index 0a8e092c0ea8..44264f78eab3 100644
> >> --- a/include/rdma/ib_umem.h
> >> +++ b/include/rdma/ib_umem.h
> >> @@ -10,6 +10,7 @@
> >>  #include <linux/list.h>
> >>  #include <linux/scatterlist.h>
> >>  #include <linux/workqueue.h>
> >> +#include <linux/refcount.h>
> >>  #include <rdma/ib_verbs.h>
> >>  
> >>  struct ib_ucontext;
> >> @@ -19,6 +20,7 @@ struct dma_buf_attach_ops;
> >>  struct ib_umem {
> >>  	struct ib_device       *ibdev;
> >>  	struct mm_struct       *owning_mm;
> >> +	refcount_t		refcount;
> >>  	u64 iova;
> >>  	size_t			length;
> >>  	unsigned long		address;
> >> @@ -110,6 +112,12 @@ static inline bool __rdma_umem_block_iter_next(struct ib_block_iter *biter)
> >>  
> >>  struct ib_umem *ib_umem_get(struct ib_device *device, unsigned long addr,
> >>  			    size_t size, int access);
> >> +
> >> +static inline void ib_umem_get_ref(struct ib_umem *umem)
> >> +{
> >> +	refcount_inc(&umem->refcount);
> >> +}
> >> +
> >>  void ib_umem_release(struct ib_umem *umem);
> >>  int ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,
> >>  		      size_t length);
> >> @@ -188,6 +196,7 @@ static inline struct ib_umem *ib_umem_get(struct ib_device *device,
> >>  {
> >>  	return ERR_PTR(-EOPNOTSUPP);
> >>  }
> >> +static inline void ib_umem_get_ref(struct ib_umem *umem) { }
> >>  static inline void ib_umem_release(struct ib_umem *umem) { }
> >>  static inline int ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,
> >>  		      		    size_t length) {
> >> -- 
> >> 2.51.1
> >> 
> >>