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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Fleck, John; Weiny, Ira
Subject: RE: [PATCH v4 1/4] IB/netlink: Add defines for local service requests
through netlink

quoted

quoted

quoted

quoted

+/* 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 0x1000000

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 0x1000000 (or 0x01000000), 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 = 0x01000000, 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=0x01000000, then ibv_path_record->r
 eversible_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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html