diff mbox series

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

Message ID 743dc7a5306f9b3368fcd4c143cdd822250444a6.1520020530.git.swise@opengridcomputing.com
State Changes Requested, archived
Delegated to: David Ahern
Headers show
Series cm_id, cq, mr, and pd resource tracking | expand

Commit Message

Steve Wise Feb. 27, 2018, 4:07 p.m. UTC
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(-)

Comments

Leon Romanovsky March 13, 2018, 6:07 p.m. UTC | #1
On Tue, Feb 27, 2018 at 08:07:05AM -0800, 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(-)
>

Steve, can you please add man pages update as a followup to this series?

Especially the fact that you are providing dst-port filter option should
be mentioned.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
Steve Wise March 13, 2018, 7:44 p.m. UTC | #2
> On Tue, Feb 27, 2018 at 08:07:05AM -0800, 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(-)
> >
> 
> Steve, can you please add man pages update as a followup to this series?
> 
> Especially the fact that you are providing dst-port filter option should
> be mentioned.
> 
> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

Will do.
David Ahern March 26, 2018, 2:17 p.m. UTC | #3
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.
Steve Wise March 26, 2018, 2:30 p.m. UTC | #4
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.
David Ahern March 26, 2018, 2:44 p.m. UTC | #5
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.
Steve Wise March 26, 2018, 2:55 p.m. UTC | #6
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.
Leon Romanovsky March 26, 2018, 3:08 p.m. UTC | #7
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.
Steve Wise March 26, 2018, 3:24 p.m. UTC | #8
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.
David Ahern March 26, 2018, 3:40 p.m. UTC | #9
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.
Leon Romanovsky March 26, 2018, 5:06 p.m. UTC | #10
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

>
>
Steve Wise March 26, 2018, 5:13 p.m. UTC | #11
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.
Leon Romanovsky March 26, 2018, 5:27 p.m. UTC | #12
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.
>
>
>
>
Steve Wise March 26, 2018, 7:55 p.m. UTC | #13
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!
Jason Gunthorpe March 26, 2018, 9:15 p.m. UTC | #14
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
Steve Wise March 26, 2018, 9:34 p.m. UTC | #15
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.
Jason Gunthorpe March 26, 2018, 10:30 p.m. UTC | #16
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
Leon Romanovsky March 27, 2018, 3:21 a.m. UTC | #17
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
Jason Gunthorpe March 27, 2018, 2:44 p.m. UTC | #18
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
Leon Romanovsky March 27, 2018, 3:15 p.m. UTC | #19
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
Jason Gunthorpe March 27, 2018, 3:23 p.m. UTC | #20
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
Steve Wise March 27, 2018, 3:45 p.m. UTC | #21
> 
> 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.
Leon Romanovsky March 27, 2018, 4:01 p.m. UTC | #22
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.
>
Steve Wise March 27, 2018, 4:20 p.m. UTC | #23
> 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...
Leon Romanovsky March 27, 2018, 4:30 p.m. UTC | #24
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

>
>
>
Steve Wise March 27, 2018, 4:38 p.m. UTC | #25
> 
> 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.
diff mbox series

