[PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The nldev was implemented by following devlink implementation,
including SET/DEL/NEW commands. However these commands were not
implemented and hence don't need to be exposed.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
index 17e59bec169e..4c77e2a7b07e 100644
--- a/include/uapi/rdma/rdma_netlink.h
+++ b/include/uapi/rdma/rdma_netlink.h
@@ -227,16 +227,14 @@ enum rdma_nldev_command {
 	RDMA_NLDEV_CMD_UNSPEC,

 	RDMA_NLDEV_CMD_GET, /* can dump */
-	RDMA_NLDEV_CMD_SET,
-	RDMA_NLDEV_CMD_NEW,
-	RDMA_NLDEV_CMD_DEL,

-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
-	RDMA_NLDEV_CMD_PORT_SET,
-	RDMA_NLDEV_CMD_PORT_NEW,
-	RDMA_NLDEV_CMD_PORT_DEL,
+	/* 2 - 4 are free to use */

-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
+
+	/* 6 - 8 are free to use */
+
+	RDMA_NLDEV_CMD_RES_GET = 9, /* can dump */

 	RDMA_NLDEV_CMD_RES_QP_GET, /* can dump */

--
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Jiri Pirko]

Tue, Jan 30, 2018 at 04:07:16PM CET, leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
>From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
>The nldev was implemented by following devlink implementation,
>including SET/DEL/NEW commands. However these commands were not
>implemented and hence don't need to be exposed.
>
>Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>---
> include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
>diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
>index 17e59bec169e..4c77e2a7b07e 100644
>--- a/include/uapi/rdma/rdma_netlink.h
>+++ b/include/uapi/rdma/rdma_netlink.h
>@@ -227,16 +227,14 @@ enum rdma_nldev_command {
> 	RDMA_NLDEV_CMD_UNSPEC,
>
> 	RDMA_NLDEV_CMD_GET, /* can dump */
>-	RDMA_NLDEV_CMD_SET,
>-	RDMA_NLDEV_CMD_NEW,
>-	RDMA_NLDEV_CMD_DEL,
>
>-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
>-	RDMA_NLDEV_CMD_PORT_SET,
>-	RDMA_NLDEV_CMD_PORT_NEW,
>-	RDMA_NLDEV_CMD_PORT_DEL,
>+	/* 2 - 4 are free to use */
>
>-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
>+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
>+
>+	/* 6 - 8 are free to use */

I don't see reason for the wholes. Also, Don't you use *_NEW for replies
to *_GET, as it is common for Netlink?

But anyway, I would rather have them in, or don't have them in but don't
leave holes for them.


>+
>+	RDMA_NLDEV_CMD_RES_GET = 9, /* can dump */
>
> 	RDMA_NLDEV_CMD_RES_QP_GET, /* can dump */
>
>--
>2.16.1
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Jason Gunthorpe]

On Tue, Jan 30, 2018 at 04:57:06PM +0100, Jiri Pirko wrote:
> Tue, Jan 30, 2018 at 04:07:16PM CET, leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
> >From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> >The nldev was implemented by following devlink implementation,
> >including SET/DEL/NEW commands. However these commands were not
> >implemented and hence don't need to be exposed.
> >
> >Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
> >index 17e59bec169e..4c77e2a7b07e 100644
> >+++ b/include/uapi/rdma/rdma_netlink.h
> >@@ -227,16 +227,14 @@ enum rdma_nldev_command {
> > 	RDMA_NLDEV_CMD_UNSPEC,
> >
> > 	RDMA_NLDEV_CMD_GET, /* can dump */
> >-	RDMA_NLDEV_CMD_SET,
> >-	RDMA_NLDEV_CMD_NEW,
> >-	RDMA_NLDEV_CMD_DEL,
> >
> >-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
> >-	RDMA_NLDEV_CMD_PORT_SET,
> >-	RDMA_NLDEV_CMD_PORT_NEW,
> >-	RDMA_NLDEV_CMD_PORT_DEL,
> >+	/* 2 - 4 are free to use */
> >
> >-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
> >+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
> >+
> >+	/* 6 - 8 are free to use */
> 
> I don't see reason for the wholes.

We are stuck with the holes, it is fixed uABI at this point, can't
change the constants we actually implemented.

> Also, Don't you use *_NEW for replies to *_GET, as it is common for
> Netlink?

It appears not..

IMHO that convention makes the most sense when there is a GET/NEW pair
with the idea the NEW is formed to be echoed back to the kernel -
these case are all GET only..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Jiri Pirko]

Tue, Jan 30, 2018 at 05:03:37PM CET, jgg-uk2M96/98Pc@public.gmane.org wrote:
>On Tue, Jan 30, 2018 at 04:57:06PM +0100, Jiri Pirko wrote:
>> Tue, Jan 30, 2018 at 04:07:16PM CET, leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
>> >From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> >
>> >The nldev was implemented by following devlink implementation,
>> >including SET/DEL/NEW commands. However these commands were not
>> >implemented and hence don't need to be exposed.
>> >
>> >Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> > include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
>> > 1 file changed, 6 insertions(+), 8 deletions(-)
>> >
>> >diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
>> >index 17e59bec169e..4c77e2a7b07e 100644
>> >+++ b/include/uapi/rdma/rdma_netlink.h
>> >@@ -227,16 +227,14 @@ enum rdma_nldev_command {
>> > 	RDMA_NLDEV_CMD_UNSPEC,
>> >
>> > 	RDMA_NLDEV_CMD_GET, /* can dump */
>> >-	RDMA_NLDEV_CMD_SET,
>> >-	RDMA_NLDEV_CMD_NEW,
>> >-	RDMA_NLDEV_CMD_DEL,
>> >
>> >-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
>> >-	RDMA_NLDEV_CMD_PORT_SET,
>> >-	RDMA_NLDEV_CMD_PORT_NEW,
>> >-	RDMA_NLDEV_CMD_PORT_DEL,
>> >+	/* 2 - 4 are free to use */
>> >
>> >-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
>> >+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
>> >+
>> >+	/* 6 - 8 are free to use */
>> 
>> I don't see reason for the wholes.
>
>We are stuck with the holes, it is fixed uABI at this point, can't
>change the constants we actually implemented.

If it is realeased already, we can't remove the defines either.


>
>> Also, Don't you use *_NEW for replies to *_GET, as it is common for
>> Netlink?
>
>It appears not..
>
>IMHO that convention makes the most sense when there is a GET/NEW pair
>with the idea the NEW is formed to be echoed back to the kernel -
>these case are all GET only..
>
>Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Leon Romanovsky]

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

On Tue, Jan 30, 2018 at 07:18:14PM +0100, Jiri Pirko wrote:
> Tue, Jan 30, 2018 at 05:03:37PM CET, jgg-uk2M96/98Pc@public.gmane.org wrote:
> >On Tue, Jan 30, 2018 at 04:57:06PM +0100, Jiri Pirko wrote:
> >> Tue, Jan 30, 2018 at 04:07:16PM CET, leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
> >> >From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> >
> >> >The nldev was implemented by following devlink implementation,
> >> >including SET/DEL/NEW commands. However these commands were not
> >> >implemented and hence don't need to be exposed.
> >> >
> >> >Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> > include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
> >> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >> >
> >> >diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
> >> >index 17e59bec169e..4c77e2a7b07e 100644
> >> >+++ b/include/uapi/rdma/rdma_netlink.h
> >> >@@ -227,16 +227,14 @@ enum rdma_nldev_command {
> >> > 	RDMA_NLDEV_CMD_UNSPEC,
> >> >
> >> > 	RDMA_NLDEV_CMD_GET, /* can dump */
> >> >-	RDMA_NLDEV_CMD_SET,
> >> >-	RDMA_NLDEV_CMD_NEW,
> >> >-	RDMA_NLDEV_CMD_DEL,
> >> >
> >> >-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
> >> >-	RDMA_NLDEV_CMD_PORT_SET,
> >> >-	RDMA_NLDEV_CMD_PORT_NEW,
> >> >-	RDMA_NLDEV_CMD_PORT_DEL,
> >> >+	/* 2 - 4 are free to use */
> >> >
> >> >-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
> >> >+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
> >> >+
> >> >+	/* 6 - 8 are free to use */
> >>
> >> I don't see reason for the wholes.
> >
> >We are stuck with the holes, it is fixed uABI at this point, can't
> >change the constants we actually implemented.
>
> If it is realeased already, we can't remove the defines either.

Why? There is exactly one consumer - rdmatool and it doesn't use removed
constants.

>
>
> >
> >> Also, Don't you use *_NEW for replies to *_GET, as it is common for
> >> Netlink?
> >
> >It appears not..
> >
> >IMHO that convention makes the most sense when there is a GET/NEW pair
> >with the idea the NEW is formed to be echoed back to the kernel -
> >these case are all GET only..
> >
> >Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Jason Gunthorpe]

On Tue, Jan 30, 2018 at 07:18:14PM +0100, Jiri Pirko wrote:
> Tue, Jan 30, 2018 at 05:03:37PM CET, jgg-uk2M96/98Pc@public.gmane.org wrote:
> >On Tue, Jan 30, 2018 at 04:57:06PM +0100, Jiri Pirko wrote:
> >> Tue, Jan 30, 2018 at 04:07:16PM CET, leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
> >> >From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> >
> >> >The nldev was implemented by following devlink implementation,
> >> >including SET/DEL/NEW commands. However these commands were not
> >> >implemented and hence don't need to be exposed.
> >> >
> >> >Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> > include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
> >> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >> >
> >> >diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
> >> >index 17e59bec169e..4c77e2a7b07e 100644
> >> >+++ b/include/uapi/rdma/rdma_netlink.h
> >> >@@ -227,16 +227,14 @@ enum rdma_nldev_command {
> >> > 	RDMA_NLDEV_CMD_UNSPEC,
> >> >
> >> > 	RDMA_NLDEV_CMD_GET, /* can dump */
> >> >-	RDMA_NLDEV_CMD_SET,
> >> >-	RDMA_NLDEV_CMD_NEW,
> >> >-	RDMA_NLDEV_CMD_DEL,
> >> >
> >> >-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
> >> >-	RDMA_NLDEV_CMD_PORT_SET,
> >> >-	RDMA_NLDEV_CMD_PORT_NEW,
> >> >-	RDMA_NLDEV_CMD_PORT_DEL,
> >> >+	/* 2 - 4 are free to use */
> >> >
> >> >-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
> >> >+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
> >> >+
> >> >+	/* 6 - 8 are free to use */
> >> 
> >> I don't see reason for the wholes.
> >
> >We are stuck with the holes, it is fixed uABI at this point, can't
> >change the constants we actually implemented.
> 
> If it is realeased already, we can't remove the defines either.

We have been less absolute on the uapi headers causing compilation
breaks.

Using these headers is so hard, eg iproute and rdma-core take them
verbatim from the kernel and bundles them inside. So as a practical
matter, it is OK to sometimes cause compilation issues.

In this case there is no correct use of the constants since the
kernel never implemented them, any user is busted in some way and
should be reviewed.

Even glibc causes compilation breaks in new versions..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Leon Romanovsky]

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

On Tue, Jan 30, 2018 at 09:03:37AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 30, 2018 at 04:57:06PM +0100, Jiri Pirko wrote:
> > Tue, Jan 30, 2018 at 04:07:16PM CET, leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
> > >From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >
> > >The nldev was implemented by following devlink implementation,
> > >including SET/DEL/NEW commands. However these commands were not
> > >implemented and hence don't need to be exposed.
> > >
> > >Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
> > > 1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > >diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
> > >index 17e59bec169e..4c77e2a7b07e 100644
> > >+++ b/include/uapi/rdma/rdma_netlink.h
> > >@@ -227,16 +227,14 @@ enum rdma_nldev_command {
> > > 	RDMA_NLDEV_CMD_UNSPEC,
> > >
> > > 	RDMA_NLDEV_CMD_GET, /* can dump */
> > >-	RDMA_NLDEV_CMD_SET,
> > >-	RDMA_NLDEV_CMD_NEW,
> > >-	RDMA_NLDEV_CMD_DEL,
> > >
> > >-	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
> > >-	RDMA_NLDEV_CMD_PORT_SET,
> > >-	RDMA_NLDEV_CMD_PORT_NEW,
> > >-	RDMA_NLDEV_CMD_PORT_DEL,
> > >+	/* 2 - 4 are free to use */
> > >
> > >-	RDMA_NLDEV_CMD_RES_GET, /* can dump */
> > >+	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
> > >+
> > >+	/* 6 - 8 are free to use */
> >
> > I don't see reason for the wholes.
>
> We are stuck with the holes, it is fixed uABI at this point, can't
> change the constants we actually implemented.
>
> > Also, Don't you use *_NEW for replies to *_GET, as it is common for
> > Netlink?
>
> It appears not..
>
> IMHO that convention makes the most sense when there is a GET/NEW pair
> with the idea the NEW is formed to be echoed back to the kernel -
> these case are all GET only..

Back then, I didn't find any reason to return _NEW, and I don't see it
now either.

There is NLM_F_ECHO flag for echo and sequence number to distinguish
between messages.

>
> Jason

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

Re: [PATCH rdma/wip/jgg-for-next] RDMA/netlink: Hide unimplemented NLDEV commands

[Jason Gunthorpe]

On Tue, Jan 30, 2018 at 05:07:16PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The nldev was implemented by following devlink implementation,
> including SET/DEL/NEW commands. However these commands were not
> implemented and hence don't need to be exposed.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  include/uapi/rdma/rdma_netlink.h | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Doug applied this to for-next

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html