Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nv_peer_mem: Fix SGls #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nv_peer_mem: Fix SGls #75

wants to merge 1 commit into from

Conversation

yishaih
Copy link
Contributor

@yishaih yishaih commented Aug 20, 2020

Fix SGls to hold the correct data in both the first and the last entries.

nv_peer_mem.c Outdated

if (page_table->page_size != NVIDIA_P2P_PAGE_SIZE_64KB) {
peer_err("nv_dma_map -- assumption of 64KB pages failed size_id=%u\n",
nv_mem_context->page_table->page_size);
return -EINVAL;
}

system_page_virt_start = nv_mem_context->addr & PAGE_MASK;
system_page_virt_end = (nv_mem_context->addr + nv_mem_context->size + PAGE_SIZE - 1) & PAGE_MASK;
system_page_offest = system_page_virt_start - nv_mem_context->page_virt_start;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: offest

sg->dma_length = nv_mem_context->page_size;
page_size = nv_mem_context->page_size;
if (i == nv_mem_context->npages - 1)
page_size = ((system_page_virt_end - system_page_virt_start +
Copy link

@drossetti drossetti Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be either system page size (4KB x86, 64KB POWER) or 64KB (GPU BAR1), right?
a comment would be useful

Also, that does not work if system page size > GPU_PAGE_SIZE, hypothetically. A BUG_ON/static assert would be useful.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And don't forget on ppc64le, sometimes the system is using HUGE pages which is configurable (default 2MB on RHEL7/8, but should always be queried).

@drossetti
Copy link

you are also changing nv_mem_get_page_size(), which is beyond the description of this patch.
but you are just adjusting the alignment for only the first and last entry of the sg_table.
I'm struggling to find the connection between the two. Are they related ?
It's weird probably because nv_mem_get_page_size() does not have a meaning for sg_table, where each entry has its own size.

@jgunthorpe
Copy link

get_page_size() was a hack that over-rides the automatic page size detection, and for old kernels it makes certain requirements on the construction of the sgl that are incompatible with this. New kernels ignore the result of get_page_size() and use the automatic page size detection from the SGL. Returning a 4k PAGE_SIZE and a properly formed SGL is compatible both ways.

@ferasd
Copy link
Contributor

ferasd commented Aug 23, 2020

For now, this change will not be merged as it fixes functionality but introduces a performance degradation
It will be on hold until changing the kernel part to override this degradation

Fix SGls to hold the correct data in both the first and the last
entries.

Signed-off-by: Artemy Kovalyov <artemyko@mellanox.com>
Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants