Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Wed, Jun 28, 2017 at 10:11:12AM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 27, 2017 at 03:18:59PM -0700, Stephen Hemminger wrote:
> > On Tue, 27 Jun 2017 20:46:15 +0300
> > Leon Romanovsky  wrote:
> >
> > > On Tue, Jun 27, 2017 at 11:37:35AM -0600, Jason Gunthorpe wrote:
> > > > On Tue, Jun 27, 2017 at 08:33:01PM +0300, Leon Romanovsky wrote:
> > > >
> > > > > My initial plan was to put all parsers under their respective names, 
> > > > > in
> > > > > the similar way as I did for caps: $ rdma dev show mlx5_4 caps
> > > >
> > > > I think you should have a useful summary display similar to 'ip a' and
> > > > other commands.
> > > >
> > > > guid(s), subnet prefix or default gid for IB, lid/lmc, link state,
> > > > speed, mtu, pkeys protocol(s)
> > >
> > > It will, but before I would like to see this tool be a part of
> > > iproute2, so other people will be able to extend it in addition
> > > to me.
> > >
> > > Are you fine with the proposed code?
> > >
> >
> > Output formats need to be nailed down. The output of iproute2 commands is 
> > almost
> > like an ABI. Users build scripts to parse it (whether that is a great idea 
> > or not
> > is debateable, it mostly shows the weakness in programatic API's). 
> > Therefore fully
> > changing output formats in later revisions is likely to get users upset.
>
> It would be nice to see an example of what the completed command
> should output to make judgements on the format.. Going bit by bit
> doesn't really give a full picture, IHO.

Bit by bit expansion allow easily control of what is needed. Mostly,
those full examples have nothing close to real use case.

>
> Jason


signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Jason Gunthorpe]

