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

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

[David Ahern]

On 2/27/18 9:07 AM, Steve Wise wrote:
> diff --git a/rdma/rdma.h b/rdma/rdma.h
> index 5809f70..e55205b 100644
> --- a/rdma/rdma.h
> +++ 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 my repo.

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

[Steve Wise]

On 3/26/2018 9:17 AM, David Ahern wrote:
> On 2/27/18 9:07 AM, Steve Wise wrote:
>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>> index 5809f70..e55205b 100644
>> --- a/rdma/rdma.h
>> +++ 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 my repo.

It is provided by the rdma-core package, upon which rdma tool now
depends for the rdma_port_space enum.

Steve.

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

[David Ahern]

On 3/26/18 8:30 AM, Steve Wise wrote:
> 
> 
> On 3/26/2018 9:17 AM, David Ahern wrote:
>> On 2/27/18 9:07 AM, Steve Wise wrote:
>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>> index 5809f70..e55205b 100644
>>> --- a/rdma/rdma.h
>>> +++ 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 my repo.
> 
> It is provided by the rdma-core package, upon which rdma tool now
> depends for the rdma_port_space enum.
> 

You need to add a check for the package, and only build rdma if that
package is installed. See check_mnl in configure for an example.

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

[Steve Wise]

On 3/26/2018 9:44 AM, David Ahern wrote:
> On 3/26/18 8:30 AM, Steve Wise wrote:
>>
>> On 3/26/2018 9:17 AM, David Ahern wrote:
>>> On 2/27/18 9:07 AM, Steve Wise wrote:
>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>>> index 5809f70..e55205b 100644
>>>> --- a/rdma/rdma.h
>>>> +++ 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 my repo.
>> It is provided by the rdma-core package, upon which rdma tool now
>> depends for the rdma_port_space enum.
>>
> You need to add a check for the package, and only build rdma if that
> package is installed. See check_mnl in configure for an example.

Ok, that makes sense.

Steve.

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

[Leon Romanovsky]

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

On Mon, Mar 26, 2018 at 09:55:46AM -0500, Steve Wise wrote:
>
>
> On 3/26/2018 9:44 AM, David Ahern wrote:
> > On 3/26/18 8:30 AM, Steve Wise wrote:
> >>
> >> On 3/26/2018 9:17 AM, David Ahern wrote:
> >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> >>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >>>> index 5809f70..e55205b 100644
> >>>> --- a/rdma/rdma.h
> >>>> +++ 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 my repo.
> >> It is provided by the rdma-core package, upon which rdma tool now
> >> depends for the rdma_port_space enum.
> >>
> > You need to add a check for the package, and only build rdma if that
> > package is installed. See check_mnl in configure for an example.
>
> Ok, that makes sense.

IMHO, better solution will be to copy those files to iproute2.

Thanks

>
> Steve.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Steve Wise]

On 3/26/2018 10:08 AM, Leon Romanovsky wrote:
> On Mon, Mar 26, 2018 at 09:55:46AM -0500, Steve Wise wrote:
>>
>> On 3/26/2018 9:44 AM, David Ahern wrote:
>>> On 3/26/18 8:30 AM, Steve Wise wrote:
>>>> On 3/26/2018 9:17 AM, David Ahern wrote:
>>>>> On 2/27/18 9:07 AM, Steve Wise wrote:
>>>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>>>>> index 5809f70..e55205b 100644
>>>>>> --- a/rdma/rdma.h
>>>>>> +++ 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 my repo.
>>>> It is provided by the rdma-core package, upon which rdma tool now
>>>> depends for the rdma_port_space enum.
>>>>
>>> You need to add a check for the package, and only build rdma if that
>>> package is installed. See check_mnl in configure for an example.
>> Ok, that makes sense.
> IMHO, better solution will be to copy those files to iproute2.

Hey Leon,

Why is it better in your opinion?  My gut tells me adding rdma_cma.h to
iproute2 means more uabi type syncing.

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

[Leon Romanovsky]

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

On Mon, Mar 26, 2018 at 10:24:25AM -0500, Steve Wise wrote:
>
>
> On 3/26/2018 10:08 AM, Leon Romanovsky wrote:
> > On Mon, Mar 26, 2018 at 09:55:46AM -0500, Steve Wise wrote:
> >>
> >> On 3/26/2018 9:44 AM, David Ahern wrote:
> >>> On 3/26/18 8:30 AM, Steve Wise wrote:
> >>>> On 3/26/2018 9:17 AM, David Ahern wrote:
> >>>>> On 2/27/18 9:07 AM, Steve Wise wrote:
> >>>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >>>>>> index 5809f70..e55205b 100644
> >>>>>> --- a/rdma/rdma.h
> >>>>>> +++ 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 my repo.
> >>>> It is provided by the rdma-core package, upon which rdma tool now
> >>>> depends for the rdma_port_space enum.
> >>>>
> >>> You need to add a check for the package, and only build rdma if that
> >>> package is installed. See check_mnl in configure for an example.
> >> Ok, that makes sense.
> > IMHO, better solution will be to copy those files to iproute2.
>
> Hey Leon,
>
> Why is it better in your opinion?  My gut tells me adding rdma_cma.h to
> iproute2 means more uabi type syncing.

