RE: [PATCH v4 1/4] IB/netlink: Add defines for local service requests through netlink
> From: Hefty, Sean
> Sent: Wednesday, June 10, 2015 4:32 PM
> To: Hal Rosenstock; Wan, Kaike
> Cc: linux-rdma@vger.kernel.org; Fleck, John; Weiny, Ira
> Subject: RE: [PATCH v4 1/4] IB/netlink: Add defines for local service requests
> through netlink
>
> > >>> +/* Local Service Reversible attribute */ struct
> > >>> +rdma_nla_ls_reversible {
> > >>> + __u32 reversible;
> > >>> +};
> > >>
> > >> Isn't __u8 sufficient for reversible ?
> > > Certainly enough. However, reversible is __u32 in struct
> > ib_user_path_rec and int in struct ib_sa_path_rec.
> >
> > OK; I hadn't double checked there. So it's "inherited" from those
> > reversible definitions.
>
> I don't think we need to adhere to the sizes defined in other structures. I
> agree with Hal's original comment. A __u8 here seems sufficient, unless we
> want to introduce some bit fields.
There are two reasons why we should use __u32 here:
1. Easy to convert from struct ib_sa_path_rec (the input) to netlink attribute:
nla_put(skb, LS_NLA_TYPE_REVERSIBLE,
sizeof(sa_rec->reversible), &sa_rec->reversible);
Instead of:
__u8 tmp;
tmp = (__u8)sa_rec->reversible;
nla_put(skb, , LS_NLA_TYPE_REVERSIBLE, sizeof(tmp), &tmp);
2. Most importantly, sa_rec->reversible is define as type "int", but it really
is "__be32" (big-endian), as proven below:
path rec: len 64 :
00 00 00 00 00 00 00 00 fe 80 00 00 00 00 00 00 00 11 75 00
00 78 ad 92 fe 80 00 00 00 00 00 00 00 11 75 00 00 78 a4 24
00 07 00 03 00 00 00 00 00 80 ff ff 00 00 84 87 92 00 00 00
00 00 00 00
sa path_rec: len 88 :
00 00 00 00 00 00 00 00 fe 80 00 00 00 00 00 00 00 11 75 00
00 78 ad 92 fe 80 00 00 00 00 00 00 00 11 75 00 00 78 a4 24
00 07 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01
00 00 ff ff 00 00 00 02 04 02 07 02 12 00 00 00 00 00 00 00
00 00 00 00 00 00 ff ff
sa path_rec reversible 0x100
The path rec is the pathrecord returned from ibacm in MAD wire format (dumped
in sa_query.c). The byte "0x80" before "ff ff" (the third row) is
reversible_numpath field (reversible bit 7: 1).
The sa_path_rec is the struct ib_sa_path_rec dumped in cma_query_handler() in
cma.c (rdma_cm) when the query callback is invoked. The data "00 00 00 01" (the
last four bytes of the third row) is the field ib_sa_path_rec->reversible
(there are some alignment padding for fields before it). Again the same field
is dumped in user-friendly version as 0x100 (or 0x0100), indicating
that it is in big-endian format.
This test was run on x86_64 machines with Opensm as the SM/SA.
I stumped across this during previous patch tests where ibacm returned struct
ib_user_path_rec as the response. In that test, I had to manually convert
struct ibv_path_record into ib_user_path_rec. Then in ib_sa, I had to convert
from struct ib_user_path_rec to struct ib_sa_path_rec. If I set
ib_user_path_rec->reversible = 1, then the returned result failed rping; If I
set ib_user_path_rec->reversible = 0x0100, then rping successfully
connected to the destination. I also used ib_pack() and ib_unpack() to convert
between struct ib_sa_path_rec and pathrecord in MAD wire format (struct
ibv_path_record) and compared the result with that from SA. If
ib_sa_path_rec->reversible = 1, the ibv_path_record->reversible_numpath=0x00;
if ib_sa_path_rec->reversible=0x0100, then
ibv_path_record->reversible_numpath=0x80.
As a result, I think that the following code is incorrect in
cma_query_ib_route() (cma.c):
path_rec.reversible = 1;
I am surprised that it still works to query SA with IB_SA_PATH_REC_REVERSIBLE
bit set in comp_mask. By the way, of the three callers of ib_sa_path_rec_get(),
this is the only place that ib_sa_path_rec.reversible is set.
That is why we should use __u32 as the attribute data type for reversible.
--
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 v4 1/4] IB/netlink: Add defines for local service requests through netlink
> >>> +/* Local Service Reversible attribute */ struct
> >>> +rdma_nla_ls_reversible {
> >>> + __u32 reversible;
> >>> +};
> >>
> >> Isn't __u8 sufficient for reversible ?
> > Certainly enough. However, reversible is __u32 in struct
> ib_user_path_rec and int in struct ib_sa_path_rec.
>
> OK; I hadn't double checked there. So it's "inherited" from those
> reversible definitions.
I don't think we need to adhere to the sizes defined in other structures. I
agree with Hal's original comment. A __u8 here seems sufficient, unless we
want to introduce some bit fields.
--
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 v4 1/4] IB/netlink: Add defines for local service requests through netlink
On 6/10/2015 2:31 PM, Wan, Kaike wrote:
>> From: Hal Rosenstock [mailto:h...@dev.mellanox.co.il]
>> Sent: Wednesday, June 10, 2015 1:47 PM
>>
>> On 6/9/2015 10:57 AM, kaike@intel.com wrote:
>>> From: Kaike Wan
>>>
>>> This patch adds netlink defines for SA client, local service group,
>>> local service operations, and related attributes.
>>>
>>> Signed-off-by: Kaike Wan
>>> Signed-off-by: John Fleck
>>> Signed-off-by: Ira Weiny
>>> Reviewed-by: Sean Hefty
>>> ---
>>> include/uapi/rdma/rdma_netlink.h | 82
>> ++
>>> 1 files changed, 82 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/uapi/rdma/rdma_netlink.h
>>> b/include/uapi/rdma/rdma_netlink.h
>>> index 6e4bb42..341e9be 100644
>>> --- a/include/uapi/rdma/rdma_netlink.h
>>> +++ b/include/uapi/rdma/rdma_netlink.h
>>> @@ -7,12 +7,14 @@ enum {
>>> RDMA_NL_RDMA_CM = 1,
>>> RDMA_NL_NES,
>>> RDMA_NL_C4IW,
>>> + RDMA_NL_SA,
>>> RDMA_NL_NUM_CLIENTS
>>> };
>>>
>>> enum {
>>> RDMA_NL_GROUP_CM = 1,
>>> RDMA_NL_GROUP_IWPM,
>>> + RDMA_NL_GROUP_LS,
>>> RDMA_NL_NUM_GROUPS
>>> };
>>>
>>> @@ -128,5 +130,85 @@ enum {
>>> IWPM_NLA_ERR_MAX
>>> };
>>>
>>> +/* Local service group opcodes */
>>> +enum {
>>> + RDMA_NL_LS_OP_RESOLVE = 0,
>>> + RDMA_NL_LS_OP_SET_TIMEOUT,
>>> + RDMA_NL_LS_NUM_OPS
>>> +};
>>> +
>>> +/* Local service netlink message flags */
>>> +#define RDMA_NL_LS_F_OK0x0100 /* Success response */
>>> +#define RDMA_NL_LS_F_ERR 0x0200 /* Failed response */
>>> +
>>> +/* Local service attribute type */
>>> +enum {
>>> + LS_NLA_TYPE_STATUS = 0,
>>> + LS_NLA_TYPE_PATH_RECORD,
>>> + LS_NLA_TYPE_TIMEOUT,
>>> + LS_NLA_TYPE_SERVICE_ID,
>>> + LS_NLA_TYPE_DGID,
>>> + LS_NLA_TYPE_SGID,
>>> + LS_NLA_TYPE_TCLASS,
>>> + LS_NLA_TYPE_REVERSIBLE,
>>> + LS_NLA_TYPE_NUM_PATH,
>>
>> Should this be NUMB_PATH rather than NUM_PATH ?
>
> Yes.
>
>>
>>> + LS_NLA_TYPE_PKEY,
>>> + LS_NLA_TYPE_QOS_CLASS,
>>
>> Should this include SL too ?
>
> It will be another attribute.
If SL is to be included, maybe it should be SL mask. Maybe this is an
example of future extensibility...
> All the fields are modeled after struct ib_sa_path_rec and struct
> ib_user_path_rec,
> not after struct ibv_path_record in the user space. In addition, only those
> pathrecord components that are currently
> used by rdma_cm, ipoib, and srp are list here.
Understood.
>>
>>> + LS_NLA_TYPE_MAX
>>> +};
>>> +
>>> +/* Local service status attribute */
>>> +enum {
>>> + LS_NLA_STATUS_SUCCESS = 0,
>>> + LS_NLA_STATUS_EINVAL,
>>> + LS_NLA_STATUS_ENODATA,
>>> + LS_NLA_STATUS_MAX
>>> +};
>>> +
>>> +struct rdma_nla_ls_status {
>>> + __u32 status;
>>> +};
>>> +
>>> +/* Local service pathrecord attribute: struct ib_path_rec_data */
>>> +
>>> +/* Local service timeout attribute */ struct rdma_nla_ls_timeout {
>>> + __u32 timeout;
>>> +};
>>> +
>>> +/* Local Service ServiceID attribute */ struct rdma_nla_ls_service_id
>>> +{
>>> + __be64 service_id;
>>> +};
>>> +
>>> +/* Local Service DGID/SGID attribute: big endian */ struct
>>> +rdma_nla_ls_gid {
>>> + __u8gid[16];
>>> +};
>>> +
>>> +/* Local Service Traffic Class attribute */ struct rdma_nla_ls_tclass
>>> +{
>>> + __u8tclass;
>>> +};
>>> +
>>> +/* Local Service Reversible attribute */ struct
>>> +rdma_nla_ls_reversible {
>>> + __u32 reversible;
>>> +};
>>
>> Isn't __u8 sufficient for reversible ?
> Certainly enough. However, reversible is __u32 in struct ib_user_path_rec and
> int in struct ib_sa_path_rec.
OK; I hadn't double checked there. So it's "inherited" from those
reversible definitions.
>>
>>> +
>>> +/* Local Service numb_path attribute */ struct rdma_nla_ls_numb_path
>>> +{
>>> + __u8numb_path;
>>> +};
>>> +
>>> +/* Local Service Pkey attribute*/
>>> +struct rdma_nla_ls_pkey {
>>> + __be16 pkey;
>>> +};
>>> +
>>> +/* Local Service Qos Class attribute */ struct rdma_nla_ls_qos_class
>>> +{
>>> + __be16 qos_class;
>>> +};
>>>
>>> #endif /* _UAPI_RDMA_NETLINK_H */
>
>
--
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 v4 1/4] IB/netlink: Add defines for local service requests through netlink
> From: Hal Rosenstock [mailto:h...@dev.mellanox.co.il]
> Sent: Wednesday, June 10, 2015 1:47 PM
>
> On 6/9/2015 10:57 AM, kaike@intel.com wrote:
> > From: Kaike Wan
> >
> > This patch adds netlink defines for SA client, local service group,
> > local service operations, and related attributes.
> >
> > Signed-off-by: Kaike Wan
> > Signed-off-by: John Fleck
> > Signed-off-by: Ira Weiny
> > Reviewed-by: Sean Hefty
> > ---
> > include/uapi/rdma/rdma_netlink.h | 82
> ++
> > 1 files changed, 82 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/uapi/rdma/rdma_netlink.h
> > b/include/uapi/rdma/rdma_netlink.h
> > index 6e4bb42..341e9be 100644
> > --- a/include/uapi/rdma/rdma_netlink.h
> > +++ b/include/uapi/rdma/rdma_netlink.h
> > @@ -7,12 +7,14 @@ enum {
> > RDMA_NL_RDMA_CM = 1,
> > RDMA_NL_NES,
> > RDMA_NL_C4IW,
> > + RDMA_NL_SA,
> > RDMA_NL_NUM_CLIENTS
> > };
> >
> > enum {
> > RDMA_NL_GROUP_CM = 1,
> > RDMA_NL_GROUP_IWPM,
> > + RDMA_NL_GROUP_LS,
> > RDMA_NL_NUM_GROUPS
> > };
> >
> > @@ -128,5 +130,85 @@ enum {
> > IWPM_NLA_ERR_MAX
> > };
> >
> > +/* Local service group opcodes */
> > +enum {
> > + RDMA_NL_LS_OP_RESOLVE = 0,
> > + RDMA_NL_LS_OP_SET_TIMEOUT,
> > + RDMA_NL_LS_NUM_OPS
> > +};
> > +
> > +/* Local service netlink message flags */
> > +#define RDMA_NL_LS_F_OK0x0100 /* Success response */
> > +#define RDMA_NL_LS_F_ERR 0x0200 /* Failed response */
> > +
> > +/* Local service attribute type */
> > +enum {
> > + LS_NLA_TYPE_STATUS = 0,
> > + LS_NLA_TYPE_PATH_RECORD,
> > + LS_NLA_TYPE_TIMEOUT,
> > + LS_NLA_TYPE_SERVICE_ID,
> > + LS_NLA_TYPE_DGID,
> > + LS_NLA_TYPE_SGID,
> > + LS_NLA_TYPE_TCLASS,
> > + LS_NLA_TYPE_REVERSIBLE,
> > + LS_NLA_TYPE_NUM_PATH,
>
> Should this be NUMB_PATH rather than NUM_PATH ?
Yes.
>
> > + LS_NLA_TYPE_PKEY,
> > + LS_NLA_TYPE_QOS_CLASS,
>
> Should this include SL too ?
It will be another attribute. All the fields are modeled after struct
ib_sa_path_rec and struct ib_user_path_rec, not after struct ibv_path_record
in the user space. In addition, only those pathrecord components that are
currently used by rdma_cm, ipoib, and srp are list here.
>
> > + LS_NLA_TYPE_MAX
> > +};
> > +
> > +/* Local service status attribute */
> > +enum {
> > + LS_NLA_STATUS_SUCCESS = 0,
> > + LS_NLA_STATUS_EINVAL,
> > + LS_NLA_STATUS_ENODATA,
> > + LS_NLA_STATUS_MAX
> > +};
> > +
> > +struct rdma_nla_ls_status {
> > + __u32 status;
> > +};
> > +
> > +/* Local service pathrecord attribute: struct ib_path_rec_data */
> > +
> > +/* Local service timeout attribute */ struct rdma_nla_ls_timeout {
> > + __u32 timeout;
> > +};
> > +
> > +/* Local Service ServiceID attribute */ struct rdma_nla_ls_service_id
> > +{
> > + __be64 service_id;
> > +};
> > +
> > +/* Local Service DGID/SGID attribute: big endian */ struct
> > +rdma_nla_ls_gid {
> > + __u8gid[16];
> > +};
> > +
> > +/* Local Service Traffic Class attribute */ struct rdma_nla_ls_tclass
> > +{
> > + __u8tclass;
> > +};
> > +
> > +/* Local Service Reversible attribute */ struct
> > +rdma_nla_ls_reversible {
> > + __u32 reversible;
> > +};
>
> Isn't __u8 sufficient for reversible ?
Certainly enough. However, reversible is __u32 in struct ib_user_path_rec and
int in struct ib_sa_path_rec.
>
> > +
> > +/* Local Service numb_path attribute */ struct rdma_nla_ls_numb_path
> > +{
> > + __u8numb_path;
> > +};
> > +
> > +/* Local Service Pkey attribute*/
> > +struct rdma_nla_ls_pkey {
> > + __be16 pkey;
> > +};
> > +
> > +/* Local Service Qos Class attribute */ struct rdma_nla_ls_qos_class
> > +{
> > + __be16 qos_class;
> > +};
> >
> > #endif /* _UAPI_RDMA_NETLINK_H */
--
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 v4 1/4] IB/netlink: Add defines for local service requests through netlink
On 6/9/2015 10:57 AM, kaike@intel.com wrote:
> From: Kaike Wan
>
> This patch adds netlink defines for SA client, local service group, local
> service operations, and related attributes.
>
> Signed-off-by: Kaike Wan
> Signed-off-by: John Fleck
> Signed-off-by: Ira Weiny
> Reviewed-by: Sean Hefty
> ---
> include/uapi/rdma/rdma_netlink.h | 82
> ++
> 1 files changed, 82 insertions(+), 0 deletions(-)
>
> diff --git a/include/uapi/rdma/rdma_netlink.h
> b/include/uapi/rdma/rdma_netlink.h
> index 6e4bb42..341e9be 100644
> --- a/include/uapi/rdma/rdma_netlink.h
> +++ b/include/uapi/rdma/rdma_netlink.h
> @@ -7,12 +7,14 @@ enum {
> RDMA_NL_RDMA_CM = 1,
> RDMA_NL_NES,
> RDMA_NL_C4IW,
> + RDMA_NL_SA,
> RDMA_NL_NUM_CLIENTS
> };
>
> enum {
> RDMA_NL_GROUP_CM = 1,
> RDMA_NL_GROUP_IWPM,
> + RDMA_NL_GROUP_LS,
> RDMA_NL_NUM_GROUPS
> };
>
> @@ -128,5 +130,85 @@ enum {
> IWPM_NLA_ERR_MAX
> };
>
> +/* Local service group opcodes */
> +enum {
> + RDMA_NL_LS_OP_RESOLVE = 0,
> + RDMA_NL_LS_OP_SET_TIMEOUT,
> + RDMA_NL_LS_NUM_OPS
> +};
> +
> +/* Local service netlink message flags */
> +#define RDMA_NL_LS_F_OK 0x0100 /* Success response */
> +#define RDMA_NL_LS_F_ERR 0x0200 /* Failed response */
> +
> +/* Local service attribute type */
> +enum {
> + LS_NLA_TYPE_STATUS = 0,
> + LS_NLA_TYPE_PATH_RECORD,
> + LS_NLA_TYPE_TIMEOUT,
> + LS_NLA_TYPE_SERVICE_ID,
> + LS_NLA_TYPE_DGID,
> + LS_NLA_TYPE_SGID,
> + LS_NLA_TYPE_TCLASS,
> + LS_NLA_TYPE_REVERSIBLE,
> + LS_NLA_TYPE_NUM_PATH,
Should this be NUMB_PATH rather than NUM_PATH ?
> + LS_NLA_TYPE_PKEY,
> + LS_NLA_TYPE_QOS_CLASS,
Should this include SL too ?
> + LS_NLA_TYPE_MAX
> +};
> +
> +/* Local service status attribute */
> +enum {
> + LS_NLA_STATUS_SUCCESS = 0,
> + LS_NLA_STATUS_EINVAL,
> + LS_NLA_STATUS_ENODATA,
> + LS_NLA_STATUS_MAX
> +};
> +
> +struct rdma_nla_ls_status {
> + __u32 status;
> +};
> +
> +/* Local service pathrecord attribute: struct ib_path_rec_data */
> +
> +/* Local service timeout attribute */
> +struct rdma_nla_ls_timeout {
> + __u32 timeout;
> +};
> +
> +/* Local Service ServiceID attribute */
> +struct rdma_nla_ls_service_id {
> + __be64 service_id;
> +};
> +
> +/* Local Service DGID/SGID attribute: big endian */
> +struct rdma_nla_ls_gid {
> + __u8gid[16];
> +};
> +
> +/* Local Service Traffic Class attribute */
> +struct rdma_nla_ls_tclass {
> + __u8tclass;
> +};
> +
> +/* Local Service Reversible attribute */
> +struct rdma_nla_ls_reversible {
> + __u32 reversible;
> +};
Isn't __u8 sufficient for reversible ?
> +
> +/* Local Service numb_path attribute */
> +struct rdma_nla_ls_numb_path {
> + __u8numb_path;
> +};
> +
> +/* Local Service Pkey attribute*/
> +struct rdma_nla_ls_pkey {
> + __be16 pkey;
> +};
> +
> +/* Local Service Qos Class attribute */
> +struct rdma_nla_ls_qos_class {
> + __be16 qos_class;
> +};
>
> #endif /* _UAPI_RDMA_NETLINK_H */
--
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