Patch

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" };
+
+	if (idx < ARRAY_SIZE(cm_id_states_str))
+		return cm_id_states_str[idx];
+	return "UNKNOWN";
+}
+
+static const char *cm_id_ps_to_str(uint32_t ps)
+{
+	switch (ps) {
+	case RDMA_PS_IPOIB:
+		return "IPoIB";
+	case RDMA_PS_IB:
+		return "IPoIB";
+	case RDMA_PS_TCP:
+		return "TCP";
+	case RDMA_PS_UDP:
+		return "UDP";
+	default:
+		return "---";
+	}
+}
+
+static void print_cm_id_state(struct rd *rd, uint8_t state)
+{
+	if (rd->json_output) {
+		jsonw_string_field(rd->jw, "state", cm_id_state_to_str(state));
+		return;
+	}
+	pr_out("state %s ", cm_id_state_to_str(state));
+}
+
+static void print_ps(struct rd *rd, uint32_t ps)
+{
+	if (rd->json_output) {
+		jsonw_string_field(rd->jw, "ps", cm_id_ps_to_str(ps));
+		return;
+	}
+	pr_out("ps %s ", cm_id_ps_to_str(ps));
+}
+
+static void print_ipaddr(struct rd *rd, const char *key, char *addrstr, uint16_t port)
+{
+	if (rd->json_output) {
+		int name_size = INET6_ADDRSTRLEN+strlen(":65535");
+		char json_name[name_size];
+
+		snprintf(json_name, name_size, "%s:%u", addrstr, port);
+		jsonw_string_field(rd->jw, key, json_name);
+		return;
+	}
+	pr_out("%s %s:%u ", key, addrstr, port);
+}
+
+static int ss_ntop(struct nlattr *nla_line, char *addr_str, uint16_t *port)
+{
+	struct __kernel_sockaddr_storage *addr;
+
+	addr = (struct __kernel_sockaddr_storage *)mnl_attr_get_payload(nla_line);
+	switch (addr->ss_family) {
+	case AF_INET: {
+		struct sockaddr_in *sin = (struct sockaddr_in *)addr;
+
+		if (!inet_ntop(AF_INET, (const void *)&sin->sin_addr, addr_str, INET6_ADDRSTRLEN))
+			return -EINVAL;
+		*port = ntohs(sin->sin_port);
+		break;
+	}
+	case AF_INET6: {
+		struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
+
+		if (!inet_ntop(AF_INET6, (const void *)&sin6->sin6_addr, addr_str, INET6_ADDRSTRLEN))
+			return -EINVAL;
+		*port = ntohs(sin6->sin6_port);
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX] = {};
+	struct nlattr *nla_table, *nla_entry;
+	struct rd *rd = data;
+	const char *name;
+	int idx;
+
+	mnl_attr_parse(nlh, 0, rd_attr_cb, tb);
+	if (!tb[RDMA_NLDEV_ATTR_DEV_INDEX] ||
+	    !tb[RDMA_NLDEV_ATTR_DEV_NAME] ||
+	    !tb[RDMA_NLDEV_ATTR_RES_CM_ID])
+		return MNL_CB_ERROR;
+
+	name = mnl_attr_get_str(tb[RDMA_NLDEV_ATTR_DEV_NAME]);
+	idx =  mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]);
+	nla_table = tb[RDMA_NLDEV_ATTR_RES_CM_ID];
+	mnl_attr_for_each_nested(nla_entry, nla_table) {
+		struct nlattr *nla_line[RDMA_NLDEV_ATTR_MAX] = {};
+		char src_addr_str[INET6_ADDRSTRLEN];
+		char dst_addr_str[INET6_ADDRSTRLEN];
+		uint16_t src_port, dst_port;
+		uint32_t port = 0, pid = 0;
+		uint8_t type = 0, state;
+		uint32_t lqpn = 0, ps;
+		char *comm = NULL;
+		int err;
+
+		err = mnl_attr_parse_nested(nla_entry, rd_attr_cb, nla_line);
+		if (err != MNL_CB_OK)
+			return -EINVAL;
+
+		if (!nla_line[RDMA_NLDEV_ATTR_RES_STATE] ||
+		    !nla_line[RDMA_NLDEV_ATTR_RES_PS] ||
+		    (!nla_line[RDMA_NLDEV_ATTR_RES_PID] &&
+		     !nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])) {
+			return MNL_CB_ERROR;
+		}
+
+		if (nla_line[RDMA_NLDEV_ATTR_PORT_INDEX])
+			port = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_PORT_INDEX]);
+
+		if (port && port != rd->port_idx)
+			continue;
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_LQPN]) {
+			lqpn = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_LQPN]);
+			if (rd_check_is_filtered(rd, "lqpn", lqpn))
+				continue;
+		}
+		if (nla_line[RDMA_NLDEV_ATTR_RES_TYPE]) {
+			type = mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_TYPE]);
+			if (rd_check_is_string_filtered(rd, "qp-type", qp_types_to_str(type)))
+				continue;
+		}
+
+		ps = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PS]);
+		if (rd_check_is_string_filtered(rd, "ps", cm_id_ps_to_str(ps)))
+			continue;
+
+		state = mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_STATE]);
+		if (rd_check_is_string_filtered(rd, "state", cm_id_state_to_str(state)))
+			continue;
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_SRC_ADDR]) {
+			if (ss_ntop(nla_line[RDMA_NLDEV_ATTR_RES_SRC_ADDR], src_addr_str, &src_port))
+				continue;
+			if (rd_check_is_string_filtered(rd, "src-addr", src_addr_str))
+				continue;
+		}
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_DST_ADDR]) {
+			if (ss_ntop(nla_line[RDMA_NLDEV_ATTR_RES_DST_ADDR], dst_addr_str, &dst_port))
+				continue;
+			if (rd_check_is_string_filtered(rd, "dst-addr", dst_addr_str))
+				continue;
+		}
+
+		if (rd_check_is_filtered(rd, "src-port", src_port))
+			continue;
+
+		if (rd_check_is_filtered(rd, "dst-port", dst_port))
+			continue;
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
+			pid = mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
+			comm = get_task_name(pid);
+		}
+
+		if (rd_check_is_filtered(rd, "pid", pid)) {
+			free(comm);
+			continue;
+		}
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
+			/* discard const from mnl_attr_get_str */
+			comm = (char *)mnl_attr_get_str(nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
+		}
+
+		if (rd->json_output)
+			jsonw_start_array(rd->jw);
+
+		print_link(rd, idx, name, port, nla_line);
+		if (nla_line[RDMA_NLDEV_ATTR_RES_LQPN])
+			print_lqpn(rd, lqpn);
+		if (nla_line[RDMA_NLDEV_ATTR_RES_TYPE])
+			print_qp_type(rd, type);
+		print_cm_id_state(rd, state);
+		print_ps(rd, ps);
+		print_pid(rd, pid);
+		print_comm(rd, comm, nla_line);
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_SRC_ADDR])
+			print_ipaddr(rd, "src-addr", src_addr_str, src_port);
+		if (nla_line[RDMA_NLDEV_ATTR_RES_DST_ADDR])
+			print_ipaddr(rd, "dst-addr", dst_addr_str, dst_port);
+
+		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
+			free(comm);
+
+		if (rd->json_output)
+			jsonw_end_array(rd->jw);
+		else
+			pr_out("\n");
+	}
+	return MNL_CB_OK;
+}
+
 RES_FUNC(res_no_args,	RDMA_NLDEV_CMD_RES_GET,	NULL, true);
 
 static const struct
