RE: non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]
> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] osm_sm_state_mgr.c Fix handling of polling retry number
On 11/27/13 13:16, Hal Rosenstock wrote:
On 11/15/2013 7:15 AM, Line Holen wrote:
The retry counter is now only updated if a packet is actually sent.
(But as before the initial request is also counted.)
Prior to this change the actual maximum number of packets sent were
polling retry number minus one.
Signed-off-by: Line Holen
---
diff --git a/opensm/osm_sm_state_mgr.c b/opensm/osm_sm_state_mgr.c
index 596ad8f..6eff9ee 100644
--- a/opensm/osm_sm_state_mgr.c
+++ b/opensm/osm_sm_state_mgr.c
@@ -197,16 +197,14 @@ void osm_sm_state_mgr_polling_callback(IN void *context)
}
/*
-* Incr the retry number.
-* If it reached the max_retry_number in the subnet opt - call
+* If retry number reached the max_retry_number in the subnet opt - call
* osm_sm_state_mgr_process with signal OSM_SM_SIGNAL_POLLING_TIMEOUT
*/
- sm->retry_number++;
OSM_LOG(sm->p_log, OSM_LOG_VERBOSE, "SM State %d (%s), Retry
number:%d\n",
sm->p_subn->sm_state,
osm_get_sm_mgr_state_str(sm->p_subn->sm_state),
sm->retry_number);
- if (sm->retry_number>= sm->p_subn->opt.polling_retry_number) {
+ if (sm->retry_number> sm->p_subn->opt.polling_retry_number) {
OSM_LOG(sm->p_log, OSM_LOG_DEBUG,
"Reached polling_retry_number value in retry_number. "
"Go to DISCOVERY state\n");
@@ -214,6 +212,9 @@ void osm_sm_state_mgr_polling_callback(IN void *context)
goto Exit;
}
+ /* Increment the retry number */
+ sm->retry_number++;
Would it be better to increment retry number if
sm_state_mgr_send_master_sm_info_req call just below this succeeds ?
-- Hal
I'm not sure really. The current placement was to avoid potential race
with response handling
and the clearing of the counter there (incrementing after the response
were received). Seemed
to me that this could happen with the current locking.
Line
+
/* Send a SubnGet(SMInfo) request to the remote sm (depends on our
state) */
sm_state_mgr_send_master_sm_info_req(sm);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for v3.13 2/7] IB/uverbs: remove implicit cast in INIT_UDATA()
Hi Matan, Le mercredi 27 novembre 2013 à 10:21 +0200, Matan Barak a écrit : > On 27/11/2013 12:02 AM, Yann Droneaud wrote: ... > > INIT_UDATA(&udata, buf + sizeof cmd, > > - (unsigned long) cmd.response + sizeof resp, > > + (void __user *)(unsigned long)cmd.response + sizeof resp, > > The response field is already __u64 and casting to (void __user *) > should match the machine's pointer type size. Why do we have to cast to > (unsigned long) and then cast to (void __user *) ? > On 32bit ABI, u64 is not matching the size of the pointer. Without the cast to unsigned long, GCC complains with: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] So the cast is required on 32bit platforms. Regards. -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] osm_sm_state_mgr.c Fix handling of polling retry number
On 11/15/2013 7:15 AM, Line Holen wrote:
> The retry counter is now only updated if a packet is actually sent.
> (But as before the initial request is also counted.)
>
> Prior to this change the actual maximum number of packets sent were
> polling retry number minus one.
>
> Signed-off-by: Line Holen
>
> ---
>
> diff --git a/opensm/osm_sm_state_mgr.c b/opensm/osm_sm_state_mgr.c
> index 596ad8f..6eff9ee 100644
> --- a/opensm/osm_sm_state_mgr.c
> +++ b/opensm/osm_sm_state_mgr.c
> @@ -197,16 +197,14 @@ void osm_sm_state_mgr_polling_callback(IN void *context)
> }
>
> /*
> - * Incr the retry number.
> - * If it reached the max_retry_number in the subnet opt - call
> + * If retry number reached the max_retry_number in the subnet opt - call
>* osm_sm_state_mgr_process with signal OSM_SM_SIGNAL_POLLING_TIMEOUT
>*/
> - sm->retry_number++;
> OSM_LOG(sm->p_log, OSM_LOG_VERBOSE, "SM State %d (%s), Retry
> number:%d\n",
> sm->p_subn->sm_state,
> osm_get_sm_mgr_state_str(sm->p_subn->sm_state),
> sm->retry_number);
>
> - if (sm->retry_number >= sm->p_subn->opt.polling_retry_number) {
> + if (sm->retry_number > sm->p_subn->opt.polling_retry_number) {
> OSM_LOG(sm->p_log, OSM_LOG_DEBUG,
> "Reached polling_retry_number value in retry_number. "
> "Go to DISCOVERY state\n");
> @@ -214,6 +212,9 @@ void osm_sm_state_mgr_polling_callback(IN void *context)
> goto Exit;
> }
>
> + /* Increment the retry number */
> + sm->retry_number++;
Would it be better to increment retry number if
sm_state_mgr_send_master_sm_info_req call just below this succeeds ?
-- Hal
> +
> /* Send a SubnGet(SMInfo) request to the remote sm (depends on our
> state) */
> sm_state_mgr_send_master_sm_info_req(sm);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for v3.13 0/7] create_flow/destroy_flow fixes for v3.13
On 27/11/2013 12:02 AM, Yann Droneaud wrote: Hi, Please find a patchset against create_flow/destroy_flow and associated extended command scheme. These are fixes that must be applied before making the new uverbs widely available. This patchset gather some patches already sent independently: - The first two patches were already sent[1] to address a warning reported by sparse. - The next patch was already sent[2] to handle an uncommon type of extended command. The three patches ensure that commands will be extensible: - One patch add a missing check of comp_mask - Two patches add checks on reserved fields following advice from an article read today[3]. The last patches fix an error path. Please review and apply for v3.13. Regards. [1] [PATCH for-next 0/2] Fix "drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer" warning http://marc.info/?i=cover.1384869925.git.ydrone...@opteya.com [2] [PATCH for-next] IB/uverbs: set ucore.outbuf to NULL if core response space is omitted http://marc.info/?i=1384872527-26154-1-git-send-email-ydrone...@opteya.com [3] "Botching up ioctls" by Daniel Vetter http://blog.ffwll.ch/2013/11/botching-up-ioctls.html Yann Droneaud (7): IB/core: const'ify inbuf in struct ib_udata IB/uverbs: remove implicit cast in INIT_UDATA() IB/uverbs: set outbuf to NULL when no core response space is provided IB/uverbs: check reserved field in extended command header IB/uverbs: check comp_mask in destroy_flow IB/uverbs: check reserved fields in create_flow IB/uverbs: set error code when fail to consume all flow_spec items drivers/infiniband/core/uverbs.h | 12 ++-- drivers/infiniband/core/uverbs_cmd.c | 31 +-- drivers/infiniband/core/uverbs_main.c | 16 +++- include/rdma/ib_verbs.h | 2 +- 4 files changed, 39 insertions(+), 22 deletions(-) Hi, Great series Yann. Thanks for your contribution. Best regards, Matan -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for v3.13 2/7] IB/uverbs: remove implicit cast in INIT_UDATA()
On 27/11/2013 12:02 AM, Yann Droneaud wrote:
Currently, INIT_UDATA() does an implicit cast to a pointer,
so that 'response' address, eg. output buffer, can be used
as is to initialize a struct ib_udata:
do {\
(udata)->inbuf = (void __user *) (ibuf); \
(udata)->outbuf = (void __user *) (obuf); \
(udata)->inlen = (ilen); \
(udata)->outlen = (olen); \
} while (0)
...
INIT_UDATA(&udata, buf + sizeof cmd,
(unsigned long) cmd.response + sizeof resp,
in_len - sizeof cmd, out_len - sizeof resp);
...
Hidding the integer to pointer conversion is prone to error
that won't be catched by compiler/static analyzer is some case.
In the other hand, sparse reports an error if literal 0 is used
to initialize inbuf or outbuf, for example in:
INIT_UDATA(&ucore,
(hdr.in_words) ? buf : 0,
(unsigned long)ex_hdr.response,
hdr.in_words * 8,
hdr.out_words * 8);
It was reported by kbuild test robot in message[1]:
From: kbuild test robot
Subject: "drivers/infiniband/core/uverbs_main.c:683:17:
sparse: Using plain integer as NULL pointer",
Message-Id: <528b3984.SVGs20ZWpcuR/jls%fengguang...@intel.com>
This patch fixes the warnings reported by sparse and allows the compiler
to report a warning in case a plain integer get used to initialize
a udata pointer.
This patch requires struct ib_udata to be modified to have a
const void __user *inbuf field[2], otherwise compiler will report warnings
regarding const to non const conversion:
drivers/infiniband/core/uverbs_main.c: In function ‘ib_uverbs_write’:
drivers/infiniband/core/uverbs_main.c:682:24: attention : assignment discards
‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_main.c:688:22: attention : assignment discards
‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_get_context’:
drivers/infiniband/core/uverbs_cmd.c:307:23: attention : assignment discards
‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_alloc_pd’:
drivers/infiniband/core/uverbs_cmd.c:516:23: attention : assignment discards
‘const’ qualifier from pointer target type [enabled by default]
...
[1] https://lists.01.org/pipermail/kbuild-all/2013-November/002120.html
[2] https://patchwork.kernel.org/patch/2846202/
http://marc.info/?i=3050a98379b4342ea59d59aeaf1ce162171df928.1376847403.git.ydrone...@opteya.com
Link: http://marc.info/?i=cover.1385501822.git.ydrone...@opteya.com
Signed-off-by: Yann Droneaud
---
drivers/infiniband/core/uverbs.h | 12 ++--
drivers/infiniband/core/uverbs_cmd.c | 20 ++--
drivers/infiniband/core/uverbs_main.c | 13 -
3 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 9879568aed8c..0dca1975d59d 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -47,12 +47,12 @@
#include
#include
-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
- do {\
- (udata)->inbuf = (const void __user *) (ibuf); \
- (udata)->outbuf = (void __user *) (obuf);\
- (udata)->inlen = (ilen);\
- (udata)->outlen = (olen);\
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
+ do {\
+ (udata)->inbuf = (ibuf);\
+ (udata)->outbuf = (obuf);\
+ (udata)->inlen = (ilen);\
+ (udata)->outlen = (olen);\
} while (0)
/*
diff --git a/drivers/infiniband/core/uverbs_cmd.c
b/drivers/infiniband/core/uverbs_cmd.c
index 65f6e7dc380c..d9d91c412628 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -305,7 +305,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
}
INIT_UDATA(&udata, buf + sizeof cmd,
- (unsigned long) cmd.response + sizeof resp,
+ (void __user *)(unsigned long)cmd.response + sizeof resp,
The response field is already __u64 and casting to (void __user *)
should match the machine's pointer type size. Why do we have to cast to
(unsigned long) and then cast to (void __user *) ?
in_len - sizeof cmd, out_len - sizeof resp);
ucontext = ibdev->alloc_ucontext(ibdev, &udata);