Making rdmatool be dependant on rdma-core will require that distributions
will update their specs to install rdma-core as a dependency for every
iprotue2 install.

The rdma-core dependency makes sense for RDMA users, but doesn't for most of
the iproute2 users.

Thanks

>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Steve Wise]

On 3/26/2018 12:06 PM, Leon Romanovsky wrote:
> On Mon, Mar 26, 2018 at 10:24:25AM -0500, Steve Wise wrote:
>>
>> On 3/26/2018 10:08 AM, Leon Romanovsky wrote:
>>> On Mon, Mar 26, 2018 at 09:55:46AM -0500, Steve Wise wrote:
>>>> On 3/26/2018 9:44 AM, David Ahern wrote:
>>>>> On 3/26/18 8:30 AM, Steve Wise wrote:
>>>>>> On 3/26/2018 9:17 AM, David Ahern wrote:
>>>>>>> On 2/27/18 9:07 AM, Steve Wise wrote:
>>>>>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>>>>>>> index 5809f70..e55205b 100644
>>>>>>>> --- a/rdma/rdma.h
>>>>>>>> +++ 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 my repo.
>>>>>> It is provided by the rdma-core package, upon which rdma tool now
>>>>>> depends for the rdma_port_space enum.
>>>>>>
>>>>> You need to add a check for the package, and only build rdma if that
>>>>> package is installed. See check_mnl in configure for an example.
>>>> Ok, that makes sense.
>>> IMHO, better solution will be to copy those files to iproute2.
>> Hey Leon,
>>
>> Why is it better in your opinion?  My gut tells me adding rdma_cma.h to
>> iproute2 means more uabi type syncing.
> Making rdmatool be dependant on rdma-core will require that distributions
> will update their specs to install rdma-core as a dependency for every
> iprotue2 install.
>
> The rdma-core dependency makes sense for RDMA users, but doesn't for most of
> the iproute2 users.

I'm fuzzy on the details of distro packaging, but David's suggestion is
that rdmatool wouldn't get built if rdma-core isn't present. But
everything else would.  Just like it does not get built if libmnl is not
installed.  For pre-built rpms, the rdma-core would have to be present. 

I'm ok pulling it in, I'm just trying to understand. :)

Steve.

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

[Leon Romanovsky]

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

On Mon, Mar 26, 2018 at 12:13:53PM -0500, Steve Wise wrote:
>
>
> On 3/26/2018 12:06 PM, Leon Romanovsky wrote:
> > On Mon, Mar 26, 2018 at 10:24:25AM -0500, Steve Wise wrote:
> >>
> >> On 3/26/2018 10:08 AM, Leon Romanovsky wrote:
> >>> On Mon, Mar 26, 2018 at 09:55:46AM -0500, Steve Wise wrote:
> >>>> On 3/26/2018 9:44 AM, David Ahern wrote:
> >>>>> On 3/26/18 8:30 AM, Steve Wise wrote:
> >>>>>> On 3/26/2018 9:17 AM, David Ahern wrote:
> >>>>>>> On 2/27/18 9:07 AM, Steve Wise wrote:
> >>>>>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >>>>>>>> index 5809f70..e55205b 100644
> >>>>>>>> --- a/rdma/rdma.h
> >>>>>>>> +++ 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 my repo.
> >>>>>> It is provided by the rdma-core package, upon which rdma tool now
> >>>>>> depends for the rdma_port_space enum.
> >>>>>>
> >>>>> You need to add a check for the package, and only build rdma if that
> >>>>> package is installed. See check_mnl in configure for an example.
> >>>> Ok, that makes sense.
> >>> IMHO, better solution will be to copy those files to iproute2.
> >> Hey Leon,
> >>
> >> Why is it better in your opinion?  My gut tells me adding rdma_cma.h to
> >> iproute2 means more uabi type syncing.
> > Making rdmatool be dependant on rdma-core will require that distributions
> > will update their specs to install rdma-core as a dependency for every
> > iprotue2 install.
> >
> > The rdma-core dependency makes sense for RDMA users, but doesn't for most of
> > the iproute2 users.
>
> I'm fuzzy on the details of distro packaging, but David's suggestion is
> that rdmatool wouldn't get built if rdma-core isn't present. But
> everything else would.  Just like it does not get built if libmnl is not
> installed.  For pre-built rpms, the rdma-core would have to be present. 
>
> I'm ok pulling it in, I'm just trying to understand. :)
>

Distros supply pre-built packages, for example Fedora's RPM:
https://rpmfind.net/linux/RPM/fedora/27/x86_64/i/iproute-4.12.0-3.fc27.x86_64.html
It requires that libmnl will be installed. Once rdmatool will need
rdma-core, it will pulled in too.

BTW, don't forget to change header's guards (ifdef/defne ..), see
rdma_netlink.h as an example.


> Steve.
>
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jason Gunthorpe]

