Re: [PATCH v2 iproute2-next 3/6] rdma: Add CM_ID resource tracking information

On Tue, Mar 27, 2018 at 11:20:25AM -0500, Steve Wise wrote:

quoted

On Tue, Mar 27, 2018 at 10:45:30AM -0500, Steve Wise wrote:

quoted

quoted

On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:

quoted

On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:

quoted

On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky

wrote:

quoted

quoted

quoted

quoted

quoted

On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:

quoted

On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:

quoted

On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On 3/26/2018 9:17 AM, David Ahern wrote:

quoted

On 2/27/18 9:07 AM, Steve Wise wrote:

quoted

diff --git a/rdma/rdma.h b/rdma/rdma.h
index 5809f70..e55205b 100644
+++ b/rdma/rdma.h

@@ -18,10 +18,12 @@
 #include <libmnl/libmnl.h>
 #include <rdma/rdma_netlink.h>
 #include <time.h>
+#include <net/if_arp.h>

 #include "list.h"
 #include "utils.h"
 #include "json_writer.h"
+#include <rdma/rdma_cma.h>

did you forget to add rdma_cma.h? I don't see that file

in

quoted

my

quoted

quoted

repo.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

It is provided by the rdma-core package, upon which rdma

tool

quoted

quoted

now

quoted

quoted

quoted

quoted

quoted

quoted

quoted

depends for the rdma_port_space enum.

It is a kernel bug that enum is not in an

include/uapi/rdma

quoted

quoted

quoted

header

quoted

quoted

quoted

quoted

quoted

quoted

Fix it there and don't try to use rdma-core headers to get

kernel

quoted

quoted

ABI.

quoted

quoted

quoted

quoted

quoted

quoted

Jason

I wish you'd commented on this just a little sooner.  I just

resent

quoted

quoted

v3

quoted

quoted

quoted

quoted

quoted

of this series... with rdma_cma.h included. :)

How about the restrack/nldev code just translates the port

space

quoted

quoted

from

quoted

quoted

quoted

quoted

quoted

enum rdma_port_space to a new ABI enum, say

nldev_rdma_port_space, that

quoted

quoted

quoted

quoted

quoted

i add to rdma_netlink.h?  I'd hate to open the can of worms

of

quoted

quoted

quoted

trying to

quoted

quoted

quoted

quoted

quoted

split rdma_cma.h into uabi and no uabi headers. :(

If port space is already part of the ABI there isn't much

reason to

quoted

quoted

quoted

quoted

quoted

quoted

quoted

translate it.

You just need to pick the right header to put it in, since it

is a

quoted

verbs

quoted

quoted

quoted

quoted

quoted

quoted

define it doesn't belong in the netlink header.

I completely understand Steve's concerns.

I tried to do such thing (expose kernel headers) in first

incarnation

quoted

of

quoted

quoted

quoted

quoted

quoted

rdmatool with attempt to clean IB/core as well to ensure that we

won't expose

quoted

quoted

quoted

anything that is not implemented. It didn't go well.

rdma-core is now using the kernel uapi/ headers natively, seems to

be

quoted

quoted

quoted

quoted

going OK. What problem did you face?

I didn't agree to move to UAPI defines which are not implemented and

not

quoted

used in the kernel, so I sent small number of patches similar to

those

quoted

[1,

quoted

quoted

2].

quoted

Those patches were rejected.

So please don't mix Steve's need to use 3 defines with very large

and

quoted

quoted

quoted

quoted

painful task to expose proper UAPIs.

Steve can just move the 3 defines he needs to the uapi, we are doing
this incrementally..

rdma_core does not define kernel ABI and it is totally wrong to use
random constants from rdma_cma.h as kernel ABI.

Proposal:

Since the cm_id port space is part of the rdma_ucm_create_id struct in

include/uapi/rdma/rdma_user_cm.h, I'll move the rdma_port_space enum
there.  And then my iproute2 series will have to add a copy of
rdma_user_cm.h locally into rdma/include/uapi/rdma, right?

quoted

Will that work for everyone?

You need to remove _PS from that structure and from the kernel with
justification that it is safe to do.

Thanks

I'm pretty sure port space is needed.  That struct is used to create a user
mode cm_id...

Sorry, it is RDMA_PS_SDP.

Thanks