On Tue, Jun 27, 2017 at 03:18:59PM -0700, Stephen Hemminger wrote:
> On Tue, 27 Jun 2017 20:46:15 +0300
> Leon Romanovsky  wrote:
> 
> > On Tue, Jun 27, 2017 at 11:37:35AM -0600, Jason Gunthorpe wrote:
> > > On Tue, Jun 27, 2017 at 08:33:01PM +0300, Leon Romanovsky wrote:
> > >  
> > > > My initial plan was to put all parsers under their respective names, in
> > > > the similar way as I did for caps: $ rdma dev show mlx5_4 caps  
> > >
> > > I think you should have a useful summary display similar to 'ip a' and
> > > other commands.
> > >
> > > guid(s), subnet prefix or default gid for IB, lid/lmc, link state,
> > > speed, mtu, pkeys protocol(s)  
> > 
> > It will, but before I would like to see this tool be a part of
> > iproute2, so other people will be able to extend it in addition
> > to me.
> > 
> > Are you fine with the proposed code?
> > 
> 
> Output formats need to be nailed down. The output of iproute2 commands is 
> almost
> like an ABI. Users build scripts to parse it (whether that is a great idea or 
> not
> is debateable, it mostly shows the weakness in programatic API's). Therefore 
> fully
> changing output formats in later revisions is likely to get users upset.

It would be nice to see an example of what the completed command
should output to make judgements on the format.. Going bit by bit
doesn't really give a full picture, IHO.

Jason

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Tue, Jun 27, 2017 at 03:18:59PM -0700, Stephen Hemminger wrote:
> On Tue, 27 Jun 2017 20:46:15 +0300
> Leon Romanovsky  wrote:
>
> > On Tue, Jun 27, 2017 at 11:37:35AM -0600, Jason Gunthorpe wrote:
> > > On Tue, Jun 27, 2017 at 08:33:01PM +0300, Leon Romanovsky wrote:
> > >
> > > > My initial plan was to put all parsers under their respective names, in
> > > > the similar way as I did for caps: $ rdma dev show mlx5_4 caps
> > >
> > > I think you should have a useful summary display similar to 'ip a' and
> > > other commands.
> > >
> > > guid(s), subnet prefix or default gid for IB, lid/lmc, link state,
> > > speed, mtu, pkeys protocol(s)
> >
> > It will, but before I would like to see this tool be a part of
> > iproute2, so other people will be able to extend it in addition
> > to me.
> >
> > Are you fine with the proposed code?
> >
>
> Output formats need to be nailed down. The output of iproute2 commands is 
> almost
> like an ABI. Users build scripts to parse it (whether that is a great idea or 
> not
> is debateable, it mostly shows the weakness in programatic API's). Therefore 
> fully
> changing output formats in later revisions is likely to get users upset.
>
> The first version doesn't have to be perfect, just close to the overall goal
> of what is planned.

In this version, I'm going to use arrays without indexes, because I prefer
to expose the bare minimum from the kernel, which is RDMA netlink. After
everything else is settled, I'll move those defines to UAPI and reuse them
in rdmatool.

I'll send new version with -d/--details flag and enrich minimal summary.
It won't include anything related to tables gids, pkey yet.

Thanks



signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Stephen Hemminger]

On Tue, 27 Jun 2017 20:46:15 +0300
Leon Romanovsky  wrote:

> On Tue, Jun 27, 2017 at 11:37:35AM -0600, Jason Gunthorpe wrote:
> > On Tue, Jun 27, 2017 at 08:33:01PM +0300, Leon Romanovsky wrote:
> >  
> > > My initial plan was to put all parsers under their respective names, in
> > > the similar way as I did for caps: $ rdma dev show mlx5_4 caps  
> >
> > I think you should have a useful summary display similar to 'ip a' and
> > other commands.
> >
> > guid(s), subnet prefix or default gid for IB, lid/lmc, link state,
> > speed, mtu, pkeys protocol(s)  
> 
> It will, but before I would like to see this tool be a part of
> iproute2, so other people will be able to extend it in addition
> to me.
> 
> Are you fine with the proposed code?
> 

Output formats need to be nailed down. The output of iproute2 commands is almost
like an ABI. Users build scripts to parse it (whether that is a great idea or 
not
is debateable, it mostly shows the weakness in programatic API's). Therefore 
fully
changing output formats in later revisions is likely to get users upset.

The first version doesn't have to be perfect, just close to the overall goal
of what is planned.  


pgpQskCG_Bf7a.pgp
Description: OpenPGP digital signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Stephen Hemminger]

On Tue, 27 Jun 2017 20:33:01 +0300
Leon Romanovsky  wrote:

> On Tue, Jun 27, 2017 at 10:41:50AM -0600, Jason Gunthorpe wrote:
> > On Tue, Jun 27, 2017 at 12:21:29PM +0300, Leon Romanovsky wrote:  
> > > > What will be the output of such command?
> > > >  $ rdma dev show mlx5_4  
> > >
> > > ip-like style:
> > >
> > > $ rdma dev show mlx5_4
> > > 5: mlx5_4:
> > > caps:  > > PORT_ACTIVE_EVENT, SYS_IMAGE_GUID, RC_RNR_NAK_GEN, MEM_WINDOW, 
> > > UD_IP_CSUM, UD_TSO, XRC, MEM_MGT_EXTENSIONS, BLOCK_MULTICAST_LOOPBACK, 
> > > MEM_WINDOW_TYPE_2B, RAW_IP_CSUM, SIGNATURE_HANDOVER, VIRTUAL_FUNCTION>
> > > $ rdma link show mlx5_3
> > > 4/1: mlx5_3/1:
> > > caps:   
> >
> > I think that is better, maybe it should only show under some kind of
> > verbose mode, I don't know, it depends what other stuff ends up being
> > displayed..
> >
> > Are you going to dump the gid table and pkey table too in one of these 
> > commands?  
> 
> My initial plan was to put all parsers under their respective names, in
> the similar way as I did for caps: $ rdma dev show mlx5_4 caps
> 
> So for large dumps, I'm going to use that technique again and maybe print 
> summary as a default.
> For example, for gids, we can print utilization as a summary while whole
> table if someone really wants it: $ rdma link show mlx5_4/1 gids 
> 
> Something like that.
> 
> Thanks
> 
> >
> > Jason  

Agree with discussion so far.

For iproute2 style commands, the show and set commands need to have similar 
arguments.
Ideally, everything after the colon in the show would be parameters to set 
command.

Please consider having a concise form for normal users and a detail form (with 
-d) for 
configuration and setup cases. The caps should not need to be displayed in 
normal show
output.


pgpbdA61_hyMI.pgp
Description: OpenPGP digital signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Tue, Jun 27, 2017 at 11:37:35AM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 27, 2017 at 08:33:01PM +0300, Leon Romanovsky wrote:
>
> > My initial plan was to put all parsers under their respective names, in
> > the similar way as I did for caps: $ rdma dev show mlx5_4 caps
>
> I think you should have a useful summary display similar to 'ip a' and
> other commands.
>
> guid(s), subnet prefix or default gid for IB, lid/lmc, link state,
> speed, mtu, pkeys protocol(s)

It will, but before I would like to see this tool be a part of
iproute2, so other people will be able to extend it in addition
to me.

Are you fine with the proposed code?

>
> A show gid table command makes sense for rocee where it can show the
> gid and the IP binding for it, rocee mode, etc..
>
> Jason


signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Jason Gunthorpe]

