[PATCH] rdma-core: Make room for source GIDs to be added

[PATCH] rdma-core: Make room for source GIDs to be added

[Somnath Kotur]

Currently sgid_index is of type 'u8' so we can't really go beyond 128
unique GID entries(one for each type so total of 256 GID entries).
Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
can be accomodated.

Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 2 +-
 include/rdma/ib_verbs.h         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 4792f52..3161e2c 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -549,7 +549,7 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
 		flow_class = be32_to_cpu(grh->version_tclass_flow);
 		rdma_ah_set_grh(ah_attr, &sgid,
 				flow_class & 0xFFFFF,
-				(u8)gid_index, hoplimit,
+				(u16)gid_index, hoplimit,
 				(flow_class >> 20) & 0xFF);
 
 	}
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index ba8314e..30ce942 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -639,7 +639,7 @@ struct ib_event_handler {
 struct ib_global_route {
 	union ib_gid	dgid;
 	u32		flow_label;
-	u8		sgid_index;
+	u16		sgid_index;
 	u8		hop_limit;
 	u8		traffic_class;
 };
@@ -3612,7 +3612,7 @@ static inline void rdma_ah_set_interface_id(struct rdma_ah_attr *attr,
 
 static inline void rdma_ah_set_grh(struct rdma_ah_attr *attr,
 				   union ib_gid *dgid, u32 flow_label,
-				   u8 sgid_index, u8 hop_limit,
+				   u16 sgid_index, u8 hop_limit,
 				   u8 traffic_class)
 {
 	struct ib_global_route *grh = rdma_ah_retrieve_grh(attr);
-- 
1.8.3.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-core: Make room for source GIDs to be added

[Leon Romanovsky]

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

On Tue, Jun 13, 2017 at 08:24:51AM +0530, Somnath Kotur wrote:
> Currently sgid_index is of type 'u8' so we can't really go beyond 128
> unique GID entries(one for each type so total of 256 GID entries).
> Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
> can be accomodated.
>
> Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/core/verbs.c | 2 +-
>  include/rdma/ib_verbs.h         | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

What type of testing did this change get?
According to spec, "The maximum number (N) of unicast GIDs supported per endport is
implementation specific".

mlx5 and mlx4 are limited to 128 GIDs and user will get errors if he try to set
more than that.

>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 4792f52..3161e2c 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -549,7 +549,7 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
>  		flow_class = be32_to_cpu(grh->version_tclass_flow);
>  		rdma_ah_set_grh(ah_attr, &sgid,
>  				flow_class & 0xFFFFF,
> -				(u8)gid_index, hoplimit,
> +				(u16)gid_index, hoplimit,
>  				(flow_class >> 20) & 0xFF);
>
>  	}
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index ba8314e..30ce942 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -639,7 +639,7 @@ struct ib_event_handler {
>  struct ib_global_route {
>  	union ib_gid	dgid;
>  	u32		flow_label;
> -	u8		sgid_index;
> +	u16		sgid_index;
>  	u8		hop_limit;
>  	u8		traffic_class;
>  };
> @@ -3612,7 +3612,7 @@ static inline void rdma_ah_set_interface_id(struct rdma_ah_attr *attr,
>
>  static inline void rdma_ah_set_grh(struct rdma_ah_attr *attr,
>  				   union ib_gid *dgid, u32 flow_label,
> -				   u8 sgid_index, u8 hop_limit,
> +				   u16 sgid_index, u8 hop_limit,
>  				   u8 traffic_class)
>  {
>  	struct ib_global_route *grh = rdma_ah_retrieve_grh(attr);
> --
> 1.8.3.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

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

Re: [PATCH] rdma-core: Make room for source GIDs to be added

[Yuval Shaia]

On Tue, Jun 13, 2017 at 08:24:51AM +0530, Somnath Kotur wrote:
> Currently sgid_index is of type 'u8' so we can't really go beyond 128
> unique GID entries(one for each type so total of 256 GID entries).
> Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
> can be accomodated.
> 
> Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/core/verbs.c | 2 +-
>  include/rdma/ib_verbs.h         | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

I would assume this kind of change has a bit wider scope, i.e. what about
all other places such as ib_user_mad_hdr, ib_uverbs_qp_dest etc?

> 
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 4792f52..3161e2c 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -549,7 +549,7 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
>  		flow_class = be32_to_cpu(grh->version_tclass_flow);
>  		rdma_ah_set_grh(ah_attr, &sgid,
>  				flow_class & 0xFFFFF,
> -				(u8)gid_index, hoplimit,
> +				(u16)gid_index, hoplimit,
>  				(flow_class >> 20) & 0xFF);
>  
>  	}
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index ba8314e..30ce942 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -639,7 +639,7 @@ struct ib_event_handler {
>  struct ib_global_route {
>  	union ib_gid	dgid;
>  	u32		flow_label;
> -	u8		sgid_index;
> +	u16		sgid_index;
>  	u8		hop_limit;
>  	u8		traffic_class;
>  };
> @@ -3612,7 +3612,7 @@ static inline void rdma_ah_set_interface_id(struct rdma_ah_attr *attr,
>  
>  static inline void rdma_ah_set_grh(struct rdma_ah_attr *attr,
>  				   union ib_gid *dgid, u32 flow_label,
> -				   u8 sgid_index, u8 hop_limit,
> +				   u16 sgid_index, u8 hop_limit,
>  				   u8 traffic_class)
>  {
>  	struct ib_global_route *grh = rdma_ah_retrieve_grh(attr);
> -- 
> 1.8.3.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-core: Make room for source GIDs to be added

[Jason Gunthorpe]

On Tue, Jun 13, 2017 at 09:03:25AM +0300, Leon Romanovsky wrote:
> On Tue, Jun 13, 2017 at 08:24:51AM +0530, Somnath Kotur wrote:
> > Currently sgid_index is of type 'u8' so we can't really go beyond 128
> > unique GID entries(one for each type so total of 256 GID entries).
> > Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
> > can be accomodated.
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/verbs.c | 2 +-
> >  include/rdma/ib_verbs.h         | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> What type of testing did this change get?
> According to spec, "The maximum number (N) of unicast GIDs supported per endport is
> implementation specific".
> 
> mlx5 and mlx4 are limited to 128 GIDs and user will get errors if he try to set
> more than that.

At a minimum I think this patch will need to add overflow protection
on the uapi paths that cast this back to a u8, and Matan should
consider expanding sgid_index to u16 in the new uapi, whenever
possible..

.. and Somnath you should review Matan's uABI patches since you want
such a fundamental change :)

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-core: Make room for source GIDs to be added

[Somnath Kotur]

Hi Leon,

On Tue, Jun 13, 2017 at 11:33 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Jun 13, 2017 at 08:24:51AM +0530, Somnath Kotur wrote:
>> Currently sgid_index is of type 'u8' so we can't really go beyond 128
>> unique GID entries(one for each type so total of 256 GID entries).
>> Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
>> can be accomodated.
>>
>> Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>>  drivers/infiniband/core/verbs.c | 2 +-
>>  include/rdma/ib_verbs.h         | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> What type of testing did this change get?
> According to spec, "The maximum number (N) of unicast GIDs supported per endport is
> implementation specific".
>

> mlx5 and mlx4 are limited to 128 GIDs and user will get errors if he try to set
> more than that.
>
Agree with your points, and yes this was tested against an adapter
that could support more than 128 GIDs

Thanks
Som
>>
>> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
>> index 4792f52..3161e2c 100644
>> --- a/drivers/infiniband/core/verbs.c
>> +++ b/drivers/infiniband/core/verbs.c
>> @@ -549,7 +549,7 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
>>               flow_class = be32_to_cpu(grh->version_tclass_flow);
>>               rdma_ah_set_grh(ah_attr, &sgid,
>>                               flow_class & 0xFFFFF,
>> -                             (u8)gid_index, hoplimit,
>> +                             (u16)gid_index, hoplimit,
>>                               (flow_class >> 20) & 0xFF);
>>
>>       }
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index ba8314e..30ce942 100644
>> --- a/include/rdma/ib_verbs.h
>> +++ b/include/rdma/ib_verbs.h
>> @@ -639,7 +639,7 @@ struct ib_event_handler {
>>  struct ib_global_route {
>>       union ib_gid    dgid;
>>       u32             flow_label;
>> -     u8              sgid_index;
>> +     u16             sgid_index;
>>       u8              hop_limit;
>>       u8              traffic_class;
>>  };
>> @@ -3612,7 +3612,7 @@ static inline void rdma_ah_set_interface_id(struct rdma_ah_attr *attr,
>>
>>  static inline void rdma_ah_set_grh(struct rdma_ah_attr *attr,
>>                                  union ib_gid *dgid, u32 flow_label,
>> -                                u8 sgid_index, u8 hop_limit,
>> +                                u16 sgid_index, u8 hop_limit,
>>                                  u8 traffic_class)
>>  {
>>       struct ib_global_route *grh = rdma_ah_retrieve_grh(attr);
>> --
>> 1.8.3.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-core: Make room for source GIDs to be added

[Somnath Kotur]

On Wed, Jun 14, 2017 at 12:50 AM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Tue, Jun 13, 2017 at 09:03:25AM +0300, Leon Romanovsky wrote:
>> On Tue, Jun 13, 2017 at 08:24:51AM +0530, Somnath Kotur wrote:
>> > Currently sgid_index is of type 'u8' so we can't really go beyond 128
>> > unique GID entries(one for each type so total of 256 GID entries).
>> > Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
>> > can be accomodated.
>> >
>> > Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> >  drivers/infiniband/core/verbs.c | 2 +-
>> >  include/rdma/ib_verbs.h         | 4 ++--
>> >  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> What type of testing did this change get?
>> According to spec, "The maximum number (N) of unicast GIDs supported per endport is
>> implementation specific".
>>
>> mlx5 and mlx4 are limited to 128 GIDs and user will get errors if he try to set
>> more than that.
>
> At a minimum I think this patch will need to add overflow protection
> on the uapi paths that cast this back to a u8, and Matan should
> consider expanding sgid_index to u16 in the new uapi, whenever
> possible..
>
> .. and Somnath you should review Matan's uABI patches since you want
> such a fundamental change :)
>
> Jason
Sure, thanks for your comments Jason. Will check with Matan on this

Thanks
Som
--
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-core: Make room for source GIDs to be added

[Matan Barak]

On Wed, Jun 14, 2017 at 7:26 AM, Somnath Kotur
<somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On Wed, Jun 14, 2017 at 12:50 AM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>> On Tue, Jun 13, 2017 at 09:03:25AM +0300, Leon Romanovsky wrote:
>>> On Tue, Jun 13, 2017 at 08:24:51AM +0530, Somnath Kotur wrote:
>>> > Currently sgid_index is of type 'u8' so we can't really go beyond 128
>>> > unique GID entries(one for each type so total of 256 GID entries).
>>> > Change it to u16 so atleast 4k GID entries (as many VLANs that can be added)
>>> > can be accomodated.
>>> >
>>> > Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> >  drivers/infiniband/core/verbs.c | 2 +-
>>> >  include/rdma/ib_verbs.h         | 4 ++--
>>> >  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> What type of testing did this change get?
>>> According to spec, "The maximum number (N) of unicast GIDs supported per endport is
>>> implementation specific".
>>>
>>> mlx5 and mlx4 are limited to 128 GIDs and user will get errors if he try to set
>>> more than that.
>>
>> At a minimum I think this patch will need to add overflow protection
>> on the uapi paths that cast this back to a u8, and Matan should
>> consider expanding sgid_index to u16 in the new uapi, whenever
>> possible..
>>

Yep, let's get the infrastructure in and then discuss verb-by-verb basis.
Seems very reasonable extending it to 16bit.

>> .. and Somnath you should review Matan's uABI patches since you want
>> such a fundamental change :)
>>
>> Jason
> Sure, thanks for your comments Jason. Will check with Matan on this
>

In (very) short - we propose a new ioctl based ABI that uses
attributes (instead of one struct-per-command).
This is easier to extend and more flexible than the current schema.
Since we change the whole ABI, we could think of fields which should
be extended and apply that change to the new ABI.
You could review this work [1].

> Thanks
> Som
> --

Regards,
Matan

[1] https://www.spinics.net/lists/linux-rdma/msg50631.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
--
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