@@ -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 }};
+
+RES_FUNC(res_cm_id,	RDMA_NLDEV_CMD_RES_CM_ID_GET, cm_id_valid_filters,
+	 false);
+
 static int res_show(struct rd *rd)
 {
 	const struct rd_cmd cmds[] = {
 		{ NULL,		res_no_args	},
 		{ "qp",		res_qp		},
+		{ "cm_id",	res_cm_id	},
 		{ 0 }
 	};
 
diff --git a/rdma/utils.c b/rdma/utils.c
index f946016..ec81737 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -375,6 +375,11 @@  static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
 	[RDMA_NLDEV_ATTR_RES_STATE]		= MNL_TYPE_U8,
 	[RDMA_NLDEV_ATTR_RES_PID]		= MNL_TYPE_U32,
 	[RDMA_NLDEV_ATTR_RES_KERN_NAME]	= MNL_TYPE_NUL_STRING,
+	[RDMA_NLDEV_ATTR_RES_CM_ID]		= MNL_TYPE_NESTED,
+	[RDMA_NLDEV_ATTR_RES_CM_ID_ENTRY]	= MNL_TYPE_NESTED,
+	[RDMA_NLDEV_ATTR_RES_PS]		= MNL_TYPE_U32,
+	[RDMA_NLDEV_ATTR_RES_SRC_ADDR]		= MNL_TYPE_UNSPEC,
+	[RDMA_NLDEV_ATTR_RES_DST_ADDR]		= MNL_TYPE_UNSPEC,
 };
 
 int rd_attr_cb(const struct nlattr *attr, void *data)