On Tue, Jun 27, 2017 at 08:33:01PM +0300, Leon Romanovsky wrote:

> My initial plan was to put all parsers under their respective names, in
> the similar way as I did for caps: $ rdma dev show mlx5_4 caps

I think you should have a useful summary display similar to 'ip a' and
other commands.

guid(s), subnet prefix or default gid for IB, lid/lmc, link state,
speed, mtu, pkeys protocol(s)

A show gid table command makes sense for rocee where it can show the
gid and the IP binding for it, rocee mode, etc..

Jason

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Tue, Jun 27, 2017 at 10:41:50AM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 27, 2017 at 12:21:29PM +0300, Leon Romanovsky wrote:
> > > What will be the output of such command?
> > >  $ rdma dev show mlx5_4
> >
> > ip-like style:
> >
> > $ rdma dev show mlx5_4
> > 5: mlx5_4:
> > caps:  > PORT_ACTIVE_EVENT, SYS_IMAGE_GUID, RC_RNR_NAK_GEN, MEM_WINDOW, UD_IP_CSUM, 
> > UD_TSO, XRC, MEM_MGT_EXTENSIONS, BLOCK_MULTICAST_LOOPBACK, 
> > MEM_WINDOW_TYPE_2B, RAW_IP_CSUM, SIGNATURE_HANDOVER, VIRTUAL_FUNCTION>
> > $ rdma link show mlx5_3
> > 4/1: mlx5_3/1:
> > caps: 
>
> I think that is better, maybe it should only show under some kind of
> verbose mode, I don't know, it depends what other stuff ends up being
> displayed..
>
> Are you going to dump the gid table and pkey table too in one of these 
> commands?

My initial plan was to put all parsers under their respective names, in
the similar way as I did for caps: $ rdma dev show mlx5_4 caps

So for large dumps, I'm going to use that technique again and maybe print 
summary as a default.
For example, for gids, we can print utilization as a summary while whole
table if someone really wants it: $ rdma link show mlx5_4/1 gids 

Something like that.

Thanks

>
> Jason


signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Jason Gunthorpe]

On Tue, Jun 27, 2017 at 12:21:29PM +0300, Leon Romanovsky wrote:
> > What will be the output of such command?
> >  $ rdma dev show mlx5_4
> 
> ip-like style:
> 
> $ rdma dev show mlx5_4
> 5: mlx5_4:
> caps:  SYS_IMAGE_GUID, RC_RNR_NAK_GEN, MEM_WINDOW, UD_IP_CSUM, UD_TSO, XRC, 
> MEM_MGT_EXTENSIONS, BLOCK_MULTICAST_LOOPBACK, MEM_WINDOW_TYPE_2B, 
> RAW_IP_CSUM, SIGNATURE_HANDOVER, VIRTUAL_FUNCTION>
> $ rdma link show mlx5_3
> 4/1: mlx5_3/1:
> caps: 

I think that is better, maybe it should only show under some kind of
verbose mode, I don't know, it depends what other stuff ends up being
displayed..

Are you going to dump the gid table and pkey table too in one of these commands?

