| Message ID | 1574106879-19211-2-git-send-email-rao.shoaib@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | rxe should use same buffer size for SGE's and inline data | expand |
On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > > Introduce maximum WQE size to impose limits on max SGE's and inline data > > Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> > drivers/infiniband/sw/rxe/rxe_param.h | 3 ++- > drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h > index 1b596fb..31fb5c7 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_param.h > @@ -68,7 +68,6 @@ enum rxe_device_param { > RXE_HW_VER = 0, > RXE_MAX_QP = 0x10000, > RXE_MAX_QP_WR = 0x4000, > - RXE_MAX_INLINE_DATA = 400, > RXE_DEVICE_CAP_FLAGS = IB_DEVICE_BAD_PKEY_CNTR > | IB_DEVICE_BAD_QKEY_CNTR > | IB_DEVICE_AUTO_PATH_MIG > @@ -79,7 +78,9 @@ enum rxe_device_param { > | IB_DEVICE_RC_RNR_NAK_GEN > | IB_DEVICE_SRQ_RESIZE > | IB_DEVICE_MEM_MGT_EXTENSIONS, > + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */ This shouldn't just be a random constant, I think you are trying to say: RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE Just say that > RXE_MAX_SGE = 32, > + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE, This is mixed up now, it should be RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe) > diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c > index aeea994..323e43d 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_qp.c > @@ -78,9 +78,12 @@ static int rxe_qp_chk_cap(struct rxe_dev *rxe, struct ib_qp_cap *cap, > } > } > > - if (cap->max_inline_data > rxe->max_inline_data) { > + if (cap->max_inline_data > > + rxe->max_inline_data - sizeof(struct rxe_send_wqe)) { > pr_warn("invalid max inline data = %d > %d\n", > - cap->max_inline_data, rxe->max_inline_data); > + cap->max_inline_data, > + rxe->max_inline_data - > + (u32)sizeof(struct rxe_send_wqe)); Then this isn't needed And this code in the other patch: + wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge), + init->cap.max_inline_data); + qp->sq.max_sge = wqe_size/sizeof(struct ib_sge); + qp->sq.max_inline = wqe_size; Makes sense as both max_inline_data and 'init->cap.max_send_sge * sizeof(struct ib_sge)' are exclusive of the wqe header Jason
On 11/19/19 12:31 PM, Jason Gunthorpe wrote: > On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote: >> From: Rao Shoaib <rao.shoaib@oracle.com> >> >> Introduce maximum WQE size to impose limits on max SGE's and inline data >> >> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> >> drivers/infiniband/sw/rxe/rxe_param.h | 3 ++- >> drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++-- >> 2 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h >> index 1b596fb..31fb5c7 100644 >> +++ b/drivers/infiniband/sw/rxe/rxe_param.h >> @@ -68,7 +68,6 @@ enum rxe_device_param { >> RXE_HW_VER = 0, >> RXE_MAX_QP = 0x10000, >> RXE_MAX_QP_WR = 0x4000, >> - RXE_MAX_INLINE_DATA = 400, >> RXE_DEVICE_CAP_FLAGS = IB_DEVICE_BAD_PKEY_CNTR >> | IB_DEVICE_BAD_QKEY_CNTR >> | IB_DEVICE_AUTO_PATH_MIG >> @@ -79,7 +78,9 @@ enum rxe_device_param { >> | IB_DEVICE_RC_RNR_NAK_GEN >> | IB_DEVICE_SRQ_RESIZE >> | IB_DEVICE_MEM_MGT_EXTENSIONS, >> + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */ > This shouldn't just be a random constant, I think you are trying to > say: > > RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE I thought you wanted this value to be independent of RXE_MAX_SGE, else why are defining it. > > Just say that > >> RXE_MAX_SGE = 32, >> + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE, > This is mixed up now, it should be > > RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe) I agree to what you are suggesting, it will make the current patch better. However, In my previous patch I had RXE_MAX_INLINE_DATA = RXE_MAX_SGE * sizeof(struct ib_sge) IMHO that conveys the intent much better. I do not see the reason for defining RXE_MAX_WQE_SIZE, ib_device_attr does not even have an entry for it and hence the value is not used anywhere by rxe or by any other relevant driver. I will re-submit with the changes you have suggested. Shoaib > >> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c >> index aeea994..323e43d 100644 >> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c >> @@ -78,9 +78,12 @@ static int rxe_qp_chk_cap(struct rxe_dev *rxe, struct ib_qp_cap *cap, >> } >> } >> >> - if (cap->max_inline_data > rxe->max_inline_data) { >> + if (cap->max_inline_data > >> + rxe->max_inline_data - sizeof(struct rxe_send_wqe)) { >> pr_warn("invalid max inline data = %d > %d\n", >> - cap->max_inline_data, rxe->max_inline_data); >> + cap->max_inline_data, >> + rxe->max_inline_data - >> + (u32)sizeof(struct rxe_send_wqe)); > Then this isn't needed > > And this code in the other patch: > > + wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge), > + init->cap.max_inline_data); > + qp->sq.max_sge = wqe_size/sizeof(struct ib_sge); > + qp->sq.max_inline = wqe_size; > > Makes sense as both max_inline_data and 'init->cap.max_send_sge * > sizeof(struct ib_sge)' are exclusive of the wqe header > > Jason
On Tue, Nov 19, 2019 at 02:38:23PM -0800, Rao Shoaib wrote: > > On 11/19/19 12:31 PM, Jason Gunthorpe wrote: > > On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote: > > > From: Rao Shoaib <rao.shoaib@oracle.com> > > > > > > Introduce maximum WQE size to impose limits on max SGE's and inline data > > > > > > Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> > > > drivers/infiniband/sw/rxe/rxe_param.h | 3 ++- > > > drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++-- > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h > > > index 1b596fb..31fb5c7 100644 > > > +++ b/drivers/infiniband/sw/rxe/rxe_param.h > > > @@ -68,7 +68,6 @@ enum rxe_device_param { > > > RXE_HW_VER = 0, > > > RXE_MAX_QP = 0x10000, > > > RXE_MAX_QP_WR = 0x4000, > > > - RXE_MAX_INLINE_DATA = 400, > > > RXE_DEVICE_CAP_FLAGS = IB_DEVICE_BAD_PKEY_CNTR > > > | IB_DEVICE_BAD_QKEY_CNTR > > > | IB_DEVICE_AUTO_PATH_MIG > > > @@ -79,7 +78,9 @@ enum rxe_device_param { > > > | IB_DEVICE_RC_RNR_NAK_GEN > > > | IB_DEVICE_SRQ_RESIZE > > > | IB_DEVICE_MEM_MGT_EXTENSIONS, > > > + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */ > > This shouldn't just be a random constant, I think you are trying to > > say: > > > > RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE > I thought you wanted this value to be independent of RXE_MAX_SGE, else why > are defining it. Then define RXE_MAX_SGE = (RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe))/sizeof(rxe_sge) And drive everything off RXE_MAX_WQE_SIZE, which sounds good > > Just say that > > > > > RXE_MAX_SGE = 32, > > > + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE, > > This is mixed up now, it should be > > > > RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe) > > I agree to what you are suggesting, it will make the current patch better. > However, In my previous patch I had > > RXE_MAX_INLINE_DATA = RXE_MAX_SGE * sizeof(struct ib_sge) > > IMHO that conveys the intent much better. I do not see the reason for > defining RXE_MAX_WQE_SIZE, ib_device_attr does not even have an entry for it > and hence the value is not used anywhere by rxe or by any other relevant > driver. Because WQE_SIZE is what you are actually concerned with here, using MAX_SGE as a proxy for the max WQE is confusing Jason
On 11/19/19 3:13 PM, Jason Gunthorpe wrote: > On Tue, Nov 19, 2019 at 02:38:23PM -0800, Rao Shoaib wrote: >> On 11/19/19 12:31 PM, Jason Gunthorpe wrote: >>> On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote: >>>> From: Rao Shoaib <rao.shoaib@oracle.com> >>>> >>>> Introduce maximum WQE size to impose limits on max SGE's and inline data >>>> >>>> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com> >>>> drivers/infiniband/sw/rxe/rxe_param.h | 3 ++- >>>> drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++-- >>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h >>>> index 1b596fb..31fb5c7 100644 >>>> +++ b/drivers/infiniband/sw/rxe/rxe_param.h >>>> @@ -68,7 +68,6 @@ enum rxe_device_param { >>>> RXE_HW_VER = 0, >>>> RXE_MAX_QP = 0x10000, >>>> RXE_MAX_QP_WR = 0x4000, >>>> - RXE_MAX_INLINE_DATA = 400, >>>> RXE_DEVICE_CAP_FLAGS = IB_DEVICE_BAD_PKEY_CNTR >>>> | IB_DEVICE_BAD_QKEY_CNTR >>>> | IB_DEVICE_AUTO_PATH_MIG >>>> @@ -79,7 +78,9 @@ enum rxe_device_param { >>>> | IB_DEVICE_RC_RNR_NAK_GEN >>>> | IB_DEVICE_SRQ_RESIZE >>>> | IB_DEVICE_MEM_MGT_EXTENSIONS, >>>> + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */ >>> This shouldn't just be a random constant, I think you are trying to >>> say: >>> >>> RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE >> I thought you wanted this value to be independent of RXE_MAX_SGE, else why >> are defining it. > Then define > > RXE_MAX_SGE = (RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe))/sizeof(rxe_sge) > > And drive everything off RXE_MAX_WQE_SIZE, which sounds good > >>> Just say that >>> >>>> RXE_MAX_SGE = 32, >>>> + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE, >>> This is mixed up now, it should be >>> >>> RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe) >> I agree to what you are suggesting, it will make the current patch better. >> However, In my previous patch I had >> >> RXE_MAX_INLINE_DATA = RXE_MAX_SGE * sizeof(struct ib_sge) >> >> IMHO that conveys the intent much better. I do not see the reason for >> defining RXE_MAX_WQE_SIZE, ib_device_attr does not even have an entry for it >> and hence the value is not used anywhere by rxe or by any other relevant >> driver. > Because WQE_SIZE is what you are actually concerned with here, using > MAX_SGE as a proxy for the max WQE is confusing > > Jason My intent is that we calculate and use the maximum buffer size using the maximum of, number of SGE's and inline data requested, not controlling the size of WQE buffer. If I was trying to limit WQE size I would agree with you. Defining MAX_WQE_SIZE based on MAX_SGE and recalculating MAX_SGE does not make sense to me. MAX_SGE and inline_data are independent variables and define the size of wqe size not the other wise around. I did make inline_dependent on MAX_SGE. Your and my preferences can be different but you are the maintainer and what you say goes. We have had a good discussion and I am going with your previous suggestion. Kind Regards, Shoaib
On Tue, Nov 19, 2019 at 03:55:35PM -0800, Rao Shoaib wrote: > My intent is that we calculate and use the maximum buffer size using the > maximum of, number of SGE's and inline data requested, not controlling the > size of WQE buffer. If I was trying to limit WQE size I would agree with > you. Defining MAX_WQE_SIZE based on MAX_SGE and recalculating MAX_SGE does > not make sense to me. MAX_SGE and inline_data are independent variables and > define the size of wqe size not the other wise around. I did make > inline_dependent on MAX_SGE. What you are trying to do is limit the size of the WQE to some maximum and from there you can compute the upper limit on the SGE and the inline data arrays, depending on how the WQE is being used. If a limit must be had then the limit is the WQE size. It is also reasonable to ask why rxe has a limit at all, or why the limit is so small ie why can't it be 2k or something? But that is something else Jason
Any update on my patch? If there is some change needed please let me know. Shoaib On 11/19/19 4:08 PM, Jason Gunthorpe wrote: > On Tue, Nov 19, 2019 at 03:55:35PM -0800, Rao Shoaib wrote: > >> My intent is that we calculate and use the maximum buffer size using the >> maximum of, number of SGE's and inline data requested, not controlling the >> size of WQE buffer. If I was trying to limit WQE size I would agree with >> you. Defining MAX_WQE_SIZE based on MAX_SGE and recalculating MAX_SGE does >> not make sense to me. MAX_SGE and inline_data are independent variables and >> define the size of wqe size not the other wise around. I did make >> inline_dependent on MAX_SGE. > What you are trying to do is limit the size of the WQE to some maximum > and from there you can compute the upper limit on the SGE and the > inline data arrays, depending on how the WQE is being used. > > If a limit must be had then the limit is the WQE size. It is also > reasonable to ask why rxe has a limit at all, or why the limit is so > small ie why can't it be 2k or something? But that is something else > > Jason
On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote: > Any update on my patch? > > If there is some change needed please let me know. You need to repost it with the comments addressed https://patchwork.kernel.org/patch/11250179/ Jason
Hi Jason, I thought I had addressed the comments and literally did what you suggested. Sorry if I missed something, can you please point it out. Shoaib On 12/19/19 10:25 AM, Jason Gunthorpe wrote: > On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote: >> Any update on my patch? >> >> If there is some change needed please let me know. > You need to repost it with the comments addressed > > https://patchwork.kernel.org/patch/11250179/ > > Jason >
On 12/19/19 10:25 AM, Jason Gunthorpe wrote: > On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote: >> Any update on my patch? >> >> If there is some change needed please let me know. > You need to repost it with the comments addressed > > https://patchwork.kernel.org/patch/11250179/ > > Jason > Jason, Following is a pointer to the patch that I posted in response to your comments https://www.spinics.net/lists/linux-rdma/msg86241.html I posted this on Nov 18. Can you please take a look and let me know what else has to be done. Shoaib
On Mon, Jan 13, 2020 at 10:35:14AM -0800, Rao Shoaib wrote: > > On 12/19/19 10:25 AM, Jason Gunthorpe wrote: > > On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote: > > > Any update on my patch? > > > > > > If there is some change needed please let me know. > > You need to repost it with the comments addressed > > > > https://patchwork.kernel.org/patch/11250179/ > > > > Jason > > > Jason, > > Following is a pointer to the patch that I posted in response to your > comments > > https://www.spinics.net/lists/linux-rdma/msg86241.html > > I posted this on Nov 18. Can you please take a look and let me know what > else has to be done. You mean this: https://www.spinics.net/lists/linux-rdma/msg86333.html ? Don't mix the inline size and the # SGEs. They both drive the maximum WQE size and all the math should be directly connected. Jason
On 1/13/20 10:47 AM, Jason Gunthorpe wrote: > On Mon, Jan 13, 2020 at 10:35:14AM -0800, Rao Shoaib wrote: >> On 12/19/19 10:25 AM, Jason Gunthorpe wrote: >>> On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote: >>>> Any update on my patch? >>>> >>>> If there is some change needed please let me know. >>> You need to repost it with the comments addressed >>> >>> https://patchwork.kernel.org/patch/11250179/ >>> >>> Jason >>> >> Jason, >> >> Following is a pointer to the patch that I posted in response to your >> comments >> >> https://www.spinics.net/lists/linux-rdma/msg86241.html >> >> I posted this on Nov 18. Can you please take a look and let me know what >> else has to be done. > You mean this: > > https://www.spinics.net/lists/linux-rdma/msg86333.html > > ? > > Don't mix the inline size and the # SGEs. They both drive the maximum > WQE size and all the math should be directly connected. > > Jason Nope. I have just resent the patch. Sorry for the confusion. Shoaib.
diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h index 1b596fb..31fb5c7 100644 --- a/drivers/infiniband/sw/rxe/rxe_param.h +++ b/drivers/infiniband/sw/rxe/rxe_param.h @@ -68,7 +68,6 @@ enum rxe_device_param { RXE_HW_VER = 0, RXE_MAX_QP = 0x10000, RXE_MAX_QP_WR = 0x4000, - RXE_MAX_INLINE_DATA = 400, RXE_DEVICE_CAP_FLAGS = IB_DEVICE_BAD_PKEY_CNTR | IB_DEVICE_BAD_QKEY_CNTR | IB_DEVICE_AUTO_PATH_MIG @@ -79,7 +78,9 @@ enum rxe_device_param { | IB_DEVICE_RC_RNR_NAK_GEN | IB_DEVICE_SRQ_RESIZE | IB_DEVICE_MEM_MGT_EXTENSIONS, + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */ RXE_MAX_SGE = 32, + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE, RXE_MAX_SGE_RD = 32, RXE_MAX_CQ = 16384, RXE_MAX_LOG_CQE = 15, diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c index aeea994..323e43d 100644 --- a/drivers/infiniband/sw/rxe/rxe_qp.c +++ b/drivers/infiniband/sw/rxe/rxe_qp.c @@ -78,9 +78,12 @@ static int rxe_qp_chk_cap(struct rxe_dev *rxe, struct ib_qp_cap *cap, } } - if (cap->max_inline_data > rxe->max_inline_data) { + if (cap->max_inline_data > + rxe->max_inline_data - sizeof(struct rxe_send_wqe)) { pr_warn("invalid max inline data = %d > %d\n", - cap->max_inline_data, rxe->max_inline_data); + cap->max_inline_data, + rxe->max_inline_data - + (u32)sizeof(struct rxe_send_wqe)); goto err1; }