* [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue
@ 2019-07-11 1:32 Lijun Ou
2019-07-22 17:50 ` Jason Gunthorpe
2019-07-22 17:54 ` Jason Gunthorpe
0 siblings, 2 replies; 3+ messages in thread
From: Lijun Ou @ 2019-07-11 1:32 UTC (permalink / raw)
To: dledford, jgg; +Cc: leon, linux-rdma, linuxarm
From: Xi Wang <wangxi11@huawei.com>
When run perftest in many times, the system will report a BUG as follows:
[ 2312.559759] BUG: Bad rss-counter state mm:(____ptrval____) idx:0 val:-1
[ 2312.574803] BUG: Bad rss-counter state mm:(____ptrval____) idx:1 val:1
We tested with different kernel version and found it started from the the
following commit:
commit d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in
SGEs")
In this commit, the sg->offset is always 0 when sg_set_page() is called in
ib_umem_get() and the drivers are not allowed to change the sgl, otherwise
it will get bad page descriptor when unfolding SGEs in __ib_umem_release()
as sg_page_count() will get wrong result while sgl->offset is not 0.
However, there is a weird sgl usage in the current hns driver, the driver
modified sg->offset after calling ib_umem_get(), which caused we iterate
past the wrong number of pages in for_each_sg_page iterator.
This patch fixes it by correcting the non-standard sgl usage found in the
hns_roce_db_map_user() function.
Fixes: 0425e3e6e0c7 ("RDMA/hns: Support flush cqe for hip08 in kernel space")
Signed-off-by: Xi Wang <wangxi11@huawei.com>
---
drivers/infiniband/hw/hns/hns_roce_db.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/hw/hns/hns_roce_db.c b/drivers/infiniband/hw/hns/hns_roce_db.c
index 0c6c1fe..d60453e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_db.c
+++ b/drivers/infiniband/hw/hns/hns_roce_db.c
@@ -12,13 +12,15 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
struct ib_udata *udata, unsigned long virt,
struct hns_roce_db *db)
{
+ unsigned long page_addr = virt & PAGE_MASK;
struct hns_roce_user_db_page *page;
+ unsigned int offset;
int ret = 0;
mutex_lock(&context->page_mutex);
list_for_each_entry(page, &context->page_list, list)
- if (page->user_virt == (virt & PAGE_MASK))
+ if (page->user_virt == page_addr)
goto found;
page = kmalloc(sizeof(*page), GFP_KERNEL);
@@ -28,8 +30,8 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
}
refcount_set(&page->refcount, 1);
- page->user_virt = (virt & PAGE_MASK);
- page->umem = ib_umem_get(udata, virt & PAGE_MASK, PAGE_SIZE, 0, 0);
+ page->user_virt = page_addr;
+ page->umem = ib_umem_get(udata, page_addr, PAGE_SIZE, 0, 0);
if (IS_ERR(page->umem)) {
ret = PTR_ERR(page->umem);
kfree(page);
@@ -39,10 +41,9 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
list_add(&page->list, &context->page_list);
found:
- db->dma = sg_dma_address(page->umem->sg_head.sgl) +
- (virt & ~PAGE_MASK);
- page->umem->sg_head.sgl->offset = virt & ~PAGE_MASK;
- db->virt_addr = sg_virt(page->umem->sg_head.sgl);
+ offset = virt - page_addr;
+ db->dma = sg_dma_address(page->umem->sg_head.sgl) + offset;
+ db->virt_addr = sg_virt(page->umem->sg_head.sgl) + offset;
db->u.user_page = page;
refcount_inc(&page->refcount);
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue
2019-07-11 1:32 [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue Lijun Ou
@ 2019-07-22 17:50 ` Jason Gunthorpe
2019-07-22 17:54 ` Jason Gunthorpe
1 sibling, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2019-07-22 17:50 UTC (permalink / raw)
To: Lijun Ou; +Cc: dledford, leon, linux-rdma, linuxarm
On Thu, Jul 11, 2019 at 09:32:17AM +0800, Lijun Ou wrote:
> From: Xi Wang <wangxi11@huawei.com>
>
> When run perftest in many times, the system will report a BUG as follows:
>
> [ 2312.559759] BUG: Bad rss-counter state mm:(____ptrval____) idx:0 val:-1
> [ 2312.574803] BUG: Bad rss-counter state mm:(____ptrval____) idx:1 val:1
>
> We tested with different kernel version and found it started from the the
> following commit:
>
> commit d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in
> SGEs")
>
> In this commit, the sg->offset is always 0 when sg_set_page() is called in
> ib_umem_get() and the drivers are not allowed to change the sgl, otherwise
> it will get bad page descriptor when unfolding SGEs in __ib_umem_release()
> as sg_page_count() will get wrong result while sgl->offset is not 0.
>
> However, there is a weird sgl usage in the current hns driver, the driver
> modified sg->offset after calling ib_umem_get(), which caused we iterate
> past the wrong number of pages in for_each_sg_page iterator.
>
> This patch fixes it by correcting the non-standard sgl usage found in the
> hns_roce_db_map_user() function.
>
> Fixes: 0425e3e6e0c7 ("RDMA/hns: Support flush cqe for hip08 in kernel space")
> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> ---
> drivers/infiniband/hw/hns/hns_roce_db.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
This looks like the right fix to the reported problem
> diff --git a/drivers/infiniband/hw/hns/hns_roce_db.c b/drivers/infiniband/hw/hns/hns_roce_db.c
> index 0c6c1fe..d60453e 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_db.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_db.c
> @@ -12,13 +12,15 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
> struct ib_udata *udata, unsigned long virt,
> struct hns_roce_db *db)
> {
> + unsigned long page_addr = virt & PAGE_MASK;
> struct hns_roce_user_db_page *page;
> + unsigned int offset;
> int ret = 0;
>
> mutex_lock(&context->page_mutex);
>
> list_for_each_entry(page, &context->page_list, list)
> - if (page->user_virt == (virt & PAGE_MASK))
> + if (page->user_virt == page_addr)
> goto found;
>
> page = kmalloc(sizeof(*page), GFP_KERNEL);
> @@ -28,8 +30,8 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
> }
>
> refcount_set(&page->refcount, 1);
> - page->user_virt = (virt & PAGE_MASK);
> - page->umem = ib_umem_get(udata, virt & PAGE_MASK, PAGE_SIZE, 0, 0);
> + page->user_virt = page_addr;
> + page->umem = ib_umem_get(udata, page_addr, PAGE_SIZE, 0, 0);
> if (IS_ERR(page->umem)) {
> ret = PTR_ERR(page->umem);
> kfree(page);
> @@ -39,10 +41,9 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
> list_add(&page->list, &context->page_list);
>
> found:
> - db->dma = sg_dma_address(page->umem->sg_head.sgl) +
> - (virt & ~PAGE_MASK);
> - page->umem->sg_head.sgl->offset = virt & ~PAGE_MASK;
> - db->virt_addr = sg_virt(page->umem->sg_head.sgl);
> + offset = virt - page_addr;
> + db->dma = sg_dma_address(page->umem->sg_head.sgl) + offset;
> + db->virt_addr = sg_virt(page->umem->sg_head.sgl) + offset;
However the use of sg_virt here is wrong. Please send a patch fixing
it.
You need to store the struct page * in the db and use kmap when you
want to access it.
Better would have been to create a shared kernel/user page via mmap
like the other drivers do.
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue
2019-07-11 1:32 [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue Lijun Ou
2019-07-22 17:50 ` Jason Gunthorpe
@ 2019-07-22 17:54 ` Jason Gunthorpe
1 sibling, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2019-07-22 17:54 UTC (permalink / raw)
To: Lijun Ou; +Cc: dledford, leon, linux-rdma, linuxarm
On Thu, Jul 11, 2019 at 09:32:17AM +0800, Lijun Ou wrote:
> From: Xi Wang <wangxi11@huawei.com>
>
> When run perftest in many times, the system will report a BUG as follows:
>
> [ 2312.559759] BUG: Bad rss-counter state mm:(____ptrval____) idx:0 val:-1
> [ 2312.574803] BUG: Bad rss-counter state mm:(____ptrval____) idx:1 val:1
>
> We tested with different kernel version and found it started from the the
> following commit:
>
> commit d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in
> SGEs")
>
> In this commit, the sg->offset is always 0 when sg_set_page() is called in
> ib_umem_get() and the drivers are not allowed to change the sgl, otherwise
> it will get bad page descriptor when unfolding SGEs in __ib_umem_release()
> as sg_page_count() will get wrong result while sgl->offset is not 0.
>
> However, there is a weird sgl usage in the current hns driver, the driver
> modified sg->offset after calling ib_umem_get(), which caused we iterate
> past the wrong number of pages in for_each_sg_page iterator.
>
> This patch fixes it by correcting the non-standard sgl usage found in the
> hns_roce_db_map_user() function.
>
> Fixes: 0425e3e6e0c7 ("RDMA/hns: Support flush cqe for hip08 in kernel space")
> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> ---
> drivers/infiniband/hw/hns/hns_roce_db.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
Applied to for-rc
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-22 17:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 1:32 [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue Lijun Ou
2019-07-22 17:50 ` Jason Gunthorpe
2019-07-22 17:54 ` Jason Gunthorpe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.