Jason

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Tue, Jun 27, 2017 at 07:06:04AM +0300, Leon Romanovsky wrote:
> On Mon, Jun 26, 2017 at 02:36:10PM -0600, Jason Gunthorpe wrote:
> > On Mon, Jun 26, 2017 at 10:21:03PM +0300, Leon Romanovsky wrote:
> > > On Mon, Jun 26, 2017 at 12:29:24PM -0600, Jason Gunthorpe wrote:
> > > > On Mon, Jun 26, 2017 at 09:21:26PM +0300, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky 
> > > > >
> > > > > Add parsing interface for the device capability flags
> > > > >
> > > > > $ rdma dev show
> > > > > 1: mlx5_0: caps 0x1257e1c26
> > > >
> > > > This seems very un ip-like. I wouldn't show an undecoded hex value
> > > > like that, it isn't really useful.
> > >
> > > It is first supported field, after new fields will be added, we will
> > > have very similar to ip interface.
> > >
> > > 1: mlx5_0: caps 0x1257e1c2 key_1 val_1 key_2 val_2 
> > >
> > > The values are presented as is can be usable as an input for different 
> > > scripts.
> >
> > I still wouldn't show an undecoded hex value.. It isn't useful.
> >
> > > > > $ rdma dev show mlx5_4 caps
> > > > > 5: mlx5_4: caps 0x1257e1c26
> > > > > Bit   Description
> > > > >  01   DEVICE_BAD_PKEY_CNTR
> > > > >  02   DEVICE_BAD_QKEY_CNTR
> > > >
> > > > This table also seems un ip-like, the usual format is a list of words,
> > > > I think.
> > >
> > > It is true for key<->value data, but it is less obvious for bit
> > > parsing.
> >
> > Several of the word decodes are from bit fields..
> >
> > > Internally, I tried to present them as list and it was ugly like hell
> > > without any chance (without extra parsing) to actual see if specific
> > > capability is present or no.
> >
> > lspci seems to have no problem being readable while doing this..
>
> No problem,
> If i understand you correctly, you are suggesting to drop parsing of
> "caps" as a separate command and embed it into general show .
>
> Can you help me and give an example of how would you present those caps?
> What will be the output of such command?
>  $ rdma dev show mlx5_4

ip-like style:

$ rdma dev show mlx5_4
5: mlx5_4:
caps: 
$ rdma link show mlx5_3
4/1: mlx5_3/1:
caps: 

Thanks

>
>  Thanks
>
> >
> > Jason




signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Mon, Jun 26, 2017 at 02:36:10PM -0600, Jason Gunthorpe wrote:
> On Mon, Jun 26, 2017 at 10:21:03PM +0300, Leon Romanovsky wrote:
> > On Mon, Jun 26, 2017 at 12:29:24PM -0600, Jason Gunthorpe wrote:
> > > On Mon, Jun 26, 2017 at 09:21:26PM +0300, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky 
> > > >
> > > > Add parsing interface for the device capability flags
> > > >
> > > > $ rdma dev show
> > > > 1: mlx5_0: caps 0x1257e1c26
> > >
> > > This seems very un ip-like. I wouldn't show an undecoded hex value
> > > like that, it isn't really useful.
> >
> > It is first supported field, after new fields will be added, we will
> > have very similar to ip interface.
> >
> > 1: mlx5_0: caps 0x1257e1c2 key_1 val_1 key_2 val_2 
> >
> > The values are presented as is can be usable as an input for different 
> > scripts.
>
> I still wouldn't show an undecoded hex value.. It isn't useful.
>
> > > > $ rdma dev show mlx5_4 caps
> > > > 5: mlx5_4: caps 0x1257e1c26
> > > > Bit Description
> > > >  01 DEVICE_BAD_PKEY_CNTR
> > > >  02 DEVICE_BAD_QKEY_CNTR
> > >
> > > This table also seems un ip-like, the usual format is a list of words,
> > > I think.
> >
> > It is true for key<->value data, but it is less obvious for bit
> > parsing.
>
> Several of the word decodes are from bit fields..
>
> > Internally, I tried to present them as list and it was ugly like hell
> > without any chance (without extra parsing) to actual see if specific
> > capability is present or no.
>
> lspci seems to have no problem being readable while doing this..

