verbs: Remove comp_mask checks from verbs.h
PATCH 1/1 verbs: Remove comp_mask checks from verbs.h
In the case of ibv_create_cq_ex this duplicates a check already done by ibv_cmd_create_cq_ex.
In the case of ibv_queryrt_values_ex, there is no check in the function op, which is an ABI mistake. So move the check there.
comp_mask should never be tested in the inline trampolines to generate EINVAL as the inlines are part of the app and could be from a future libibverbs with an expanded comp_mask.
--- libibverbs/verbs.h | 8 -------- providers/mlx4/verbs.c | 3 +++ providers/mlx5/verbs.c | 3 +++ 3 files changed, 6 insertions(+), 8 deletions(-)
I missed these when reviewing the earlier patches, and now people are cargo-cult copying them into new APIs.
libibverbs/verbs.h +0 -8
@@ -1967,11 +1967,6 @@ struct ibv_cq_ex *ibv_create_cq_ex(struct ibv_context *context,return NULL ; } -if (cq_attr->comp_mask & ~(IBV_CQ_INIT_ATTR_MASK_RESERVED -1 )) { - errno =EINVAL ; -return NULL ; - } -return vctx->create_cq_ex (context, cq_attr); }
@@ -2200,9 +2195,6 @@ ibv_query_rt_values_ex(struct ibv_context *context,if (!vctx)return ENOSYS ; -if (values->comp_mask & ~(IBV_VALUES_MASK_RESERVED -1 )) -return EINVAL ; -return vctx->query_rt_values (context, values); }
providers/mlx4/verbs.c +3 -0
@@ -135,6 +135,9 @@ int mlx4_query_rt_values(struct ibv_context *context,uint32_t comp_mask =0 ;int err =0 ; +if (!check_comp_mask (values->comp_mask,IBV_VALUES_MASK_RAW_CLOCK )) +return EINVAL ; +if (values->comp_mask &IBV_VALUES_MASK_RAW_CLOCK ) {uint64_t cycles;
providers/mlx5/verbs.c +3 -0
@@ -108,6 +108,9 @@ int mlx5_query_rt_values(struct ibv_context *context,uint32_t comp_mask =0 ;int err =0 ; +if (!check_comp_mask (values->comp_mask,IBV_VALUES_MASK_RAW_CLOCK )) +return EINVAL ; +if (values->comp_mask &IBV_VALUES_MASK_RAW_CLOCK ) {uint64_t cycles;
Signature · 6 lines
2.15.1 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20171220231350.GA19130-uk2M96/[email protected]
Quoted 3 lines · Jason Gunthorpe
In the case of
ibv_create_cq_exthis duplicates a check already done byibv_cmd_create_cq_ex.
Yes and no.
Yes, the analysis is right and it is should be fixed.
No, the ibv_cmd_create_cq_ex check is performed after we entered into vctx->create_cq_ex(context, cq_attr) callback, which is translated into mlx4_create_cq_ex and mlx5_create_cq_ex.
Both these functions are performing their checks of cq_attr->comp_mask, but return different errno. I think that it is perfect timing to start organize and unite our return errors. They should return the same errno.
mlx4:
470 if (cq_attr->comp_mask & ~CREATE_CQ_SUPPORTED_COMP_MASK) {
471 errno = ENOTSUP;
472 return NULL;
473 }
474
475 if (cq_attr->comp_mask & IBV_CQ_INIT_ATTR_MASK_FLAGS &&
476 cq_attr->flags & ~CREATE_CQ_SUPPORTED_FLAGS) {
477 errno = ENOTSUP;
478 return NULL;
479 }
mlx5:
365 if (cq_attr->comp_mask & ~CREATE_CQ_SUPPORTED_COMP_MASK) {
366 mlx5_dbg(fp, MLX5_DBG_CQ,
367 "Unsupported comp_mask for create_cq\n");
368 errno = EINVAL;
369 return NULL;
370 }
Quoted 2 lines
In the case of
ibv_queryrt_values_ex, there is no check in the function op, which is an ABI mistake. So move the check there.
ibv_queryrt_values_ex -> ibv_query_rt_values_ex
Quoted 76 lines
comp_maskshould never be tested in the inline trampolines to generate EINVAL as the inlines are part of the app and could be from a future libibverbs with an expandedcomp_mask.--- libibverbs/verbs.h | 8 -------- providers/mlx4/verbs.c | 3 +++ providers/mlx5/verbs.c | 3 +++ 3 files changed, 6 insertions(+), 8 deletions(-)I missed these when reviewing the earlier patches, and now people are cargo-cult copying them into new APIs.
libibverbs/verbs.h +0
-8@@ -1967,11 +1967,6 @@ struct ibv_cq_ex *ibv_create_cq_ex(struct ibv_context *context,return NULL ; } -if (cq_attr->comp_mask & ~(IBV_CQ_INIT_ATTR_MASK_RESERVED -1 )) { - errno =EINVAL ; -return NULL ; - } -return vctx->create_cq_ex (context, cq_attr); }@@ -2200,9 +2195,6 @@ ibv_query_rt_values_ex(struct ibv_context *context,if (!vctx)return ENOSYS ; -if (values->comp_mask & ~(IBV_VALUES_MASK_RESERVED -1 )) -return EINVAL ; -return vctx->query_rt_values (context, values); }providers/mlx4/verbs.c +3
-0@@ -135,6 +135,9 @@ int mlx4_query_rt_values(struct ibv_context *context,uint32_t comp_mask =0 ;int err =0 ; +if (!check_comp_mask (values->comp_mask,IBV_VALUES_MASK_RAW_CLOCK )) +return EINVAL ; +if (values->comp_mask &IBV_VALUES_MASK_RAW_CLOCK ) {uint64_t cycles;providers/mlx5/verbs.c +3
-1@@ -108,6 +108,9 @@ int mlx5_query_rt_values(struct ibv_context *context,uint32_t comp_mask =0 ;int err =0 ; +if (!check_comp_mask (values->comp_mask,IBV_VALUES_MASK_RAW_CLOCK )) +return EINVAL ; +if (values->comp_mask &IBV_VALUES_MASK_RAW_CLOCK ) {uint64_t cycles; --2.15.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20171221054216.GF2942-U/[email protected]
Quoted 3 lines · Leon Romanovsky
Both these functions are performing their checks of
cq_attr->comp_mask, but return different errno. I think that it is perfect timing to start organize and unite our return errors. They should return the same errno.
Bugs bugs everywhere.. This is going to be another patch. I would drop the errno set into check_comp_mask and change all comp_mask users to call it, and probably drop the _RESERVED uglyness too, but another patch.
Quoted 4 lines
Quoted 2 lines
In the case of
ibv_queryrt_values_ex, there is no check in the function op, which is an ABI mistake. So move the check there.
ibv_queryrt_values_ex->ibv_query_rt_values_ex
K.. Are you going to collect the loose patches on the list? Can you fix the typo when you grab this one :)
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20171221162442.GC20015-uk2M96/[email protected]
Quoted 11 lines · Jason Gunthorpe
Quoted 3 lines · Leon Romanovsky
Both these functions are performing their checks of
cq_attr->comp_mask, but return different errno. I think that it is perfect timing to start organize and unite our return errors. They should return the same errno.Bugs bugs everywhere.. This is going to be another patch. I would drop the errno set into
check_comp_maskand change allcomp_maskusers to call it, and probably drop the_RESERVEDuglyness too, but another patch.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
In the case of
ibv_queryrt_values_ex, there is no check in the function op, which is an ABI mistake. So move the check there.
ibv_queryrt_values_ex->ibv_query_rt_values_exK.. Are you going to collect the loose patches on the list? Can you fix the typo when you grab this one :)
No problem, I scheduled for myself time on Sunday to sync list, patchworks and rdma-core.
Thanks
Quoted 2 lines
Jason
A message that is not in the archive (sent off-list or not yet received): 20171221171947.GK2942-U/[email protected]
Quoted 15 lines · Leon Romanovsky
Quoted 14 lines · Jason Gunthorpe
Quoted 3 lines · Leon Romanovsky
Both these functions are performing their checks of
cq_attr->comp_mask, but return different errno. I think that it is perfect timing to start organize and unite our return errors. They should return the same errno.Bugs bugs everywhere.. This is going to be another patch. I would drop the errno set into
check_comp_maskand change allcomp_maskusers to call it, and probably drop the_RESERVEDuglyness too, but another patch.Quoted 3 lines
Quoted 2 lines
In the case of
ibv_queryrt_values_ex, there is no check in the function op, which is an ABI mistake. So move the check there.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
ibv_queryrt_values_ex->ibv_query_rt_values_exK.. Are you going to collect the loose patches on the list? Can you fix the typo when you grab this one :)
No problem, I scheduled for myself time on Sunday to sync list, patchworks and rdma-core.
I fixed the patch and changed it to preserve the errno in mlx4 to be EINVAL.
Thanks
Quoted 5 lines
Thanks
Quoted 2 lines
Jason