Re: [PATCH libibverbs V3 2/3] Use neighbour lookup for RoCE UD QPs Eth L2 resolution

Jason Gunthorpe
2014-05-08T19:29:07Z

On Thu, May 08, 2014 at 09:51:22AM +0300, Or Gerlitz wrote:

> In order to implement RoCE IP based addressing for UD QPs, without
> introducing uverbs changes, we need a way to resolve the L2 Ethernet
> addresses from user-space.

Please put anything that changes the public verbs ABI in a seperate
patch.

Burying the create_ah_ex stuff in this giant thing is not condusive to
reviewing the ABI portion.

The ABI is *important* do everything you can to bring as many eyes as
possible to those changes!!

> +struct ibv_ah_attr_ex {
> + struct ibv_global_route grh;
> + uint16_t dlid;
> + uint8_t sl;
> + uint8_t src_path_bits;
> + uint8_t static_rate;
> + uint8_t is_global;
> + uint8_t port_num;
> + uint32_t comp_mask;
> + struct {
> + enum ll_address_type type;
> + uint32_t len;
> + char *address;
> + } ll_address;
> + uint16_t vid;
> +};

The above looks sorta reasonable, but:

What is the 'char *address'? Should it be 'const void *' ?????

Who owns that memory? Who allocates it, who frees it? Seems sketchy
as heck.

What happens if a ibv_ah_attr_ex is returned in some kind of
ibv_qp_attr_ex structure from a future query_qp?

Can you just put in a 'uint64_t address[8];' or sockaddr_storage, or
something?

> +
> enum ibv_srq_attr_mask {
> IBV_SRQ_MAX_WR = 1 << 0,
> IBV_SRQ_LIMIT = 1 << 1
> @@ -968,11 +998,14 @@ enum verbs_context_mask {
> VERBS_CONTEXT_QP = 1 << 2,
> VERBS_CONTEXT_CREATE_FLOW = 1 << 3,
> VERBS_CONTEXT_DESTROY_FLOW = 1 << 4,
> - VERBS_CONTEXT_RESERVED = 1 << 5
> + VERBS_CONTEXT_CREATE_AH = 1 << 5,
> + VERBS_CONTEXT_RESERVED = 1 << 6
> };

What is going on here??

verbs_context_mask is messed up already!?

It is NOT supposed to be a mask if the function is available - we
can already test for NULL for that.

It is supposed to indicate if structures *allocated by the provider*
are extended.

For instance if VERBS_CONTEXT_QP is set, then everyone knows that the
'struct ibv_qp' returned by *any* driver function (crate_qp, open_qp,
create_qp_ex) is convertible to 'verbs_qp'.

VERBS_CONTEXT_CREATE_FLOW/VERBS_CONTEXT_DESTROY_FLOW are screwed
up. They shouldn't even exist, there is no unextended ibv_flow
structure to worry about.

Fix this patch to not add VERBS_CONTEXT_CREATE_AH, ibv_ah_attr_ex is
not a provider allocated structure so it doesn't need an entry.

Please send a patch to remove the screwed up two above and replace
with RESERVED.

And BE MORE CAREFUL. THIS SHOULD NOT HAVE HAPPENED!

Jason