On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> 
> 
> On 3/26/2018 9:17 AM, David Ahern wrote:
> > On 2/27/18 9:07 AM, Steve Wise wrote:
> >> 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 my repo.
> 
> It is provided by the rdma-core package, upon which rdma tool now
> depends for the rdma_port_space enum.

It is a kernel bug that enum is not in an include/uapi/rdma header

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

Jason

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

[Steve Wise]

On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
>>
>> On 3/26/2018 9:17 AM, David Ahern wrote:
>>> On 2/27/18 9:07 AM, Steve Wise wrote:
>>>> 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 my repo.
>> It is provided by the rdma-core package, upon which rdma tool now
>> depends for the rdma_port_space enum.
> It is a kernel bug that enum is not in an include/uapi/rdma header
>
> Fix it there and don't try to use rdma-core headers to get kernel ABI.
>
> Jason

I wish you'd commented on this just a little sooner.  I just resent v3
of this series... with rdma_cma.h included. :)

How about the restrack/nldev code just translates the port space from
enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
i add to rdma_netlink.h?  I'd hate to open the can of worms of trying to
split rdma_cma.h into uabi and no uabi headers. :(

Steve.

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

[Jason Gunthorpe]

On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> 
> On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> >>
> >> On 3/26/2018 9:17 AM, David Ahern wrote:
> >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> >>>> 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 my repo.
> >> It is provided by the rdma-core package, upon which rdma tool now
> >> depends for the rdma_port_space enum.
> > It is a kernel bug that enum is not in an include/uapi/rdma header
> >
> > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> >
> > Jason
> 
> I wish you'd commented on this just a little sooner.  I just resent v3
> of this series... with rdma_cma.h included. :)
> 
> How about the restrack/nldev code just translates the port space from
> enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> i add to rdma_netlink.h?  I'd hate to open the can of worms of trying to
> 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
translate it.

You just need to pick the right header to put it in, since it is a verbs
define it doesn't belong in the netlink header.

Jason

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

[Leon Romanovsky]

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

On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> >
> > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > >>
> > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > >>>> 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 my repo.
> > >> It is provided by the rdma-core package, upon which rdma tool now
> > >> depends for the rdma_port_space enum.
> > > It is a kernel bug that enum is not in an include/uapi/rdma header
> > >
> > > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> > >
> > > Jason
> >
> > I wish you'd commented on this just a little sooner.  I just resent v3
> > of this series... with rdma_cma.h included. :)
> >
> > How about the restrack/nldev code just translates the port space from
> > enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> > i add to rdma_netlink.h?  I'd hate to open the can of worms of trying to
> > 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
> translate it.
>
> You just need to pick the right header to put it in, since it is a verbs
> 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 of
rdmatool with attempt to clean IB/core as well to ensure that we won't expose
anything that is not implemented. It didn't go well.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jason Gunthorpe]

On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > >
> > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > >>
> > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > >>>> 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 my repo.
> > > >> It is provided by the rdma-core package, upon which rdma tool now
> > > >> depends for the rdma_port_space enum.
> > > > It is a kernel bug that enum is not in an include/uapi/rdma header
> > > >
> > > > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> > > >
> > > > Jason
> > >
> > > I wish you'd commented on this just a little sooner.  I just resent v3
> > > of this series... with rdma_cma.h included. :)
> > >
> > > How about the restrack/nldev code just translates the port space from
> > > enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> > > i add to rdma_netlink.h?  I'd hate to open the can of worms of trying to
> > > 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
> > translate it.
> >
> > You just need to pick the right header to put it in, since it is a verbs
> > 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 of
> rdmatool with attempt to clean IB/core as well to ensure that we won't expose
> anything that is not implemented. It didn't go well.

rdma-core is now using the kernel uapi/ headers natively, seems to be
going OK. What problem did you face?

Jason

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

[Leon Romanovsky]

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

On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > >
> > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > >>
> > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > >>>> 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 my repo.
> > > > >> It is provided by the rdma-core package, upon which rdma tool now
> > > > >> depends for the rdma_port_space enum.
> > > > > It is a kernel bug that enum is not in an include/uapi/rdma header
> > > > >
> > > > > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> > > > >
> > > > > Jason
> > > >
> > > > I wish you'd commented on this just a little sooner.  I just resent v3
> > > > of this series... with rdma_cma.h included. :)
> > > >
> > > > How about the restrack/nldev code just translates the port space from
> > > > enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> > > > i add to rdma_netlink.h?  I'd hate to open the can of worms of trying to
> > > > 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
> > > translate it.
> > >
> > > You just need to pick the right header to put it in, since it is a verbs
> > > 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 of
> > rdmatool with attempt to clean IB/core as well to ensure that we won't expose
> > anything that is not implemented. It didn't go well.
>
> rdma-core is now using the kernel uapi/ headers natively, seems to be
> going OK. What problem did you face?

I didn't agree to move to UAPI defines which are not implemented and not
used in the kernel, so I sent small number of patches similar to those [1, 2].

Those patches were rejected.

So please don't mix Steve's need to use 3 defines with very large and
painful task to expose proper UAPIs.

