Re: [PATCH libibverbs V4 4/5] Add ibv_query_port_ex support

[Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 21/5/2014 11:10 PM, Jason Gunthorpe wrote:
> On Sun, May 18, 2014 at 12:38:48PM +0300, Or Gerlitz wrote:
>> +enum ibv_query_port_ex_attr_mask {
>> +	IBV_QUERY_PORT_EX_STATE			= 1 << 0,
>> +	IBV_QUERY_PORT_EX_MAX_MTU		= 1 << 1,
>> +	IBV_QUERY_PORT_EX_ACTIVE_MTU		= 1 << 2,
>> +	IBV_QUERY_PORT_EX_GID_TBL_LEN		= 1 << 3,
>> +	IBV_QUERY_PORT_EX_CAP_FLAGS		= 1 << 4,
>> +	IBV_QUERY_PORT_EX_MAX_MSG_SZ		= 1 << 5,
>> +	IBV_QUERY_PORT_EX_BAD_PKEY_CNTR		= 1 << 6,
>> +	IBV_QUERY_PORT_EX_QKEY_VIOL_CNTR	= 1 << 7,
>> +	IBV_QUERY_PORT_EX_PKEY_TBL_LEN		= 1 << 8,
>> +	IBV_QUERY_PORT_EX_LID			= 1 << 9,
>> +	IBV_QUERY_PORT_EX_SM_LID		= 1 << 10,
>> +	IBV_QUERY_PORT_EX_LMC			= 1 << 11,
>> +	IBV_QUERY_PORT_EX_MAX_VL_NUM		= 1 << 12,
>> +	IBV_QUERY_PORT_EX_SM_SL			= 1 << 13,
>> +	IBV_QUERY_PORT_EX_SUBNET_TIMEOUT	= 1 << 14,
>> +	IBV_QUERY_PORT_EX_INIT_TYPE_REPLY	= 1 << 15,
>> +	IBV_QUERY_PORT_EX_ACTIVE_WIDTH		= 1 << 16,
>> +	IBV_QUERY_PORT_EX_ACTIVE_SPEED		= 1 << 17,
>> +	IBV_QUERY_PORT_EX_PHYS_STATE		= 1 << 18,
>> +	IBV_QUERY_PORT_EX_LINK_LAYER		= 1 << 19,
>> +	/* mask of the fields that exists in the standard query_port_command */
>> +	IBV_QUERY_PORT_EX_STD_MASK		= (1 << 20) - 1,
>> +	/* mask of all supported fields */
>> +	IBV_QUERY_PORT_EX_MASK			= IBV_QUERY_PORT_EX_STD_MASK,
>> +};
>
> OK
>
>> +struct ibv_port_attr_ex {
>> +	enum ibv_port_state	state;
>> +	enum ibv_mtu		max_mtu;
>> +	enum ibv_mtu		active_mtu;
>> +	int			gid_tbl_len;
>> +	uint32_t		port_cap_flags;
>> +	uint32_t		max_msg_sz;
>> +	uint32_t		bad_pkey_cntr;
>> +	uint32_t		qkey_viol_cntr;
>> +	uint16_t		pkey_tbl_len;
>> +	uint16_t		lid;
>> +	uint16_t		sm_lid;
>> +	uint8_t			lmc;
>> +	uint8_t			max_vl_num;
>> +	uint8_t			sm_sl;
>> +	uint8_t			subnet_timeout;
>> +	uint8_t			init_type_reply;
>> +	uint8_t			active_width;
>> +	uint8_t			active_speed;
>> +	uint8_t			phys_state;
>> +	uint8_t			link_layer;
>> +	uint8_t			reserved;
>
> OK
>
>> +	uint32_t		comp_mask;
>
> This uses the first 20 bits already, should comp_mask just be 64 bits
> off the bat?
>

First of all, I think that comp_mask should be the same type for all 
extension verbs and since ibv_flow_attr already uses a 32bit comp_mask, 
so should this verb.
Moreover, I don't think we expect to reach 32 values anytime soon.
In term of future scalability, I understand that the last bit is 
reserved for comp_mask2 field.

>> @@ -998,6 +1050,8 @@ enum verbs_context_mask {
>>
>>   struct verbs_context {
>>   	/*  "grows up" - new fields go here */
>> +	int (*query_port_ex)(struct ibv_context *context, uint8_t port_num,
>> +			     struct ibv_port_attr_ex *port_attr);
>
> OK
>
>> +static inline int ibv_query_port_ex(struct ibv_context *context,
>> +				    uint8_t port_num,
>> +				    struct ibv_port_attr_ex *port_attr)
>> +{
>> +	struct verbs_context *vctx;
>> +
>> +	port_attr->link_layer = IBV_LINK_LAYER_UNSPECIFIED;
>> +	port_attr->reserved   = 0;
>
> We don't need this. All the calls to ibv_query_port already set these
> values and we can simply require that all implementations of
> ibv_query_port_ex set them too.

Correct

>
>> +	if (0 == port_attr->comp_mask)
>> +		return ibv_query_port(context, port_num,
>> +				      (struct ibv_port_attr *)port_attr);
>
> I'm confused what this is doing? Why is 0 ever a valid comp_mask?
>

I'll remove this.

>> +	/* Check that only valid flags were given */
>> +	if (port_attr->comp_mask & ~IBV_QUERY_PORT_EX_MASK) {
>> +		errno = EINVAL;
>> +		return -errno;
>> +	}
>
> And this doesn't seem to make sense either.
>

Sanity check - the user should provide a combination of 
ibv_query_port_ex_attr_mask flags.

>> +	vctx = verbs_get_ctx_op(context, query_port_ex);
>> +
>> +	if (!vctx) {
>> +		/* Fallback to legacy mode */
>> +		if (!(port_attr->comp_mask & ~IBV_QUERY_PORT_EX_STD_MASK))
>> +			return ibv_query_port(context, port_num,
>> +					      (struct ibv_port_attr *)port_attr);
>> +
>> +		/* Unsupported field was requested */
>> +		errno = ENOSYS;
>> +		return -errno;
>> +	}
>> +
>> +	return vctx->query_port_ex(context, port_num, port_attr);
>> +}
>> +
>>   #define ibv_query_port(context, port_num, port_attr) \
>>   	___ibv_query_port(context, port_num, port_attr)
>>
>> diff --git a/man/ibv_query_port_ex.3 b/man/ibv_query_port_ex.3
>> new file mode 100644
>> index 0000000..07073fd
>> +++ b/man/ibv_query_port_ex.3
>> @@ -0,0 +1,97 @@
>> +.\" -*- nroff -*-
>> +.\"
>> +.TH IBV_QUERY_PORT_EX 3 2006-10-31 libibverbs "Libibverbs Programmer's Manual"
>> +.SH "NAME"
>> +ibv_query_port_ex \- query an RDMA port's attributes
>> +.SH "SYNOPSIS"
>> +.nf
>> +.B #include <infiniband/verbs.h>
>> +.sp
>> +.BI "int ibv_query_port_ex(struct ibv_context " "*context" ", uint8_t " "port_num" ,
>> +.BI "                      struct ibv_port_attr_ex " "*port_attr" ");
>> +.fi
>> +.SH "DESCRIPTION"
>> +.B ibv_query_port_ex()
>
> I feel it would be nicer to just patch the existing ibv_query_port man
> page to have the new call and a paragraph about the extra field.
>
> Similar to how 'man accept' works with accept and accept4
>
>

Ok

>> +returns the attributes of port
>> +.I port_num
>> +for device context
>> +.I context
>> +through the pointer
>> +.I port_attr\fR.
>> +The argument
>> +.I port_attr
>> +is an ibv_port_attr struct, as defined in <infiniband/verbs.h>.
>                   ^^^^^^^
>
> No it isn't. Please proof-read everything.
>
> Jason
>

Matan
--
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