| Message ID | 1484588681-12987-2-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> For devices that can register page list that is bigger than > USHRT_MAX, we actually take the wrong value for sg_tablesize. > E.g: for CX4 max_fast_reg_page_list_len is 65536 (bigger than USHRT_MAX) > so we set sg_tablesize to 0 by mistake. Therefore, each IO that is > bigger than 4k splitted to "< 4k" chunks that cause performance degredation. > > Fixes: 7854550ae6d8 ("RDMA/iser: Limit sgs to the device fastreg depth") > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 9104e6b..ff37f19 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -656,7 +656,8 @@ static void iscsi_iser_cleanup_task(struct iscsi_task *task) > * max fastreg page list length. > */ Hi Max, > shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize, > - ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len); > + min_t(unsigned int, USHRT_MAX, > + ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len)); Hmm, This looks broken since I fixed up mlx5 capability at: 911f4331bc87 ("IB/mlx5: Expose correct max_fast_reg_page_list_len") However, I suspect this assignment should be removed altogether because we already take that into account in the address resolution handler, there the min statement is for unsigned and casted back to ushrt. -- 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/drivers/infiniband/ulp/iser/iscsi_iser.c > b/drivers/infiniband/ulp/iser/iscsi_iser.c > > index 9104e6b..ff37f19 100644 > > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > > @@ -656,7 +656,8 @@ static void iscsi_iser_cleanup_task(struct iscsi_task *task) > > * max fastreg page list length. > > */ > > Hi Max, > > > shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize, > > - ib_conn->device->ib_device- > >attrs.max_fast_reg_page_list_len); > > + min_t(unsigned int, USHRT_MAX, > > + ib_conn->device->ib_device- > >attrs.max_fast_reg_page_list_len)); > > Hmm, This looks broken since I fixed up mlx5 capability at: > 911f4331bc87 ("IB/mlx5: Expose correct max_fast_reg_page_list_len") > > However, I suspect this assignment should be removed altogether because > we already take that into account in the address resolution handler, > there the min statement is for unsigned and casted back to ushrt. Hey Sagi, where is the code to which you refer? "address resolution handler?" Thanks, Steve -- 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
> Hey Sagi, Hey Steve, > where is the code to which you refer? "address resolution handler?" iser_calc_scsi_params() -- 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 1/16/2017 11:03 PM, Sagi Grimberg wrote: > >> For devices that can register page list that is bigger than >> USHRT_MAX, we actually take the wrong value for sg_tablesize. >> E.g: for CX4 max_fast_reg_page_list_len is 65536 (bigger than USHRT_MAX) >> so we set sg_tablesize to 0 by mistake. Therefore, each IO that is >> bigger than 4k splitted to "< 4k" chunks that cause performance >> degredation. >> >> Fixes: 7854550ae6d8 ("RDMA/iser: Limit sgs to the device fastreg depth") >> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> >> --- >> drivers/infiniband/ulp/iser/iscsi_iser.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c >> b/drivers/infiniband/ulp/iser/iscsi_iser.c >> index 9104e6b..ff37f19 100644 >> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >> @@ -656,7 +656,8 @@ static void iscsi_iser_cleanup_task(struct >> iscsi_task *task) >> * max fastreg page list length. >> */ > > Hi Max, > >> shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize, >> - >> ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len); >> + min_t(unsigned int, USHRT_MAX, >> + >> ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len)); > > Hmm, This looks broken since I fixed up mlx5 capability at: > 911f4331bc87 ("IB/mlx5: Expose correct max_fast_reg_page_list_len") > > However, I suspect this assignment should be removed altogether because > we already take that into account in the address resolution handler, > there the min statement is for unsigned and casted back to ushrt. Hi Sagi, I can send v2 with assignment removal (there is some work to do in iser_calc_scsi_params too) but I think that your commit 911f4331bc87 broke it accidently and not logically. Think of other providers that also may support max_fast_reg_page_list_len > USHRT_MAX (there was no such providers till that point in time). Altought it's not optimal but it's the best solution in competability means since there are 4 commits involved in that area (from first to last): 1. 7854550ae6 ("RDMA/iser: Limit sgs to the device fastreg depth") 2. df749cdc45 ("IB/iser: Support up to 8MB data transfer in a single command") 3. 911f4331bc ("IB/mlx5: Expose correct max_fast_reg_page_list_len") 4. 9c674815d3 ("IB/iser: Fix max_sectors calculation") and the last one fixes all the loop. -- 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/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 9104e6b..ff37f19 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -656,7 +656,8 @@ static void iscsi_iser_cleanup_task(struct iscsi_task *task) * max fastreg page list length. */ shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize, - ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len); + min_t(unsigned int, USHRT_MAX, + ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len)); if (iscsi_host_add(shost, ib_conn->device->ib_device->dma_device)) {
For devices that can register page list that is bigger than USHRT_MAX, we actually take the wrong value for sg_tablesize. E.g: for CX4 max_fast_reg_page_list_len is 65536 (bigger than USHRT_MAX) so we set sg_tablesize to 0 by mistake. Therefore, each IO that is bigger than 4k splitted to "< 4k" chunks that cause performance degredation. Fixes: 7854550ae6d8 ("RDMA/iser: Limit sgs to the device fastreg depth") Signed-off-by: Max Gurtovoy <maxg@mellanox.com> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)