* [PATCH rdma-core] verbs: Corrected description of max_sge and max_sge_rd
@ 2017-05-10 16:24 Yishai Hadas
[not found] ` <1494433465-1883-1-git-send-email-yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Yishai Hadas @ 2017-05-10 16:24 UTC (permalink / raw)
To: dledford-H+wXaHxf7aLQT0dZR+AlfA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
yishaih-VPRAkNaXOzVWk0Htik3J/w, parav-VPRAkNaXOzVWk0Htik3J/w,
majd-VPRAkNaXOzVWk0Htik3J/w
From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
This patch corrects ambiguity in documentation of max_sge and max_sge_rd
fields. This ambiguity seems to be created due to [1] and due to mention
of "RD and non RD QPs" in man page.
Here RD can be understood as reliable datagram QP of Infiniband
specification as opposed to "read".
max_sge_rd: Maximum number of sges per WR for RDMA Read operations.
max_sge: Maximum number of sges per WR for QP for non RDMA Read
operations.
[1] http://www.rdmamojo.com/2012/07/13/ibv_query_device/
Signed-off-by: Parav Pandit<parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
Pull request was sent:
https://github.com/linux-rdma/rdma-core/pull/131
libibverbs/man/ibv_query_device.3 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libibverbs/man/ibv_query_device.3 b/libibverbs/man/ibv_query_device.3
index 35daa53..0e9d286 100644
--- a/libibverbs/man/ibv_query_device.3
+++ b/libibverbs/man/ibv_query_device.3
@@ -33,8 +33,8 @@ uint32_t hw_ver; /* Hardware version */
int max_qp; /* Maximum number of supported QPs */
int max_qp_wr; /* Maximum number of outstanding WR on any work queue */
int device_cap_flags; /* HCA capabilities mask */
-int max_sge; /* Maximum number of s/g per WR for non-RD QPs */
-int max_sge_rd; /* Maximum number of s/g per WR for RD QPs */
+int max_sge; /* Maximum number of s/g per WR for SQ & RQ of QP for non RDMA Read operations */
+int max_sge_rd; /* Maximum number of s/g per WR for RDMA Read operations */
int max_cq; /* Maximum number of supported CQs */
int max_cqe; /* Maximum number of CQE capacity per CQ */
int max_mr; /* Maximum number of supported MRs */
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH rdma-core] verbs: Corrected description of max_sge and max_sge_rd
[not found] ` <1494433465-1883-1-git-send-email-yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2017-05-11 8:33 ` Yuval Shaia
2017-05-11 9:44 ` Yishai Hadas
0 siblings, 1 reply; 4+ messages in thread
From: Yuval Shaia @ 2017-05-11 8:33 UTC (permalink / raw)
To: Yishai Hadas
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, parav-VPRAkNaXOzVWk0Htik3J/w,
majd-VPRAkNaXOzVWk0Htik3J/w
On Wed, May 10, 2017 at 07:24:25PM +0300, Yishai Hadas wrote:
> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> This patch corrects ambiguity in documentation of max_sge and max_sge_rd
> fields. This ambiguity seems to be created due to [1] and due to mention
> of "RD and non RD QPs" in man page.
>
> Here RD can be understood as reliable datagram QP of Infiniband
> specification as opposed to "read".
>
> max_sge_rd: Maximum number of sges per WR for RDMA Read operations.
> max_sge: Maximum number of sges per WR for QP for non RDMA Read
> operations.
So the new comments makes it clear but can you consider change the name to
reflect that? i.e. it is good to have comments describing variables role
but better to have the names self explanatory.
Something like the following will make comments almost unneeded.
max_sge_rdma_rd: Maximum number of sges per WR for RDMA Read operations.
max_sge_rd: Maximum number of sges per WR for QP for non RDMA Read
My 2 cents.
>
> [1] http://www.rdmamojo.com/2012/07/13/ibv_query_device/
>
> Signed-off-by: Parav Pandit<parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>
> Pull request was sent:
> https://github.com/linux-rdma/rdma-core/pull/131
>
> libibverbs/man/ibv_query_device.3 | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libibverbs/man/ibv_query_device.3 b/libibverbs/man/ibv_query_device.3
> index 35daa53..0e9d286 100644
> --- a/libibverbs/man/ibv_query_device.3
> +++ b/libibverbs/man/ibv_query_device.3
> @@ -33,8 +33,8 @@ uint32_t hw_ver; /* Hardware version */
> int max_qp; /* Maximum number of supported QPs */
> int max_qp_wr; /* Maximum number of outstanding WR on any work queue */
> int device_cap_flags; /* HCA capabilities mask */
> -int max_sge; /* Maximum number of s/g per WR for non-RD QPs */
> -int max_sge_rd; /* Maximum number of s/g per WR for RD QPs */
> +int max_sge; /* Maximum number of s/g per WR for SQ & RQ of QP for non RDMA Read operations */
Should the line be break here as it exceed 80 characters?
> +int max_sge_rd; /* Maximum number of s/g per WR for RDMA Read operations */
> int max_cq; /* Maximum number of supported CQs */
> int max_cqe; /* Maximum number of CQE capacity per CQ */
> int max_mr; /* Maximum number of supported MRs */
> --
> 1.8.3.1
>
> --
> 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
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH rdma-core] verbs: Corrected description of max_sge and max_sge_rd
2017-05-11 8:33 ` Yuval Shaia
@ 2017-05-11 9:44 ` Yishai Hadas
[not found] ` <dc3fc605-0636-f6d1-5e8c-b4cd10369260-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Yishai Hadas @ 2017-05-11 9:44 UTC (permalink / raw)
To: Yuval Shaia
Cc: Yishai Hadas, dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, parav-VPRAkNaXOzVWk0Htik3J/w,
majd-VPRAkNaXOzVWk0Htik3J/w
On 5/11/2017 11:33 AM, Yuval Shaia wrote:
> On Wed, May 10, 2017 at 07:24:25PM +0300, Yishai Hadas wrote:
>> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> This patch corrects ambiguity in documentation of max_sge and max_sge_rd
>> fields. This ambiguity seems to be created due to [1] and due to mention
>> of "RD and non RD QPs" in man page.
>>
>> Here RD can be understood as reliable datagram QP of Infiniband
>> specification as opposed to "read".
>>
>> max_sge_rd: Maximum number of sges per WR for RDMA Read operations.
>> max_sge: Maximum number of sges per WR for QP for non RDMA Read
>> operations.
>
> So the new comments makes it clear but can you consider change the name to
> reflect that?
Those fields are exposed to user applications and can't be renamed
without breaking compatibility. Please note that the notation of "rd" is
used in other fields as well and are documented correctly (e.g. see man
page for max_qp_rd_atom), so this patch just aligns the man page with
other fields around and fixes the ambiguity.
>> diff --git a/libibverbs/man/ibv_query_device.3 b/libibverbs/man/ibv_query_device.3
>> index 35daa53..0e9d286 100644
>> --- a/libibverbs/man/ibv_query_device.3
>> +++ b/libibverbs/man/ibv_query_device.3
>> @@ -33,8 +33,8 @@ uint32_t hw_ver; /* Hardware version */
>> int max_qp; /* Maximum number of supported QPs */
>> int max_qp_wr; /* Maximum number of outstanding WR on any work queue */
>> int device_cap_flags; /* HCA capabilities mask */
>> -int max_sge; /* Maximum number of s/g per WR for non-RD QPs */
>> -int max_sge_rd; /* Maximum number of s/g per WR for RD QPs */
>> +int max_sge; /* Maximum number of s/g per WR for SQ & RQ of QP for non RDMA Read operations */
>
> Should the line be break here as it exceed 80 characters?
Not really required, this is aligned with having one descriptive line
per field as done for other fields which are even longer, when using
'man' it looks fine.
Thanks for your review.
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH rdma-core] verbs: Corrected description of max_sge and max_sge_rd
[not found] ` <dc3fc605-0636-f6d1-5e8c-b4cd10369260-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
@ 2017-05-11 10:11 ` Yuval Shaia
0 siblings, 0 replies; 4+ messages in thread
From: Yuval Shaia @ 2017-05-11 10:11 UTC (permalink / raw)
To: Yishai Hadas
Cc: Yishai Hadas, dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, parav-VPRAkNaXOzVWk0Htik3J/w,
majd-VPRAkNaXOzVWk0Htik3J/w
On Thu, May 11, 2017 at 12:44:19PM +0300, Yishai Hadas wrote:
> On 5/11/2017 11:33 AM, Yuval Shaia wrote:
> >On Wed, May 10, 2017 at 07:24:25PM +0300, Yishai Hadas wrote:
> >>From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >>
> >>This patch corrects ambiguity in documentation of max_sge and max_sge_rd
> >>fields. This ambiguity seems to be created due to [1] and due to mention
> >>of "RD and non RD QPs" in man page.
> >>
> >>Here RD can be understood as reliable datagram QP of Infiniband
> >>specification as opposed to "read".
> >>
> >>max_sge_rd: Maximum number of sges per WR for RDMA Read operations.
> >>max_sge: Maximum number of sges per WR for QP for non RDMA Read
> >>operations.
> >
> >So the new comments makes it clear but can you consider change the name to
> >reflect that?
>
> Those fields are exposed to user applications and can't be renamed without
> breaking compatibility. Please note that the notation of "rd" is used in
> other fields as well and are documented correctly (e.g. see man page for
> max_qp_rd_atom), so this patch just aligns the man page with other fields
> around and fixes the ambiguity.
Make sense
>
> >>diff --git a/libibverbs/man/ibv_query_device.3 b/libibverbs/man/ibv_query_device.3
> >>index 35daa53..0e9d286 100644
> >>--- a/libibverbs/man/ibv_query_device.3
> >>+++ b/libibverbs/man/ibv_query_device.3
> >>@@ -33,8 +33,8 @@ uint32_t hw_ver; /* Hardware version */
> >> int max_qp; /* Maximum number of supported QPs */
> >> int max_qp_wr; /* Maximum number of outstanding WR on any work queue */
> >> int device_cap_flags; /* HCA capabilities mask */
> >>-int max_sge; /* Maximum number of s/g per WR for non-RD QPs */
> >>-int max_sge_rd; /* Maximum number of s/g per WR for RD QPs */
> >>+int max_sge; /* Maximum number of s/g per WR for SQ & RQ of QP for non RDMA Read operations */
> >
> >Should the line be break here as it exceed 80 characters?
> Not really required, this is aligned with having one descriptive line per
> field as done for other fields which are even longer, when using 'man' it
> looks fine.
>
> Thanks for your review.
Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-11 10:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 16:24 [PATCH rdma-core] verbs: Corrected description of max_sge and max_sge_rd Yishai Hadas
[not found] ` <1494433465-1883-1-git-send-email-yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2017-05-11 8:33 ` Yuval Shaia
2017-05-11 9:44 ` Yishai Hadas
[not found] ` <dc3fc605-0636-f6d1-5e8c-b4cd10369260-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2017-05-11 10:11 ` Yuval Shaia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.