Re: [RFC] IB/sa: Route SA pathrecord query through netlink

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5320 bytes --]

On Thu, 2015-05-21 at 13:21 -0400, Doug Ledford wrote:
> On Thu, 2015-05-21 at 13:52 +0000, Wan, Kaike wrote:
> > In our previous posting to the mailing list, we proposed to send a MAD request from kernel (more
> > specifically, from ib_sa module) to a user space application (ibacm in this case) through netlink.
> > The user space application will send back the response. This simple scheme can achieve the goal 
> > of a local SA cache in user space.
> > 
> > The format of the request and response is diagrammed below:
> > 
> >   ------------------
> >   | netlink header |
> >   ------------------
> >   |     MAD        |
> >   ------------------
> > 
> > The kernel requests for a pathrecord, and the user application finds it in its local cache and sends
> > it to the kernel. If the netlink request fails, the kernel will send the request to SA through the
> > normal IB path (ib_mad -> hca driver -> wire).
> > 
> > Jason pointed out that this message format was limited to lower stack format (MAD) and its use
> > could not be readily extended to upper layer modules like rdma_cm. After lengthy discussions, we 
> > come up with a new and modified scheme, as described below.
> > 
> > The general format of the request and response will be the same:
> > 
> >   ------------------
> >   | netlink header |
> >   ------------------
> >   |  Data header   |
> >   ------------------
> >   |      Data      |
> >   ------------------
> > 
> > The data header contains information about the type of request/response, the status (for response),
> > the type (format) of the data, the total length of the data header + data, and a flags field about
> > the request/response or data.
> > 
> > Based on the type of the data, the data section may be in different format: a string about the host
> > name to resolve, an IP4/IP6 address, a pathrecord, a user pathrecord (struct ib_user_path_rec),
> > or simply a MAD (like our posted patches), etc. Essentially it can be of any format based on the 
> > data type. The key is to document the format so that the kernel and user space can communicate 
> > correctly.
> > 
> > The details are described below:
> > 
> > #define IB_NL_VERSION		0x01
> > 
> > #define IB_NL_OP_MASK		0x0F
> > #define IB_NL_OP_RESOLVE	0x01
> > #define IB_NL_OP_QUERY_PATH	0x02
> > #define IB_NL_OP_SET_TIMEOUT	0x03
> > #define IB_NL_OP_ACK		0x80
> 
> If OP_ACK is one bit, why isn't the OP_MASK 0x7f?
> 
> > #define IB_NL_STATUS_SUCCESS	0x0000
> > #define IB_NL_STATUS_ENODATA	0x0001
> 
> Do we need 16 bits for a bool?  In fact, couldn't this actually be
> switched so that the return of the message uses OP_SUCCESS instead of
> OP_ACK?
> 
> In other words, instead of two items here, couldn't the ACK bit be
> dropped entirely and replaced with SUCCESS so that when the user app
> returns the netlink packet, if the op on return == to the op on send, it
> failed, if it's op | SUCCESS, it succeeded?
> 
> > #define IB_NL_DATA_TYPE_INVALID			0x0000
> > #define IB_NL_DATA_TYPE_NAME			0x0001
> > #define IB_NL_DATA_TYPE_ADDRESS_IP		0x0002
> > #define IB_NL_DATA_TYPE_ADDRESS_IP6		0x0003
> > #define IB_NL_DATA_TYPE_PATH_RECORD		0x0004
> > #define IB_NL_DATA_TYPE_USER_PATH_REC		0x0005
> > #define IB_NL_DATA_TYPE_MAD			0x0006
> > 
> > #define IB_NL_FLAGS_PATH_GMP			1
> > #define IB_NL_FLAGS_PATH_PRIMARY		(1<<1)
> > #define IB_NL_FLAGS_PATH_ALTERNATE		(1<<2)
> > #define IB_NL_FLAGS_PATH_OUTBOUND		(1<<3)
> > #define IB_NL_FLAGS_PATH_INBOUND		(1<<4)
> > #define IB_NL_FLAGS_PATH_INBOUND_REVERSE 	(1<<5)
> > #define IB_NL_FLAGS_PATH_BIDIRECTIONAL		(IB_PATH_OUTBOUND | IB_PATH_INBOUND_REVERSE)
> > #define IB_NL_FLAGS_QUERY_SA			(1<<31)
> > #define IB_NL_FLAGS_NODELAY			(1<<30)
> 
> Please keep these in numerical order, don't put <<31 and below it <<30
> 
> > struct ib_nl_data_hdr {
> > 	__u8	version;
> > 	__u8	opcode;
> > 	__u16	status;
> Drop status because we fold it into opcode
> > 	__u16	type;
> > 	__u16	reserved;
> Drop reserved because we don't need alignment any more
> > 	__u32	flags;
> Flags is the only thing using bits fast, and we would want to make this
> header an even 128bits in length, so add a __u32 reserved; here.  That's
> more likely to be useful than the current layout since we are likely to
> run out of flags before anything else.
> > 	__u32	length;
> > };
> > 
> > struct ib_nl_data {
> > 	struct ib_nl_data_hdr		hdr;
> > 	__u8				data[0];
> > };
> > 
> > 
> > These defines and structures can be added to file include/upai/rdma/rdma_netlink.h (replace with
> > RDMA_NL prefix) or contained in a seperate file (include/upai/rdma/ib_netlink.h ???). 
> > 
> > Please share your thoughts.
> 
> I think an extensible netlink framework here is the right way to go,
> certainly better than the one shot method you had first.

The one thing I left out of the above that might be worth changing is
the fact that you bury your sequence number down in your mad header.  If
there is a generic mechanism that multiple modules can use to send
customized data via nl, then it might be worthwhile to have the sequence
moved to the generic level.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]