non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]

non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]

[Hannes Weisbach]

Hi,

I just started working with librdmacm and I was wondering if there is a specific reason why rdma_reg_* functions and rdma_post_send/write functions take the local memory address as non-const pointer "void * addr". These functions shouldn't and don't change the memory pointed to by addr. I think this should be made explicit by using the type const void * for addr.
In case you agree, I would volunteer to make the necessary changes.

Best regards,
Hannes Weisbach--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]

[Hefty, Sean]

> I just started working with librdmacm and I was wondering if there is a
> specific reason why rdma_reg_* functions and rdma_post_send/write functions
> take the local memory address as non-const pointer "void * addr". These
> functions shouldn't and don't change the memory pointed to by addr. I think
> this should be made explicit by using the type const void * for addr.
> In case you agree, I would volunteer to make the necessary changes.

The librdmacm calls simply abstract the libibverbs ibv_post_send() call.  I agree that making them const makes sense, but the inline code would simply cast away the const anyway.  Either way seems fine with me.  If you submitted a patch, I would accept it.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]

[Yann Droneaud]

Hi,

Le mercredi 27 novembre 2013 à 19:00 +0100, Hannes Weisbach a écrit :

> I just started working with librdmacm and I was wondering if there is a specific reason why rdma_reg_* functions and rdma_post_send/write functions take the local memory address as non-const pointer "void * addr". These functions shouldn't and don't change the memory pointed to by addr. I think this should be made explicit by using the type const void * for addr.
> In case you agree, I would volunteer to make the necessary changes.
> 

:)

Two days ago I started to work on a kernel patchset to address similar
concerns on the verbs/RDMA APIs

BTW, it's easier to change the kernel "internal" API than the public
userspace API of the library. But adding "const" should not break
compilation of existing userspace program.

PS: this issue could also be raised against libibverbs, so once you've
fixed librdmacm, why not fix libibverbs after ?

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Add const qualifier in rdma_reg*, rdma_post_send*|write and ibv_reg_mr (was: Re: non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write])

[Hannes Weisbach]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

Hi.
> Two days ago I started to work on a kernel patchset to address similar
> concerns on the verbs/RDMA APIs
Nice.
> 
> BTW, it's easier to change the kernel "internal" API than the public
> userspace API of the library. But adding "const" should not break
> compilation of existing userspace program.
That was my thought too.  However, because those functions are defined
in as inline in a header, they actually do break user code.  For
example, rdma_reg_msgs passes a const void * to ibv_reg_mr, which
takes a void *, thus resulting in a warning.  If you (like me) compile
with the -Werror flag, this results in broken code.  The only
offending libibverbs function I found so far is ibv_reg_mr().
> 
> PS: this issue could also be raised against libibverbs, so once you've
> fixed librdmacm, why not fix libibverbs after ?
This was my plan.  But first I wanted to see if there even is interest
in fixing this.  As a user I deal with librdmacm directly, so I
started to fix it first.

Anyway, since patching librdmacm alone would break user code, I also
included a patch for libibverbs, which adds the const qualifier to
'addr' parameter ib ibv_reg_mr().

lists.openfabrics.org says, this is also the mailing list for
libibverbs (README is outdated ;)), I hope this is ok.

Best regards,
Hannes