Thanks

[1] https://patchwork.kernel.org/patch/9901319/
[2] https://patchwork.kernel.org/patch/9901317/

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jason Gunthorpe]

On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > >
> > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > > >>
> > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > >>>> 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 my repo.
> > > > > >> It is provided by the rdma-core package, upon which rdma tool now
> > > > > >> depends for the rdma_port_space enum.
> > > > > > It is a kernel bug that enum is not in an include/uapi/rdma header
> > > > > >
> > > > > > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> > > > > >
> > > > > > Jason
> > > > >
> > > > > I wish you'd commented on this just a little sooner.  I just resent v3
> > > > > of this series... with rdma_cma.h included. :)
> > > > >
> > > > > How about the restrack/nldev code just translates the port space from
> > > > > enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> > > > > i add to rdma_netlink.h?  I'd hate to open the can of worms of trying to
> > > > > 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
> > > > translate it.
> > > >
> > > > You just need to pick the right header to put it in, since it is a verbs
> > > > 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 of
> > > rdmatool with attempt to clean IB/core as well to ensure that we won't expose
> > > anything that is not implemented. It didn't go well.
> >
> > rdma-core is now using the kernel uapi/ headers natively, seems to be
> > going OK. What problem did you face?
> 
> I didn't agree to move to UAPI defines which are not implemented and not
> used in the kernel, so I sent small number of patches similar to those [1, 2].
> 
> Those patches were rejected.
> 
> So please don't mix Steve's need to use 3 defines with very large and
> 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.

Jason

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

[Steve Wise]

> 
> On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> > On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > > >
> > > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > > > >>
> > > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > > >>>> 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 my
> repo.
> > > > > > >> It is provided by the rdma-core package, upon which rdma tool
> now
> > > > > > >> depends for the rdma_port_space enum.
> > > > > > > It is a kernel bug that enum is not in an include/uapi/rdma
> header
> > > > > > >
> > > > > > > Fix it there and don't try to use rdma-core headers to get kernel
> ABI.
> > > > > > >
> > > > > > > Jason
> > > > > >
> > > > > > I wish you'd commented on this just a little sooner.  I just resent
> v3
> > > > > > of this series... with rdma_cma.h included. :)
> > > > > >
> > > > > > How about the restrack/nldev code just translates the port space
> from
> > > > > > enum rdma_port_space to a new ABI enum, say
> nldev_rdma_port_space, that
> > > > > > i add to rdma_netlink.h?  I'd hate to open the can of worms of
> trying to
> > > > > > 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
> > > > > translate it.
> > > > >
> > > > > You just need to pick the right header to put it in, since it is a verbs
> > > > > 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 of
> > > > rdmatool with attempt to clean IB/core as well to ensure that we
> won't expose
> > > > anything that is not implemented. It didn't go well.
> > >
> > > rdma-core is now using the kernel uapi/ headers natively, seems to be
> > > going OK. What problem did you face?
> >
> > I didn't agree to move to UAPI defines which are not implemented and
> not
> > used in the kernel, so I sent small number of patches similar to those [1,
> 2].
> >
> > Those patches were rejected.
> >
> > So please don't mix Steve's need to use 3 defines with very large and
> > 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?  

Will that work for everyone?

Steve.

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

[Leon Romanovsky]

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

On Tue, Mar 27, 2018 at 10:45:30AM -0500, Steve Wise wrote:
>
> >
> > On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> > > On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > > > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky wrote:
> > > > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe wrote:
> > > > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > > > >
> > > > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> > > > > > > >>
> > > > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > > > >>>> 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 my
> > repo.
> > > > > > > >> It is provided by the rdma-core package, upon which rdma tool
> > now
> > > > > > > >> depends for the rdma_port_space enum.
> > > > > > > > It is a kernel bug that enum is not in an include/uapi/rdma
> > header
> > > > > > > >
> > > > > > > > Fix it there and don't try to use rdma-core headers to get kernel
> > ABI.
> > > > > > > >
> > > > > > > > Jason
> > > > > > >
> > > > > > > I wish you'd commented on this just a little sooner.  I just resent
> > v3
> > > > > > > of this series... with rdma_cma.h included. :)
> > > > > > >
> > > > > > > How about the restrack/nldev code just translates the port space
> > from
> > > > > > > enum rdma_port_space to a new ABI enum, say
> > nldev_rdma_port_space, that
> > > > > > > i add to rdma_netlink.h?  I'd hate to open the can of worms of
> > trying to
> > > > > > > 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
> > > > > > translate it.
> > > > > >
> > > > > > You just need to pick the right header to put it in, since it is a verbs
> > > > > > 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 of
> > > > > rdmatool with attempt to clean IB/core as well to ensure that we
> > won't expose
> > > > > anything that is not implemented. It didn't go well.
> > > >
> > > > rdma-core is now using the kernel uapi/ headers natively, seems to be
> > > > going OK. What problem did you face?
> > >
> > > I didn't agree to move to UAPI defines which are not implemented and
> > not
> > > used in the kernel, so I sent small number of patches similar to those [1,
> > 2].
> > >
> > > Those patches were rejected.
> > >
> > > So please don't mix Steve's need to use 3 defines with very large and
> > > 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?
>
> 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

