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.

Sign up for GitHub

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

Jump to bottom

fix: server sample not marking dirty pages #748

Merged
merged 12 commits into from
Aug 2, 2023
Merged

fix: server sample not marking dirty pages #748

merged 12 commits into from
Aug 2, 2023

Conversation

w-henderson
Copy link
Contributor

@w-henderson w-henderson commented Jul 21, 2023 •
edited
Loading

The server sample is supposed to demonstrate dirty page logging, but it was not marking dirty pages. This commit both adds client-side dirty page tracking for pages dirtied with vfu_sgl_write and server-side dirty page tracking for pages directly dirtied by the server using vfu_sgl_get/put.

@tmakatos
Copy link
Member

Could you also add a check in the client that pages do get dirtied by the server?

jlevon
jlevon reviewed Jul 24, 2023
View reviewed changes
samples/client.c Outdated Show resolved Hide resolved
samples/server.c Outdated Show resolved Hide resolved
@w-henderson w-henderson mentioned this pull request Jul 24, 2023
Closed
@w-henderson
Copy link
Contributor Author

Replaced by #753.

@w-henderson
Copy link
Contributor Author

Could you also add a check in the client that pages do get dirtied by the server?

I'll do this separately.

@w-henderson w-henderson deleted the sample-dirty-pages branch July 24, 2023 14:57
@w-henderson w-henderson restored the sample-dirty-pages branch July 25, 2023 09:01
@w-henderson w-henderson reopened this Jul 25, 2023
@w-henderson
Copy link
Contributor Author

Reopening this one to replace #753.

tmakatos
tmakatos reviewed Jul 25, 2023
View reviewed changes
samples/client.c Outdated Show resolved Hide resolved
samples/server.c Outdated Show resolved Hide resolved
@tmakatos
Copy link
Member

@w-henderson I'm not sure why you requested another review, you haven't addressed my comment here: #748 (comment)

@w-henderson
Copy link
Contributor Author

Sorry I missed that - I'm not sure I completely understand your comment, can you explain again what it is I need to do please?

@tmakatos
Copy link
Member

Sorry I missed that - I'm not sure I completely understand your comment, can you explain again what it is I need to do please?

Sure I'll rephrase. In the server sample we try to demonstrate two things:

  1. Use of vfu_sgl_write.
  2. Tracking of dirty guest pages during live miration.

The server should not have to use vfu_sgl_mark_dirty when using vfu_sgl_write, it's the client's responsibility to track dirty pages in that case. Therefore, to demonstrate dirty page tracking in libvfio-user (point 2.), the server also needs to write to guest RAM directly by using vfu_sgl_get, write to memory, mark dirty (vfu_sgl_mark_dirty), etc. The client should track pages dirtied by:

  1. VFIO_USER_DMA_WRITE commands, and
  2. by explicitly requesting which pages the server dirtied by directly accessing guest RAM, via VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP.

@jlevon
Copy link
Collaborator

jlevon commented Jul 26, 2023

The server should not have to use vfu_sgl_mark_dirty when using vfu_sgl_write, it's the client's responsibility to track dirty pages in that case.

I'd like to add something to memory-mapping.md for that, to describe our expectations here.
And a comment in the vfu_sgl_write() server code.

Perhaps even a comment in handle_dma_write() saying "here we'd mark these pages as dirty".
Going further, actually keep track of dirty pages in client, both from shared memory regions and the handle_dma_write(), then assert it's what we expect later.

Therefore, to demonstrate dirty page tracking in libvfio-user (point 2.), the server also needs to write to guest RAM directly by using vfu_sgl_get, write to memory, mark dirty (vfu_sgl_mark_dirty), etc. The client should track pages dirtied by:

1. `VFIO_USER_DMA_WRITE` commands, and

2. by explicitly requesting which pages the server dirtied by directly accessing guest RAM, via `VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP`.

@w-henderson w-henderson requested review from tmakatos and removed request for tmakatos July 28, 2023 10:41
tmakatos
tmakatos reviewed Jul 28, 2023
View reviewed changes
Copy link
Member

@tmakatos tmakatos left a comment

Choose a reason for hiding this comment

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

Can you explain in the commit how the patch achieves what you state in the commit title?

jlevon
jlevon reviewed Jul 28, 2023
View reviewed changes
samples/server.c Show resolved Hide resolved
samples/server.c Show resolved Hide resolved
samples/server.c Outdated Show resolved Hide resolved
tmakatos
tmakatos reviewed Aug 1, 2023
View reviewed changes
samples/server.c Outdated Show resolved Hide resolved
samples/server.c Outdated Show resolved Hide resolved
samples/server.c Show resolved Hide resolved
jlevon
jlevon reviewed Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

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

just tiny nits

samples/client.c Show resolved Hide resolved
samples/client.c Outdated Show resolved Hide resolved
samples/server.c Outdated Show resolved Hide resolved
@w-henderson w-henderson requested a review from jlevon August 2, 2023 14:32
@w-henderson
fix: server sample not marking dirty pages
467b372 
In `do_dma_io`, the server sample was calling `vfu_sgl_write` without calling
`vfu_sgl_mark_dirty`, which resulted in no pages being marked dirty.

Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
add check in client that pages are dirtied
e530852 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
add assertion that we only dirty first 8 pages
765c2e1 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
assert that client/server example dirties some pages
1379638 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
have server sample use vfu_sgl_write and direct writes
5d866f6 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
have client track pages dirtied by messages
eef1828 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
fix: use vfu_sgl_put correctly
52f2ae9 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
use vfu_sgl_mark_dirty with explanatory comment
357cdc8 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
make client dirty page tracking less hacky
87fe917 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
minor comment wording changes
d12239e 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson
fix: small style things
a4b9b0e 
Signed-off-by: William Henderson <william.henderson@nutanix.com>
@w-henderson w-henderson merged commit 8872c36 into master Aug 2, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlevon jlevon jlevon approved these changes

@tmakatos tmakatos Awaiting requested review from tmakatos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@w-henderson @tmakatos @jlevon