[-- Attachment #2: 0001-Make-addr-parameter-of-rdma_reg_-and-rdma_post_send-.patch --]
[-- Type: application/octet-stream, Size: 2839 bytes --]

From 74b73fe292c73fd9e7906e883588809c0908a404 Mon Sep 17 00:00:00 2001
From: Hannes Weisbach <hannes.weisbach-cl+VPiYnx/09lWAAmjDAixS11BummzK+@public.gmane.org>
Date: Thu, 28 Nov 2013 20:35:07 +0100
Subject: [PATCH] Make 'addr' parameter of rdma_reg_* and rdma_post_send/write
 const

The following functions should only read from their 'addr' parameter and this
should be made explicit in the API by adding a const qualifier to the
parameter:
rdma_reg_[read|write|msgs]
rdma_post_[send|write|ud_send]

Signed-off-by: Hannes Weisbach <hannes_weisbach-hi6Y0CQ0nG0@public.gmane.org>
---
 include/rdma/rdma_verbs.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/rdma/rdma_verbs.h b/include/rdma/rdma_verbs.h
index 198c6a5..57e8f1e 100644
--- a/include/rdma/rdma_verbs.h
+++ b/include/rdma/rdma_verbs.h
@@ -64,20 +64,20 @@ void rdma_destroy_srq(struct rdma_cm_id *id);
  * Memory registration helpers.
  */
 static inline struct ibv_mr *
-rdma_reg_msgs(struct rdma_cm_id *id, void *addr, size_t length)
+rdma_reg_msgs(struct rdma_cm_id *id, const void *addr, size_t length)
 {
 	return ibv_reg_mr(id->pd, addr, length, IBV_ACCESS_LOCAL_WRITE);
 }
 
 static inline struct ibv_mr *
-rdma_reg_read(struct rdma_cm_id *id, void *addr, size_t length)
+rdma_reg_read(struct rdma_cm_id *id, const void *addr, size_t length)
 {
 	return ibv_reg_mr(id->pd, addr, length, IBV_ACCESS_LOCAL_WRITE |
 						IBV_ACCESS_REMOTE_READ);
 }
 
 static inline struct ibv_mr *
-rdma_reg_write(struct rdma_cm_id *id, void *addr, size_t length)
+rdma_reg_write(struct rdma_cm_id *id, const void *addr, size_t length)
 {
 	return ibv_reg_mr(id->pd, addr, length, IBV_ACCESS_LOCAL_WRITE |
 						IBV_ACCESS_REMOTE_WRITE);
@@ -182,7 +182,7 @@ rdma_post_recv(struct rdma_cm_id *id, void *context, void *addr,
 }
 
 static inline int
-rdma_post_send(struct rdma_cm_id *id, void *context, void *addr,
+rdma_post_send(struct rdma_cm_id *id, void *context, const void *addr,
 	       size_t length, struct ibv_mr *mr, int flags)
 {
 	struct ibv_sge sge;
@@ -209,7 +209,7 @@ rdma_post_read(struct rdma_cm_id *id, void *context, void *addr,
 }
 
 static inline int
-rdma_post_write(struct rdma_cm_id *id, void *context, void *addr,
+rdma_post_write(struct rdma_cm_id *id, void *context, const void *addr,
 		size_t length, struct ibv_mr *mr, int flags,
 		uint64_t remote_addr, uint32_t rkey)
 {
@@ -223,7 +223,7 @@ rdma_post_write(struct rdma_cm_id *id, void *context, void *addr,
 }
 
 static inline int
-rdma_post_ud_send(struct rdma_cm_id *id, void *context, void *addr,
+rdma_post_ud_send(struct rdma_cm_id *id, void *context, const void *addr,
 		  size_t length, struct ibv_mr *mr, int flags,
 		  struct ibv_ah *ah, uint32_t remote_qpn)
 {
-- 
1.8.3.2


[-- Attachment #3: 0001-Make-ibv_mr_reg-parameter-void-addr-const.patch --]
[-- Type: application/octet-stream, Size: 1197 bytes --]

From 624b7c6c3236d299a14352525724194ebaf82214 Mon Sep 17 00:00:00 2001
From: Hannes Weisbach <hannes.weisbach-cl+VPiYnx/09lWAAmjDAixS11BummzK+@public.gmane.org>
Date: Thu, 28 Nov 2013 20:19:59 +0100
Subject: [PATCH] Make ibv_mr_reg parameter 'void * addr' const

ibv_reg_mr does not modify the memory pointed to by addr. By declaring addr
const, this fact is made explicit in the API. The main reason for this change
is the similar API-adaption in librdmacm. Also, passing non-const pointers
still works, thus this change does not break user code.

Signed-off-by: Hannes Weisbach <hannes_weisbach-hi6Y0CQ0nG0@public.gmane.org>
---
 include/infiniband/verbs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
index 4b1ab57..eeaa3cc 100644
--- a/include/infiniband/verbs.h
+++ b/include/infiniband/verbs.h
@@ -837,7 +837,7 @@ int ibv_dealloc_pd(struct ibv_pd *pd);
 /**
  * ibv_reg_mr - Register a memory region
  */
-struct ibv_mr *ibv_reg_mr(struct ibv_pd *pd, void *addr,
+struct ibv_mr *ibv_reg_mr(struct ibv_pd *pd, const void *addr,
 			  size_t length, int access);
 
 /**
-- 
1.8.3.2

Re: [PATCH] Add const qualifier in rdma_reg*, rdma_post_send*|write and ibv_reg_mr (was: Re: non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write])

[Yann Droneaud]

Hi,

Le jeudi 28 novembre 2013 à 20:47 +0100, Hannes Weisbach a écrit :

> > But adding "const" should not break compilation of existing userspace program.

> That was my thought too.  However, because those functions are defined
> in as inline in a header, they actually do break user code.  For
> example, rdma_reg_msgs passes a const void * to ibv_reg_mr, which
> takes a void *, thus resulting in a warning.  If you (like me) compile
> with the -Werror flag, this results in broken code.  The only
> offending libibverbs function I found so far is ibv_reg_mr().

> > 
> > PS: this issue could also be raised against libibverbs, so once you've
> > fixed librdmacm, why not fix libibverbs after ?
> This was my plan.  But first I wanted to see if there even is interest
> in fixing this.  As a user I deal with librdmacm directly, so I
> started to fix it first.
> 
> Anyway, since patching librdmacm alone would break user code, I also
> included a patch for libibverbs, which adds the const qualifier to
> 'addr' parameter ib ibv_reg_mr().
> 

To avoid adding cast to non-const in librdmacm that no one will remove
once added, you're correct in patching libibverbs first.

> lists.openfabrics.org says, this is also the mailing list for
> libibverbs (README is outdated ;)), I hope this is ok.
> 

It's OK. Roland Dreier, libibverbs maintainer, is here (but sadly not as
often as we would like).

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html