>
> Steve.
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Steve Wise]

> On Tue, Mar 27, 2018 at 10:45:30AM -0500, Steve Wise wrote:
> >
> > >
> > > On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > > > > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky
> wrote:
> > > > > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe
> wrote:
> > > > > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > > > > >
> > > > > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise
> wrote:
> > > > > > > > >>
> > > > > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > > > > >>>> 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
> my
> > > repo.
> > > > > > > > >> It is provided by the rdma-core package, upon which rdma
> tool
> > > now
> > > > > > > > >> depends for the rdma_port_space enum.
> > > > > > > > > It is a kernel bug that enum is not in an
include/uapi/rdma
> > > header
> > > > > > > > >
> > > > > > > > > Fix it there and don't try to use rdma-core headers to get
> kernel
> > > ABI.
> > > > > > > > >
> > > > > > > > > Jason
> > > > > > > >
> > > > > > > > I wish you'd commented on this just a little sooner.  I just
> resent
> > > v3
> > > > > > > > of this series... with rdma_cma.h included. :)
> > > > > > > >
> > > > > > > > How about the restrack/nldev code just translates the port
> space
> > > from
> > > > > > > > enum rdma_port_space to a new ABI enum, say
> > > nldev_rdma_port_space, that
> > > > > > > > i add to rdma_netlink.h?  I'd hate to open the can of worms
of
> > > trying to
> > > > > > > > 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
> > > > > > > translate it.
> > > > > > >
> > > > > > > You just need to pick the right header to put it in, since it
is a
> verbs
> > > > > > > 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
> of
> > > > > > rdmatool with attempt to clean IB/core as well to ensure that we
> > > won't expose
> > > > > > anything that is not implemented. It didn't go well.
> > > > >
> > > > > rdma-core is now using the kernel uapi/ headers natively, seems to
> be
> > > > > going OK. What problem did you face?
> > > >
> > > > I didn't agree to move to UAPI defines which are not implemented and
> > > not
> > > > used in the kernel, so I sent small number of patches similar to
those
> [1,
> > > 2].
> > > >
> > > > Those patches were rejected.
> > > >
> > > > So please don't mix Steve's need to use 3 defines with very large
and
> > > > 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?
> >
> > 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...

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

[Leon Romanovsky]

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

On Tue, Mar 27, 2018 at 11:20:25AM -0500, Steve Wise wrote:
> > On Tue, Mar 27, 2018 at 10:45:30AM -0500, Steve Wise wrote:
> > >
> > > >
> > > > On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> > > > > On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe wrote:
> > > > > > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky
> > wrote:
> > > > > > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe
> > wrote:
> > > > > > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
> > > > > > > > >
> > > > > > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise
> > wrote:
> > > > > > > > > >>
> > > > > > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > > > > > >>>> 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
> > my
> > > > repo.
> > > > > > > > > >> It is provided by the rdma-core package, upon which rdma
> > tool
> > > > now
> > > > > > > > > >> depends for the rdma_port_space enum.
> > > > > > > > > > It is a kernel bug that enum is not in an
> include/uapi/rdma
> > > > header
> > > > > > > > > >
> > > > > > > > > > Fix it there and don't try to use rdma-core headers to get
> > kernel
> > > > ABI.
> > > > > > > > > >
> > > > > > > > > > Jason
> > > > > > > > >
> > > > > > > > > I wish you'd commented on this just a little sooner.  I just
> > resent
> > > > v3
> > > > > > > > > of this series... with rdma_cma.h included. :)
> > > > > > > > >
> > > > > > > > > How about the restrack/nldev code just translates the port
> > space
> > > > from
> > > > > > > > > enum rdma_port_space to a new ABI enum, say
> > > > nldev_rdma_port_space, that
> > > > > > > > > i add to rdma_netlink.h?  I'd hate to open the can of worms
> of
> > > > trying to
> > > > > > > > > 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
> > > > > > > > translate it.
> > > > > > > >
> > > > > > > > You just need to pick the right header to put it in, since it
> is a
> > verbs
> > > > > > > > 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
> > of
> > > > > > > rdmatool with attempt to clean IB/core as well to ensure that we
> > > > won't expose
> > > > > > > anything that is not implemented. It didn't go well.
> > > > > >
> > > > > > rdma-core is now using the kernel uapi/ headers natively, seems to
> > be
> > > > > > going OK. What problem did you face?
> > > > >
> > > > > I didn't agree to move to UAPI defines which are not implemented and
> > > > not
> > > > > used in the kernel, so I sent small number of patches similar to
> those
> > [1,
> > > > 2].
> > > > >
> > > > > Those patches were rejected.
> > > > >
> > > > > So please don't mix Steve's need to use 3 defines with very large
> and
> > > > > 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?
> > >
> > > 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

>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Steve Wise]