No problem,
If i understand you correctly, you are suggesting to drop parsing of
"caps" as a separate command and embed it into general show .

Can you help me and give an example of how would you present those caps?
What will be the output of such command?
 $ rdma dev show mlx5_4

 Thanks

>
> Jason


signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Jason Gunthorpe]

On Mon, Jun 26, 2017 at 10:21:03PM +0300, Leon Romanovsky wrote:
> On Mon, Jun 26, 2017 at 12:29:24PM -0600, Jason Gunthorpe wrote:
> > On Mon, Jun 26, 2017 at 09:21:26PM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky 
> > >
> > > Add parsing interface for the device capability flags
> > >
> > > $ rdma dev show
> > > 1: mlx5_0: caps 0x1257e1c26
> >
> > This seems very un ip-like. I wouldn't show an undecoded hex value
> > like that, it isn't really useful.
> 
> It is first supported field, after new fields will be added, we will
> have very similar to ip interface.
> 
> 1: mlx5_0: caps 0x1257e1c2 key_1 val_1 key_2 val_2 
> 
> The value are presented as is can be usable as an input for different scripts.

I still wouldn't show an undecoded hex value.. It isn't useful.

> > > $ rdma dev show mlx5_4 caps
> > > 5: mlx5_4: caps 0x1257e1c26
> > > Bit   Description
> > >  01   DEVICE_BAD_PKEY_CNTR
> > >  02   DEVICE_BAD_QKEY_CNTR
> >
> > This table also seems un ip-like, the usual format is a list of words,
> > I think.
> 
> It is true for key<->value data, but it is less obvious for bit
> parsing.

Several of the word decodes are from bit fields..

> Internally, I tried to present them as list and it was ugly like hell
> without any chance (without extra parsing) to actual see if specific
> capability is present or no.

lspci seems to have no problem being readable while doing this..

Jason

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Leon Romanovsky]

On Mon, Jun 26, 2017 at 12:29:24PM -0600, Jason Gunthorpe wrote:
> On Mon, Jun 26, 2017 at 09:21:26PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky 
> >
> > Add parsing interface for the device capability flags
> >
> > $ rdma dev show
> > 1: mlx5_0: caps 0x1257e1c26
>
> This seems very un ip-like. I wouldn't show an undecoded hex value
> like that, it isn't really useful.

It is first supported field, after new fields will be added, we will
have very similar to ip interface.

1: mlx5_0: caps 0x1257e1c2 key_1 val_1 key_2 val_2 

The value are presented as is can be usable as an input for different scripts.

>
> > $ rdma dev show mlx5_4 caps
> > 5: mlx5_4: caps 0x1257e1c26
> > Bit Description
> >  01 DEVICE_BAD_PKEY_CNTR
> >  02 DEVICE_BAD_QKEY_CNTR
>
> This table also seems un ip-like, the usual format is a list of words,
> I think.

It is true for key<->value data, but it is less obvious for bit parsing.
Internally, I tried to present them as list and it was ugly like hell
without any chance (without extra parsing) to actual see if specific
capability is present or no.

The question is: is such presentation usable and does it make sense?

>
> > $ rdma dev show mlx5_4 cap_flags
> > Unknown parameter 'caps_flags'.
>
> ?

An example of "wrong" parameter. There is no such caps_flags.

>
> Jason


signature.asc
Description: PGP signature

Re: [PATCH iproute2 3/5] rdma: Add device capability parsing

[Jason Gunthorpe]

On Mon, Jun 26, 2017 at 09:21:26PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky 
> 
> Add parsing interface for the device capability flags
> 
> $ rdma dev show
> 1: mlx5_0: caps 0x1257e1c26

This seems very un ip-like. I wouldn't show an undecoded hex value
like that, it isn't really useful.

> $ rdma dev show mlx5_4 caps
> 5: mlx5_4: caps 0x1257e1c26
> Bit   Description
>  01   DEVICE_BAD_PKEY_CNTR
>  02   DEVICE_BAD_QKEY_CNTR

This table also seems un ip-like, the usual format is a list of words,
I think.

> $ rdma dev show mlx5_4 cap_flags
> Unknown parameter 'caps_flags'.

?

Jason