| Message ID | 20210617182511.1257629-3-Rao.Shoaib@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v1,1/3] RDMA/rxe: Increase value of RXE_MAX_UCONTEXT | expand |
On 6/17/21 1:25 PM, Rao Shoaib wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > > In our internal testing we have found that the > current limit is too small, this patch bumps it > up to a higher value required for our tests, which > are indicative of our customer usage. > > Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> > --- > drivers/infiniband/sw/rxe/rxe_param.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h > index 3b9b1ff4234f..d375f2cff484 100644 > --- a/drivers/infiniband/sw/rxe/rxe_param.h > +++ b/drivers/infiniband/sw/rxe/rxe_param.h > @@ -66,7 +66,7 @@ enum rxe_device_param { > RXE_MAX_MCAST_GRP = 8192, > RXE_MAX_MCAST_QP_ATTACH = 56, > RXE_MAX_TOT_MCAST_QP_ATTACH = 0x70000, > - RXE_MAX_AH = 100, > + RXE_MAX_AH = 64000, > RXE_MAX_SRQ = 17408, > RXE_MAX_SRQ_WR = 0x4000, > RXE_MIN_SRQ_WR = 1, > Interesting. There is no real reason to pick most of these values since it's just memory and does not reflect underlying hardware resources. It is advantageous to also be able to set them smaller to verify whether test cases correctly limit resources. It seems that there should be a way (module parameter or other) to adjust these values without having to recompile the driver. Thoughts? Regards, Bob Pearson
On 6/17/21 1:45 PM, Bob Pearson wrote: > On 6/17/21 1:25 PM, Rao Shoaib wrote: >> From: Rao Shoaib <rao.shoaib@oracle.com> >> >> In our internal testing we have found that the >> current limit is too small, this patch bumps it >> up to a higher value required for our tests, which >> are indicative of our customer usage. >> >> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> >> --- >> drivers/infiniband/sw/rxe/rxe_param.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h >> index 3b9b1ff4234f..d375f2cff484 100644 >> --- a/drivers/infiniband/sw/rxe/rxe_param.h >> +++ b/drivers/infiniband/sw/rxe/rxe_param.h >> @@ -66,7 +66,7 @@ enum rxe_device_param { >> RXE_MAX_MCAST_GRP = 8192, >> RXE_MAX_MCAST_QP_ATTACH = 56, >> RXE_MAX_TOT_MCAST_QP_ATTACH = 0x70000, >> - RXE_MAX_AH = 100, >> + RXE_MAX_AH = 64000, >> RXE_MAX_SRQ = 17408, >> RXE_MAX_SRQ_WR = 0x4000, >> RXE_MIN_SRQ_WR = 1, >> > Interesting. There is no real reason to pick most of these values since it's just memory and does not reflect underlying hardware resources. It is advantageous to also be able to set them smaller to verify whether test cases correctly limit resources. It seems that there should be a way (module parameter or other) to adjust these values without having to recompile the driver. Thoughts? I agree with you 100% but it seems like the original design did not intent to make them configurable at run time. I see that recently some other values were bumped up by others so I went with hard coded values. In the inhouse kernel version we used to decide on the values had them configurable because we did not know what values we wanted, and recompiling was time consuming. While some tests may require small values we want to test with lots of users/connections. If there is consensus I can submit a different patch to make these values configurable. In the meantime I hope we can bump up these values. Kindly let me know. Shoaib > > Regards, > > Bob Pearson
On 6/17/21 5:57 PM, Shoaib Rao wrote: > > On 6/17/21 1:45 PM, Bob Pearson wrote: >> On 6/17/21 1:25 PM, Rao Shoaib wrote: >>> From: Rao Shoaib <rao.shoaib@oracle.com> >>> >>> In our internal testing we have found that the >>> current limit is too small, this patch bumps it >>> up to a higher value required for our tests, which >>> are indicative of our customer usage. >>> >>> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> >>> --- >>> drivers/infiniband/sw/rxe/rxe_param.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h >>> index 3b9b1ff4234f..d375f2cff484 100644 >>> --- a/drivers/infiniband/sw/rxe/rxe_param.h >>> +++ b/drivers/infiniband/sw/rxe/rxe_param.h >>> @@ -66,7 +66,7 @@ enum rxe_device_param { >>> RXE_MAX_MCAST_GRP = 8192, >>> RXE_MAX_MCAST_QP_ATTACH = 56, >>> RXE_MAX_TOT_MCAST_QP_ATTACH = 0x70000, >>> - RXE_MAX_AH = 100, >>> + RXE_MAX_AH = 64000, >>> RXE_MAX_SRQ = 17408, >>> RXE_MAX_SRQ_WR = 0x4000, >>> RXE_MIN_SRQ_WR = 1, >>> >> Interesting. There is no real reason to pick most of these values since it's just memory and does not reflect underlying hardware resources. It is advantageous to also be able to set them smaller to verify whether test cases correctly limit resources. It seems that there should be a way (module parameter or other) to adjust these values without having to recompile the driver. Thoughts? > > I agree with you 100% but it seems like the original design did not intent to make them configurable at run time. I see that recently some other values were bumped up by others so I went with hard coded values. In the inhouse kernel version we used to decide on the values had them configurable because we did not know what values we wanted, and recompiling was time consuming. > > While some tests may require small values we want to test with lots of users/connections. > > If there is consensus I can submit a different patch to make these values configurable. In the meantime I hope we can bump up these values. > > Kindly let me know. > > Shoaib > >> >> Regards, >> >> Bob Pearson It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson
On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote:
> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson
Can we just delete the concept completely?
Jason
On 6/18/21 11:33 AM, Jason Gunthorpe wrote: > On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: > >> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson > > Can we just delete the concept completely? > > Jason > Not sure where you are headed here. Do you mean just lift the limits all together? That would not be in the spirit of the IBA even if that is not very meaningful. One of the useful things rxe does is to provide a cheap 'reference' implementation of the spec. If OTOH you want to put it in production then the limits are not very useful. Bob
On Fri, Jun 18, 2021 at 01:58:48PM -0500, Bob Pearson wrote: > On 6/18/21 11:33 AM, Jason Gunthorpe wrote: > > On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: > > > >> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson > > > > Can we just delete the concept completely? > > > > Jason > > > Not sure where you are headed here. Do you mean just lift the limits > all together? Yes.. The spec doesn't have like a UCONTEXT limit for instance, and real HW like mlx5 has huge reported limits anyhow. Jason
On 6/18/21 4:25 PM, Jason Gunthorpe wrote: > On Fri, Jun 18, 2021 at 01:58:48PM -0500, Bob Pearson wrote: >> On 6/18/21 11:33 AM, Jason Gunthorpe wrote: >>> On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: >>> >>>> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson >>> Can we just delete the concept completely? >>> >>> Jason >>> >> Not sure where you are headed here. Do you mean just lift the limits >> all together? > Yes.. The spec doesn't have like a UCONTEXT limit for instance, and > real HW like mlx5 has huge reported limits anyhow. > > Jason So do you want me to submit a patch that removes all limits or just these limits or something else? Shoaib
On 6/18/21 4:25 PM, Jason Gunthorpe wrote: > On Fri, Jun 18, 2021 at 01:58:48PM -0500, Bob Pearson wrote: >> On 6/18/21 11:33 AM, Jason Gunthorpe wrote: >>> On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: >>> >>>> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson >>> Can we just delete the concept completely? >>> >>> Jason >>> >> Not sure where you are headed here. Do you mean just lift the limits >> all together? > Yes.. The spec doesn't have like a UCONTEXT limit for instance, and > real HW like mlx5 has huge reported limits anyhow. These limits are reported via uverbs, so what do we report without current applications. Creating pool also requires limits but I guess we can use something like -1 to indicate there is no limit. I would have to look at all the values to see if we can implement this. Shoaib > > Jason
On 6/24/21 4:21 PM, Shoaib Rao wrote: > > On 6/18/21 4:25 PM, Jason Gunthorpe wrote: >> On Fri, Jun 18, 2021 at 01:58:48PM -0500, Bob Pearson wrote: >>> On 6/18/21 11:33 AM, Jason Gunthorpe wrote: >>>> On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: >>>> >>>>> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson >>>> Can we just delete the concept completely? >>>> >>>> Jason >>>> >>> Not sure where you are headed here. Do you mean just lift the limits >>> all together? >> Yes.. The spec doesn't have like a UCONTEXT limit for instance, and >> real HW like mlx5 has huge reported limits anyhow. > > These limits are reported via uverbs, so what do we report without current applications. Creating pool also requires limits but I guess we can use something like -1 to indicate there is no limit. I would have to look at all the values to see if we can implement this. > > Shoaib > > >> >> Jason The object create in pools (rxe_alloc_locked) just calls kzalloc for objects allocated by rxe and checks the limits. For objects allocated by rdma-core (__rxe_add_to_pool) it just checks the limits. The only place where the limit really matters is when a pool is indexed (RXE_POOL_INDEXED). Then there is a bitmask used to allocate the indices for the objects which consumes one byte for each 8 objects. From Jason's comment I think just setting the limits fairly large but not excessive is the right approach. Bob
On Fri, Jun 25, 2021 at 12:13:57AM -0500, Bob Pearson wrote: > On 6/24/21 4:21 PM, Shoaib Rao wrote: > > > > On 6/18/21 4:25 PM, Jason Gunthorpe wrote: > >> On Fri, Jun 18, 2021 at 01:58:48PM -0500, Bob Pearson wrote: > >>> On 6/18/21 11:33 AM, Jason Gunthorpe wrote: > >>>> On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: > >>>> > >>>>> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson > >>>> Can we just delete the concept completely? > >>>> > >>>> Jason > >>>> > >>> Not sure where you are headed here. Do you mean just lift the limits > >>> all together? > >> Yes.. The spec doesn't have like a UCONTEXT limit for instance, and > >> real HW like mlx5 has huge reported limits anyhow. > > > > These limits are reported via uverbs, so what do we report without current applications. Creating pool also requires limits but I guess we can use something like -1 to indicate there is no limit. I would have to look at all the values to see if we can implement this. > > > > Shoaib > > > > > >> > >> Jason > > The object create in pools (rxe_alloc_locked) just calls kzalloc for > objects allocated by rxe and checks the limits. For objects > allocated by rdma-core (__rxe_add_to_pool) it just checks the > limits. The only place where the limit really matters is when a pool > is indexed (RXE_POOL_INDEXED). Then there is a bitmask used to > allocate the indices for the objects which consumes one byte for > each 8 objects. Use an ida or xarray instead? Jason
On 6/25/21 5:49 AM, Jason Gunthorpe wrote: > On Fri, Jun 25, 2021 at 12:13:57AM -0500, Bob Pearson wrote: >> On 6/24/21 4:21 PM, Shoaib Rao wrote: >>> On 6/18/21 4:25 PM, Jason Gunthorpe wrote: >>>> On Fri, Jun 18, 2021 at 01:58:48PM -0500, Bob Pearson wrote: >>>>> On 6/18/21 11:33 AM, Jason Gunthorpe wrote: >>>>>> On Thu, Jun 17, 2021 at 10:56:58PM -0500, Bob Pearson wrote: >>>>>> >>>>>>> It isn't my call. But I am in favor of tunable parameters. -- Bob Pearson >>>>>> Can we just delete the concept completely? >>>>>> >>>>>> Jason >>>>>> >>>>> Not sure where you are headed here. Do you mean just lift the limits >>>>> all together? >>>> Yes.. The spec doesn't have like a UCONTEXT limit for instance, and >>>> real HW like mlx5 has huge reported limits anyhow. >>> These limits are reported via uverbs, so what do we report without current applications. Creating pool also requires limits but I guess we can use something like -1 to indicate there is no limit. I would have to look at all the values to see if we can implement this. >>> >>> Shoaib >>> >>> >>>> Jason >> The object create in pools (rxe_alloc_locked) just calls kzalloc for >> objects allocated by rxe and checks the limits. For objects >> allocated by rdma-core (__rxe_add_to_pool) it just checks the >> limits. The only place where the limit really matters is when a pool >> is indexed (RXE_POOL_INDEXED). Then there is a bitmask used to >> allocate the indices for the objects which consumes one byte for >> each 8 objects. > Use an ida or xarray instead? > > Jason We do not have to change any data structures, just decide on the max limit. RXE_POOL_INDEX flag as shown below is used to indicate what type of index to use and is not an issue if (rxe_type_info[type].flags & RXE_POOL_INDEX) { err = rxe_pool_init_index(pool, rxe_type_info[type].max_index, rxe_type_info[type].min_index); if (err) goto out; } And static int rxe_pool_init_index(struct rxe_pool *pool, u32 max, u32 min) { So the maximum index can be U32_MAX or for now we can limit is to (1 << 20) i.e 1048576 I am assuming that changing the limits for the objects I listed earlier (list below) is fine. Shoaib RXE_MAX_QP RXE_MAX_QP_WR RXE_MAX_CQ RXE_MAX_MR RXE_MAX_PD RXE_MAX_AH RXE_MAX_FMR RXE_MAX_SRQ RXE_MAX_SRQ_WR RXE_MAX_PKEYS RXE_MAX_UCONTEXT
diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h index 3b9b1ff4234f..d375f2cff484 100644 --- a/drivers/infiniband/sw/rxe/rxe_param.h +++ b/drivers/infiniband/sw/rxe/rxe_param.h @@ -66,7 +66,7 @@ enum rxe_device_param { RXE_MAX_MCAST_GRP = 8192, RXE_MAX_MCAST_QP_ATTACH = 56, RXE_MAX_TOT_MCAST_QP_ATTACH = 0x70000, - RXE_MAX_AH = 100, + RXE_MAX_AH = 64000, RXE_MAX_SRQ = 17408, RXE_MAX_SRQ_WR = 0x4000, RXE_MIN_SRQ_WR = 1,