> 
> On Tue, Mar 27, 2018 at 11:20:25AM -0500, Steve Wise wrote:
> > > On Tue, Mar 27, 2018 at 10:45:30AM -0500, Steve Wise wrote:
> > > >
> > > > >
> > > > > On Tue, Mar 27, 2018 at 06:15:44PM +0300, Leon Romanovsky wrote:
> > > > > > On Tue, Mar 27, 2018 at 08:44:55AM -0600, Jason Gunthorpe
> wrote:
> > > > > > > On Tue, Mar 27, 2018 at 06:21:41AM +0300, Leon Romanovsky
> > > wrote:
> > > > > > > > On Mon, Mar 26, 2018 at 04:30:33PM -0600, Jason Gunthorpe
> > > wrote:
> > > > > > > > > On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise
> wrote:
> > > > > > > > > >
> > > > > > > > > > On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > > > > > > > > > > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise
> > > wrote:
> > > > > > > > > > >>
> > > > > > > > > > >> On 3/26/2018 9:17 AM, David Ahern wrote:
> > > > > > > > > > >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> > > > > > > > > > >>>> 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
> > > my
> > > > > repo.
> > > > > > > > > > >> It is provided by the rdma-core package, upon which
> rdma
> > > tool
> > > > > now
> > > > > > > > > > >> depends for the rdma_port_space enum.
> > > > > > > > > > > It is a kernel bug that enum is not in an
> > include/uapi/rdma
> > > > > header
> > > > > > > > > > >
> > > > > > > > > > > Fix it there and don't try to use rdma-core headers to
get
> > > kernel
> > > > > ABI.
> > > > > > > > > > >
> > > > > > > > > > > Jason
> > > > > > > > > >
> > > > > > > > > > I wish you'd commented on this just a little sooner.  I
just
> > > resent
> > > > > v3
> > > > > > > > > > of this series... with rdma_cma.h included. :)
> > > > > > > > > >
> > > > > > > > > > How about the restrack/nldev code just translates the
port
> > > space
> > > > > from
> > > > > > > > > > enum rdma_port_space to a new ABI enum, say
> > > > > nldev_rdma_port_space, that
> > > > > > > > > > i add to rdma_netlink.h?  I'd hate to open the can of
worms
> > of
> > > > > trying to
> > > > > > > > > > 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
> > > > > > > > > translate it.
> > > > > > > > >
> > > > > > > > > You just need to pick the right header to put it in, since
it
> > is a
> > > verbs
> > > > > > > > > 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
> > > of
> > > > > > > > rdmatool with attempt to clean IB/core as well to ensure
that
> we
> > > > > won't expose
> > > > > > > > anything that is not implemented. It didn't go well.
> > > > > > >
> > > > > > > rdma-core is now using the kernel uapi/ headers natively,
seems
> to
> > > be
> > > > > > > going OK. What problem did you face?
> > > > > >
> > > > > > I didn't agree to move to UAPI defines which are not implemented
> and
> > > > > not
> > > > > > used in the kernel, so I sent small number of patches similar to
> > those
> > > [1,
> > > > > 2].
> > > > > >
> > > > > > Those patches were rejected.
> > > > > >
> > > > > > So please don't mix Steve's need to use 3 defines with very
large
> > and
> > > > > > 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?
> > > >
> > > > 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

Right.  Will do.

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

[David Ahern]

