| Message ID | 1494251057-1060-1-git-send-email-devesh.sharma@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 08, 2017 at 09:44:17AM -0400, Devesh Sharma wrote: > Currently application either need to define its own enum > values for link-speed and link-width or use hard coded > values. This patch inharits the enums from kernel space > ib_verbs.h and puts into verbs.h for ease of programming. > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > --- > libibverbs/examples/devinfo.c | 22 ++++++++++++---------- > libibverbs/verbs.h | 18 ++++++++++++++++++ > 2 files changed, 30 insertions(+), 10 deletions(-) I think the better approach will be move these defines from include/rdma/ib_verbs.h to include/uapi/rdma/ib_user_verbs.h and reuse ib_user_verbs.h file directly in libibverbs. > > diff --git a/libibverbs/examples/devinfo.c b/libibverbs/examples/devinfo.c > index 2c1e3f1..42ed50c 100644 > --- a/libibverbs/examples/devinfo.c > +++ b/libibverbs/examples/devinfo.c > @@ -124,10 +124,10 @@ static const char *mtu_str(enum ibv_mtu max_mtu) > static const char *width_str(uint8_t width) > { > switch (width) { > - case 1: return "1"; > - case 2: return "4"; > - case 4: return "8"; > - case 8: return "12"; > + case IBV_WIDTH_1X: return "1"; > + case IBV_WIDTH_4X: return "4"; > + case IBV_WIDTH_8X: return "8"; > + case IBV_WIDTH_12X: return "12"; > default: return "invalid width"; > } > } > @@ -135,14 +135,16 @@ static const char *width_str(uint8_t width) > static const char *speed_str(uint8_t speed) > { > switch (speed) { > - case 1: return "2.5 Gbps"; > - case 2: return "5.0 Gbps"; > + case IBV_SPEED_SDR: return "2.5 Gbps"; > + case IBV_SPEED_DDR: return "5.0 Gbps"; > > - case 4: /* fall through */ > - case 8: return "10.0 Gbps"; > + case IBV_SPEED_QDR: /* fall through */ > + case IBV_SPEED_FDR10: return "10.0 Gbps"; > > - case 16: return "14.0 Gbps"; > - case 32: return "25.0 Gbps"; > + case IBV_SPEED_FDR: return "14.0 Gbps"; > + case IBV_SPEED_EDR: return "25.0 Gbps"; > + case IBV_SPEED_HDR: return "50.0 Gbps"; > + case IBV_SPEE_NDR: return "100.0 Gbps"; > default: return "invalid speed"; > } > } > diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h > index b27dfd1..39a65fe 100644 > --- a/libibverbs/verbs.h > +++ b/libibverbs/verbs.h > @@ -290,6 +290,24 @@ enum ibv_port_state { > IBV_PORT_ACTIVE_DEFER = 5 > }; > > +enum ibv_port_speed { > + IBV_SPEED_SDR = 1, > + IBV_SPEED_DDR = 2, > + IBV_SPEED_QDR = 4, > + IBV_SPEED_FDR10 = 8, > + IBV_SPEED_FDR = 16, > + IBV_SPEED_EDR = 32, > + IBV_SPEED_HDR = 64, > + IBV_SPEED_NDR = 128 > +}; > + > +enum ibv_port_width { > + IBV_WIDTH_1X = 1, > + IBV_WIDTH_4X = 2, > + IBV_WIDTH_8X = 4, > + IBV_WIDTH_12X = 8 > +}; > + > enum { > IBV_LINK_LAYER_UNSPECIFIED, > IBV_LINK_LAYER_INFINIBAND, > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/8/2017 9:44 AM, Devesh Sharma wrote: > Currently application either need to define its own enum > values for link-speed and link-width or use hard coded > values. This patch inharits the enums from kernel space > ib_verbs.h and puts into verbs.h for ease of programming. > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > --- > libibverbs/examples/devinfo.c | 22 ++++++++++++---------- > libibverbs/verbs.h | 18 ++++++++++++++++++ > 2 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/libibverbs/examples/devinfo.c b/libibverbs/examples/devinfo.c > index 2c1e3f1..42ed50c 100644 > --- a/libibverbs/examples/devinfo.c > +++ b/libibverbs/examples/devinfo.c > @@ -124,10 +124,10 @@ static const char *mtu_str(enum ibv_mtu max_mtu) > static const char *width_str(uint8_t width) > { > switch (width) { > - case 1: return "1"; > - case 2: return "4"; > - case 4: return "8"; > - case 8: return "12"; > + case IBV_WIDTH_1X: return "1"; > + case IBV_WIDTH_4X: return "4"; > + case IBV_WIDTH_8X: return "8"; > + case IBV_WIDTH_12X: return "12"; > default: return "invalid width"; > } > } > @@ -135,14 +135,16 @@ static const char *width_str(uint8_t width) > static const char *speed_str(uint8_t speed) > { > switch (speed) { > - case 1: return "2.5 Gbps"; > - case 2: return "5.0 Gbps"; > + case IBV_SPEED_SDR: return "2.5 Gbps"; > + case IBV_SPEED_DDR: return "5.0 Gbps"; > > - case 4: /* fall through */ > - case 8: return "10.0 Gbps"; > + case IBV_SPEED_QDR: /* fall through */ > + case IBV_SPEED_FDR10: return "10.0 Gbps"; > > - case 16: return "14.0 Gbps"; > - case 32: return "25.0 Gbps"; > + case IBV_SPEED_FDR: return "14.0 Gbps"; > + case IBV_SPEED_EDR: return "25.0 Gbps"; > + case IBV_SPEED_HDR: return "50.0 Gbps"; > + case IBV_SPEE_NDR: return "100.0 Gbps"; typo > default: return "invalid speed"; > } > } > diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h > index b27dfd1..39a65fe 100644 > --- a/libibverbs/verbs.h > +++ b/libibverbs/verbs.h > @@ -290,6 +290,24 @@ enum ibv_port_state { > IBV_PORT_ACTIVE_DEFER = 5 > }; > > +enum ibv_port_speed { > + IBV_SPEED_SDR = 1, > + IBV_SPEED_DDR = 2, > + IBV_SPEED_QDR = 4, > + IBV_SPEED_FDR10 = 8, > + IBV_SPEED_FDR = 16, > + IBV_SPEED_EDR = 32, > + IBV_SPEED_HDR = 64, > + IBV_SPEED_NDR = 128 HDR is almost defined now but NDR is not. Is it premature to put in NDR now ? Also, if either of these are being added, should 2x also be added ? -- Hal > +}; > + > +enum ibv_port_width { > + IBV_WIDTH_1X = 1, > + IBV_WIDTH_4X = 2, > + IBV_WIDTH_8X = 4, > + IBV_WIDTH_12X = 8 > +}; > + > enum { > IBV_LINK_LAYER_UNSPECIFIED, > IBV_LINK_LAYER_INFINIBAND, > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/08/2017 09:54 AM, Leon Romanovsky wrote: > On Mon, May 08, 2017 at 09:44:17AM -0400, Devesh Sharma wrote: >> Currently application either need to define its own enum >> values for link-speed and link-width or use hard coded >> values. This patch inharits the enums from kernel space >> ib_verbs.h and puts into verbs.h for ease of programming. >> >> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> >> --- >> libibverbs/examples/devinfo.c | 22 ++++++++++++---------- >> libibverbs/verbs.h | 18 ++++++++++++++++++ >> 2 files changed, 30 insertions(+), 10 deletions(-) > > I think the better approach will be move these defines from > include/rdma/ib_verbs.h to include/uapi/rdma/ib_user_verbs.h > and reuse ib_user_verbs.h file directly in libibverbs. I agree on this. Please put this stuff in the uapi header file and include that in user space. -Denny -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 8, 2017 at 7:24 PM, Hal Rosenstock <hal@dev.mellanox.co.il> wrote: > On 5/8/2017 9:44 AM, Devesh Sharma wrote: >> Currently application either need to define its own enum >> values for link-speed and link-width or use hard coded >> values. This patch inharits the enums from kernel space >> ib_verbs.h and puts into verbs.h for ease of programming. >> >> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> >> --- >> libibverbs/examples/devinfo.c | 22 ++++++++++++---------- >> libibverbs/verbs.h | 18 ++++++++++++++++++ >> 2 files changed, 30 insertions(+), 10 deletions(-) >> >> diff --git a/libibverbs/examples/devinfo.c b/libibverbs/examples/devinfo.c >> index 2c1e3f1..42ed50c 100644 >> --- a/libibverbs/examples/devinfo.c >> +++ b/libibverbs/examples/devinfo.c >> @@ -124,10 +124,10 @@ static const char *mtu_str(enum ibv_mtu max_mtu) >> static const char *width_str(uint8_t width) >> { >> switch (width) { >> - case 1: return "1"; >> - case 2: return "4"; >> - case 4: return "8"; >> - case 8: return "12"; >> + case IBV_WIDTH_1X: return "1"; >> + case IBV_WIDTH_4X: return "4"; >> + case IBV_WIDTH_8X: return "8"; >> + case IBV_WIDTH_12X: return "12"; >> default: return "invalid width"; >> } >> } >> @@ -135,14 +135,16 @@ static const char *width_str(uint8_t width) >> static const char *speed_str(uint8_t speed) >> { >> switch (speed) { >> - case 1: return "2.5 Gbps"; >> - case 2: return "5.0 Gbps"; >> + case IBV_SPEED_SDR: return "2.5 Gbps"; >> + case IBV_SPEED_DDR: return "5.0 Gbps"; >> >> - case 4: /* fall through */ >> - case 8: return "10.0 Gbps"; >> + case IBV_SPEED_QDR: /* fall through */ >> + case IBV_SPEED_FDR10: return "10.0 Gbps"; >> >> - case 16: return "14.0 Gbps"; >> - case 32: return "25.0 Gbps"; >> + case IBV_SPEED_FDR: return "14.0 Gbps"; >> + case IBV_SPEED_EDR: return "25.0 Gbps"; >> + case IBV_SPEED_HDR: return "50.0 Gbps"; >> + case IBV_SPEE_NDR: return "100.0 Gbps"; > > typo [DS]: I fixed this specifically, my bad while sending out. Will fix. > >> default: return "invalid speed"; >> } >> } >> diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h >> index b27dfd1..39a65fe 100644 >> --- a/libibverbs/verbs.h >> +++ b/libibverbs/verbs.h >> @@ -290,6 +290,24 @@ enum ibv_port_state { >> IBV_PORT_ACTIVE_DEFER = 5 >> }; >> >> +enum ibv_port_speed { >> + IBV_SPEED_SDR = 1, >> + IBV_SPEED_DDR = 2, >> + IBV_SPEED_QDR = 4, >> + IBV_SPEED_FDR10 = 8, >> + IBV_SPEED_FDR = 16, >> + IBV_SPEED_EDR = 32, >> + IBV_SPEED_HDR = 64, >> + IBV_SPEED_NDR = 128 > > HDR is almost defined now but NDR is not. Is it premature to put in NDR > now ? [DS]: Okay, will remove NDR for now. I saw HDR was added into kernel ib_verbs.h but not NDR, it makes perfect sense to remove it. > > Also, if either of these are being added, should 2x also be added ? > > -- Hal > >> +}; >> + >> +enum ibv_port_width { >> + IBV_WIDTH_1X = 1, >> + IBV_WIDTH_4X = 2, >> + IBV_WIDTH_8X = 4, >> + IBV_WIDTH_12X = 8 >> +}; >> + >> enum { >> IBV_LINK_LAYER_UNSPECIFIED, >> IBV_LINK_LAYER_INFINIBAND, >> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 8, 2017 at 7:35 PM, Dennis Dalessandro <dennis.dalessandro@intel.com> wrote: > On 05/08/2017 09:54 AM, Leon Romanovsky wrote: >> >> On Mon, May 08, 2017 at 09:44:17AM -0400, Devesh Sharma wrote: >>> >>> Currently application either need to define its own enum >>> values for link-speed and link-width or use hard coded >>> values. This patch inharits the enums from kernel space >>> ib_verbs.h and puts into verbs.h for ease of programming. >>> >>> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> >>> --- >>> libibverbs/examples/devinfo.c | 22 ++++++++++++---------- >>> libibverbs/verbs.h | 18 ++++++++++++++++++ >>> 2 files changed, 30 insertions(+), 10 deletions(-) >> >> >> I think the better approach will be move these defines from >> include/rdma/ib_verbs.h to include/uapi/rdma/ib_user_verbs.h >> and reuse ib_user_verbs.h file directly in libibverbs. > > > I agree on this. Please put this stuff in the uapi header file and include > that in user space. Done, will post a V1 with 2 patches... > > -Denny -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 08, 2017 at 08:17:06PM +0530, Devesh Sharma wrote: > On Mon, May 8, 2017 at 7:35 PM, Dennis Dalessandro > <dennis.dalessandro@intel.com> wrote: > > On 05/08/2017 09:54 AM, Leon Romanovsky wrote: > >> > >> On Mon, May 08, 2017 at 09:44:17AM -0400, Devesh Sharma wrote: > >>> > >>> Currently application either need to define its own enum > >>> values for link-speed and link-width or use hard coded > >>> values. This patch inharits the enums from kernel space > >>> ib_verbs.h and puts into verbs.h for ease of programming. > >>> > >>> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > >>> libibverbs/examples/devinfo.c | 22 ++++++++++++---------- > >>> libibverbs/verbs.h | 18 ++++++++++++++++++ > >>> 2 files changed, 30 insertions(+), 10 deletions(-) > >> > >> > >> I think the better approach will be move these defines from > >> include/rdma/ib_verbs.h to include/uapi/rdma/ib_user_verbs.h > >> and reuse ib_user_verbs.h file directly in libibverbs. > > > > > > I agree on this. Please put this stuff in the uapi header file and include > > that in user space. > > Done, will post a V1 with 2 patches... Relying on the kernel header from a uapi header means that we now require the distro to have a sufficient version of that kernel header. This might be OK, but you have to check to see what distros are including (cbuild --run-shell can help with this) Further, I seem to recall last time I lookd at this that using the kernel header was hard because there were lots of name conflicts that had to be resolved first. It would be great to see that done of course.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 8, 2017 at 9:42 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, May 08, 2017 at 08:17:06PM +0530, Devesh Sharma wrote: >> On Mon, May 8, 2017 at 7:35 PM, Dennis Dalessandro >> <dennis.dalessandro@intel.com> wrote: >> > On 05/08/2017 09:54 AM, Leon Romanovsky wrote: >> >> >> >> On Mon, May 08, 2017 at 09:44:17AM -0400, Devesh Sharma wrote: >> >>> >> >>> Currently application either need to define its own enum >> >>> values for link-speed and link-width or use hard coded >> >>> values. This patch inharits the enums from kernel space >> >>> ib_verbs.h and puts into verbs.h for ease of programming. >> >>> >> >>> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> >> >>> libibverbs/examples/devinfo.c | 22 ++++++++++++---------- >> >>> libibverbs/verbs.h | 18 ++++++++++++++++++ >> >>> 2 files changed, 30 insertions(+), 10 deletions(-) >> >> >> >> >> >> I think the better approach will be move these defines from >> >> include/rdma/ib_verbs.h to include/uapi/rdma/ib_user_verbs.h >> >> and reuse ib_user_verbs.h file directly in libibverbs. >> > >> > >> > I agree on this. Please put this stuff in the uapi header file and include >> > that in user space. >> >> Done, will post a V1 with 2 patches... > > Relying on the kernel header from a uapi header means that we now > require the distro to have a sufficient version of that kernel header. Which means we need to have a mechanism to track the versions. is relying on the ABI version is good enough or you see any blockers there.. > > This might be OK, but you have to check to see what distros are > including (cbuild --run-shell can help with this) And if a given distro is not including then fall-back to current scheme, is it okay to have defining/un-defining compile time macros to enable/disable certain code sections? > > Further, I seem to recall last time I lookd at this that using the > kernel header was hard because there were lots of name conflicts > that had to be resolved first. It would be great to see that done of > course.. Thanks for the heads-up, I am still trying to understand the complete problem, reviewing what we have currently I will get back if there are any questions. > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 08, 2017 at 10:30:29PM +0530, Devesh Sharma wrote: > > Relying on the kernel header from a uapi header means that we now > > require the distro to have a sufficient version of that kernel header. > Which means we need to have a mechanism to track the versions. > is relying on the ABI version is good enough or you see any blockers > there.. Realistically I think it means those distros will have to be de-supported. This might not be a problem, the uapi headers are fairly old now, you'll have to check.. If something really old like rhel6 has what you need then things are OK. > > This might be OK, but you have to check to see what distros are > > including (cbuild --run-shell can help with this) > And if a given distro is not including then fall-back to current > scheme, is it okay to have defining/un-defining compile time macros > to enable/disable certain code sections? No, generally not. This is too hard to do safely in public headers.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/libibverbs/examples/devinfo.c b/libibverbs/examples/devinfo.c index 2c1e3f1..42ed50c 100644 --- a/libibverbs/examples/devinfo.c +++ b/libibverbs/examples/devinfo.c @@ -124,10 +124,10 @@ static const char *mtu_str(enum ibv_mtu max_mtu) static const char *width_str(uint8_t width) { switch (width) { - case 1: return "1"; - case 2: return "4"; - case 4: return "8"; - case 8: return "12"; + case IBV_WIDTH_1X: return "1"; + case IBV_WIDTH_4X: return "4"; + case IBV_WIDTH_8X: return "8"; + case IBV_WIDTH_12X: return "12"; default: return "invalid width"; } } @@ -135,14 +135,16 @@ static const char *width_str(uint8_t width) static const char *speed_str(uint8_t speed) { switch (speed) { - case 1: return "2.5 Gbps"; - case 2: return "5.0 Gbps"; + case IBV_SPEED_SDR: return "2.5 Gbps"; + case IBV_SPEED_DDR: return "5.0 Gbps"; - case 4: /* fall through */ - case 8: return "10.0 Gbps"; + case IBV_SPEED_QDR: /* fall through */ + case IBV_SPEED_FDR10: return "10.0 Gbps"; - case 16: return "14.0 Gbps"; - case 32: return "25.0 Gbps"; + case IBV_SPEED_FDR: return "14.0 Gbps"; + case IBV_SPEED_EDR: return "25.0 Gbps"; + case IBV_SPEED_HDR: return "50.0 Gbps"; + case IBV_SPEE_NDR: return "100.0 Gbps"; default: return "invalid speed"; } } diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h index b27dfd1..39a65fe 100644 --- a/libibverbs/verbs.h +++ b/libibverbs/verbs.h @@ -290,6 +290,24 @@ enum ibv_port_state { IBV_PORT_ACTIVE_DEFER = 5 }; +enum ibv_port_speed { + IBV_SPEED_SDR = 1, + IBV_SPEED_DDR = 2, + IBV_SPEED_QDR = 4, + IBV_SPEED_FDR10 = 8, + IBV_SPEED_FDR = 16, + IBV_SPEED_EDR = 32, + IBV_SPEED_HDR = 64, + IBV_SPEED_NDR = 128 +}; + +enum ibv_port_width { + IBV_WIDTH_1X = 1, + IBV_WIDTH_4X = 2, + IBV_WIDTH_8X = 4, + IBV_WIDTH_12X = 8 +}; + enum { IBV_LINK_LAYER_UNSPECIFIED, IBV_LINK_LAYER_INFINIBAND,
Currently application either need to define its own enum values for link-speed and link-width or use hard coded values. This patch inharits the enums from kernel space ib_verbs.h and puts into verbs.h for ease of programming. Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> --- libibverbs/examples/devinfo.c | 22 ++++++++++++---------- libibverbs/verbs.h | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 10 deletions(-)