On 2/27/18 9:07 AM, Steve Wise wrote:
> Sample output:
> 
> # rdma resource
> 2: cxgb4_0: pd 5 cq 2 qp 2 cm_id 3 mr 7
> 3: mlx4_0: pd 7 cq 3 qp 3 cm_id 3 mr 7
> 
> # rdma resource show cm_id
> link cxgb4_0/- lqpn 0 qp-type RC state LISTEN ps TCP pid 30485 comm rping src-addr 0.0.0.0:7174
> link cxgb4_0/2 lqpn 1048 qp-type RC state CONNECT ps TCP pid 30503 comm rping src-addr 172.16.2.1:7174 dst-addr 172.16.2.1:38246
> link cxgb4_0/2 lqpn 1040 qp-type RC state CONNECT ps TCP pid 30498 comm rping src-addr 172.16.2.1:38246 dst-addr 172.16.2.1:7174
> link mlx4_0/- lqpn 0 qp-type RC state LISTEN ps TCP pid 30485 comm rping src-addr 0.0.0.0:7174
> link mlx4_0/1 lqpn 539 qp-type RC state CONNECT ps TCP pid 30494 comm rping src-addr 172.16.99.1:7174 dst-addr 172.16.99.1:43670
> link mlx4_0/1 lqpn 538 qp-type RC state CONNECT ps TCP pid 30492 comm rping src-addr 172.16.99.1:43670 dst-addr 172.16.99.1:7174
> 
> # rdma resource show cm_id dst-port 7174
> link cxgb4_0/2 lqpn 1040 qp-type RC state CONNECT ps TCP pid 30498 comm rping src-addr 172.16.2.1:38246 dst-addr 172.16.2.1:7174
> link mlx4_0/1 lqpn 538 qp-type RC state CONNECT ps TCP pid 30492 comm rping src-addr 172.16.99.1:43670 dst-addr 172.16.99.1:7174
> 
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/rdma.h  |   2 +
>  rdma/res.c   | 258 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  rdma/utils.c |   5 ++
>  3 files changed, 264 insertions(+), 1 deletion(-)
> 
> diff --git a/rdma/rdma.h b/rdma/rdma.h
> index 5809f70..e55205b 100644
> --- a/rdma/rdma.h
> +++ 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>
>  
>  #define pr_err(args...) fprintf(stderr, ##args)
>  #define pr_out(args...) fprintf(stdout, ##args)
> diff --git a/rdma/res.c b/rdma/res.c
> index 62f5c54..1ef4f20 100644
> --- a/rdma/res.c
> +++ b/rdma/res.c
> @@ -16,9 +16,11 @@ static int res_help(struct rd *rd)
>  {
>  	pr_out("Usage: %s resource\n", rd->filename);
>  	pr_out("          resource show [DEV]\n");
> -	pr_out("          resource show [qp]\n");
> +	pr_out("          resource show [qp|cm_id]\n");
>  	pr_out("          resource show qp link [DEV/PORT]\n");
>  	pr_out("          resource show qp link [DEV/PORT] [FILTER-NAME FILTER-VALUE]\n");
> +	pr_out("          resource show cm_id link [DEV/PORT]\n");
> +	pr_out("          resource show cm_id link [DEV/PORT] [FILTER-NAME FILTER-VALUE]\n");
>  	return 0;
>  }
>  
> @@ -433,6 +435,230 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data)
>  	return MNL_CB_OK;
>  }
>  
> +static void print_qp_type(struct rd *rd, uint32_t val)
> +{
> +	if (rd->json_output)
> +		jsonw_string_field(rd->jw, "qp-type",
> +				   qp_types_to_str(val));
> +	else
> +		pr_out("qp-type %s ", qp_types_to_str(val));
> +}
> +
> +static const char *cm_id_state_to_str(uint8_t idx)
> +{
> +	static const char * const cm_id_states_str[] = { "IDLE", "ADDR_QUERY",
> +						      "ADDR_RESOLVED", "ROUTE_QUERY", "ROUTE_RESOLVED",
> +						      "CONNECT", "DISCONNECT",
> +						      "ADDR_BOUND", "LISTEN", "DEVICE_REMOVAL", "DESTROYING" };
> +

In general lines should stay under 80 columns. There are a few
exceptions to the rule (e.g., print strings), but most of the long lines
you have in this patch need to conform.

> @@ -457,11 +683,41 @@ filters qp_valid_filters[MAX_NUMBER_OF_FILTERS] = {{ .name = "link",
>  
>  RES_FUNC(res_qp,	RDMA_NLDEV_CMD_RES_QP_GET, qp_valid_filters, false);
>  
> +static const struct
> +filters cm_id_valid_filters[MAX_NUMBER_OF_FILTERS] = {{ .name = "link",
> +						   .is_number = false },
> +						   { .name = "lqpn",
> +						   .is_number = true },
> +						   { .name = "qp-type",
> +						   .is_number = false },
> +						   { .name = "state",
> +						   .is_number = false },
> +						   { .name = "ps",
> +						   .is_number = false },
> +						   { .name = "dev-type",
> +						   .is_number = false },
> +						   { .name = "transport-type",
> +						   .is_number = false },
> +						   { .name = "pid",
> +						   .is_number = true },
> +						   { .name = "src-addr",
> +						   .is_number = false },
> +						   { .name = "src-port",
> +						   .is_number = true },
> +						   { .name = "dst-addr",
> +						   .is_number = false },
> +						   { .name = "dst-port",
> +						   .is_number = true }};
> +

The above would be more readable as
	static const
	struct filters cm_id_valid_filters[MAX_NUMBER_OF_FILTERS] = {
		{ .name = "link", .is_number = false },
		{ .name = "lqpn", .is_number = true },
...

Definitely do not split between struct and filters.

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

[Steve Wise]

On 3/26/2018 10:40 AM, David Ahern wrote:
> On 2/27/18 9:07 AM, Steve Wise wrote:
>> Sample output:
>>
>> # rdma resource
>> 2: cxgb4_0: pd 5 cq 2 qp 2 cm_id 3 mr 7
>> 3: mlx4_0: pd 7 cq 3 qp 3 cm_id 3 mr 7
>>
>> # rdma resource show cm_id
>> link cxgb4_0/- lqpn 0 qp-type RC state LISTEN ps TCP pid 30485 comm rping src-addr 0.0.0.0:7174
>> link cxgb4_0/2 lqpn 1048 qp-type RC state CONNECT ps TCP pid 30503 comm rping src-addr 172.16.2.1:7174 dst-addr 172.16.2.1:38246
>> link cxgb4_0/2 lqpn 1040 qp-type RC state CONNECT ps TCP pid 30498 comm rping src-addr 172.16.2.1:38246 dst-addr 172.16.2.1:7174
>> link mlx4_0/- lqpn 0 qp-type RC state LISTEN ps TCP pid 30485 comm rping src-addr 0.0.0.0:7174
>> link mlx4_0/1 lqpn 539 qp-type RC state CONNECT ps TCP pid 30494 comm rping src-addr 172.16.99.1:7174 dst-addr 172.16.99.1:43670
>> link mlx4_0/1 lqpn 538 qp-type RC state CONNECT ps TCP pid 30492 comm rping src-addr 172.16.99.1:43670 dst-addr 172.16.99.1:7174
>>
>> # rdma resource show cm_id dst-port 7174
>> link cxgb4_0/2 lqpn 1040 qp-type RC state CONNECT ps TCP pid 30498 comm rping src-addr 172.16.2.1:38246 dst-addr 172.16.2.1:7174
>> link mlx4_0/1 lqpn 538 qp-type RC state CONNECT ps TCP pid 30492 comm rping src-addr 172.16.99.1:43670 dst-addr 172.16.99.1:7174
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/rdma.h  |   2 +
>>  rdma/res.c   | 258 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  rdma/utils.c |   5 ++
>>  3 files changed, 264 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>> index 5809f70..e55205b 100644
>> --- a/rdma/rdma.h
>> +++ 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>
>>  
>>  #define pr_err(args...) fprintf(stderr, ##args)
>>  #define pr_out(args...) fprintf(stdout, ##args)
>> diff --git a/rdma/res.c b/rdma/res.c
>> index 62f5c54..1ef4f20 100644
>> --- a/rdma/res.c
>> +++ b/rdma/res.c
>> @@ -16,9 +16,11 @@ static int res_help(struct rd *rd)
>>  {
>>  	pr_out("Usage: %s resource\n", rd->filename);
>>  	pr_out("          resource show [DEV]\n");
>> -	pr_out("          resource show [qp]\n");
>> +	pr_out("          resource show [qp|cm_id]\n");
>>  	pr_out("          resource show qp link [DEV/PORT]\n");
>>  	pr_out("          resource show qp link [DEV/PORT] [FILTER-NAME FILTER-VALUE]\n");
>> +	pr_out("          resource show cm_id link [DEV/PORT]\n");
>> +	pr_out("          resource show cm_id link [DEV/PORT] [FILTER-NAME FILTER-VALUE]\n");
>>  	return 0;
>>  }
>>  
>> @@ -433,6 +435,230 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  	return MNL_CB_OK;
>>  }
>>  
>> +static void print_qp_type(struct rd *rd, uint32_t val)
>> +{
>> +	if (rd->json_output)
>> +		jsonw_string_field(rd->jw, "qp-type",
>> +				   qp_types_to_str(val));
>> +	else
>> +		pr_out("qp-type %s ", qp_types_to_str(val));
>> +}
>> +
>> +static const char *cm_id_state_to_str(uint8_t idx)
>> +{
>> +	static const char * const cm_id_states_str[] = { "IDLE", "ADDR_QUERY",
>> +						      "ADDR_RESOLVED", "ROUTE_QUERY", "ROUTE_RESOLVED",
>> +						      "CONNECT", "DISCONNECT",
>> +						      "ADDR_BOUND", "LISTEN", "DEVICE_REMOVAL", "DESTROYING" };
>> +
> In general lines should stay under 80 columns. There are a few
> exceptions to the rule (e.g., print strings), but most of the long lines
> you have in this patch need to conform.

Ok. Will do.

>> @@ -457,11 +683,41 @@ filters qp_valid_filters[MAX_NUMBER_OF_FILTERS] = {{ .name = "link",
>>  
>>  RES_FUNC(res_qp,	RDMA_NLDEV_CMD_RES_QP_GET, qp_valid_filters, false);
>>  
>> +static const struct
>> +filters cm_id_valid_filters[MAX_NUMBER_OF_FILTERS] = {{ .name = "link",
>> +						   .is_number = false },
>> +						   { .name = "lqpn",
>> +						   .is_number = true },
>> +						   { .name = "qp-type",
>> +						   .is_number = false },
>> +						   { .name = "state",
>> +						   .is_number = false },
>> +						   { .name = "ps",
>> +						   .is_number = false },
>> +						   { .name = "dev-type",
>> +						   .is_number = false },
>> +						   { .name = "transport-type",
>> +						   .is_number = false },
>> +						   { .name = "pid",
>> +						   .is_number = true },
>> +						   { .name = "src-addr",
>> +						   .is_number = false },
>> +						   { .name = "src-port",
>> +						   .is_number = true },
>> +						   { .name = "dst-addr",
>> +						   .is_number = false },
>> +						   { .name = "dst-port",
>> +						   .is_number = true }};
>> +
> The above would be more readable as
> 	static const
> 	struct filters cm_id_valid_filters[MAX_NUMBER_OF_FILTERS] = {
> 		{ .name = "link", .is_number = false },
> 		{ .name = "lqpn", .is_number = true },
> ...
>
> Definitely do not split between struct and filters.

Your formatting is easier to read/maintain.

Thanks for the comments!