virtio-pmem: fixes and improvements by ShadowCurse · Pull Request #5789 · firecracker-microvm/firecracker

ShadowCurse · 2026-03-24T17:01:06Z

Changes

Validate descriptors len field to be 4 since the code expects this
Cache msync result for better efficiency in case multiple flush requests are presented at once
Add rate-limiter support

Reason

Edge case handling and addition of missing features

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

This functionality cannot be added in rust-vmm.

codecov · 2026-03-24T17:08:56Z

Codecov Report

❌ Patch coverage is 39.70588% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.88%. Comparing base (054b647) to head (90f29cf).

Files with missing lines	Patch %	Lines
src/vmm/src/devices/virtio/pmem/device.rs	46.15%	35 Missing ⚠️
src/vmm/src/rpc_interface.rs	0.00%	18 Missing ⚠️
src/vmm/src/lib.rs	0.00%	12 Missing ⚠️
src/firecracker/src/api_server/request/pmem.rs	60.86%	9 Missing ⚠️
src/vmm/src/devices/virtio/pmem/event_handler.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5789      +/-   ##
==========================================
- Coverage   83.07%   82.88%   -0.20%     
==========================================
  Files         275      275              
  Lines       29462    29586     +124     
==========================================
+ Hits        24476    24522      +46     
- Misses       4986     5064      +78

Flag	Coverage Δ
5.10-m5n.metal	`83.19% <39.70%> (-0.23%)`	⬇️
5.10-m6a.metal	`82.52% <39.70%> (-0.23%)`	⬇️
5.10-m6g.metal	`79.77% <39.70%> (-0.21%)`	⬇️
5.10-m6i.metal	`83.19% <39.70%> (-0.22%)`	⬇️
5.10-m7a.metal-48xl	`82.50% <39.70%> (-0.23%)`	⬇️
5.10-m7g.metal	`79.78% <39.70%> (-0.21%)`	⬇️
5.10-m7i.metal-24xl	`83.16% <39.70%> (-0.23%)`	⬇️
5.10-m7i.metal-48xl	`83.16% <39.70%> (-0.23%)`	⬇️
5.10-m8g.metal-24xl	`79.77% <39.70%> (-0.21%)`	⬇️
5.10-m8g.metal-48xl	`79.77% <39.70%> (-0.21%)`	⬇️
5.10-m8i.metal-48xl	`83.16% <39.70%> (-0.22%)`	⬇️
5.10-m8i.metal-96xl	`83.17% <39.70%> (-0.22%)`	⬇️
6.1-m5n.metal	`83.22% <39.70%> (-0.21%)`	⬇️
6.1-m6a.metal	`82.54% <39.70%> (-0.24%)`	⬇️
6.1-m6g.metal	`79.77% <39.70%> (-0.21%)`	⬇️
6.1-m6i.metal	`83.21% <39.70%> (-0.22%)`	⬇️
6.1-m7a.metal-48xl	`82.53% <39.70%> (-0.22%)`	⬇️
6.1-m7g.metal	`79.77% <39.70%> (-0.21%)`	⬇️
6.1-m7i.metal-24xl	`83.23% <39.70%> (-0.22%)`	⬇️
6.1-m7i.metal-48xl	`83.23% <39.70%> (-0.22%)`	⬇️
6.1-m8g.metal-24xl	`79.77% <39.70%> (-0.21%)`	⬇️
6.1-m8g.metal-48xl	`79.77% <39.70%> (-0.21%)`	⬇️
6.1-m8i.metal-48xl	`83.23% <39.70%> (-0.22%)`	⬇️
6.1-m8i.metal-96xl	`83.23% <39.70%> (-0.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/vmm/src/devices/virtio/pmem/device.rs

Manciukic · 2026-04-01T08:59:46Z

docs/pmem.md

+available for block devices. It throttles flush requests using two token
+buckets:
+
+- `bandwidth` — limits the total bytes flushed per refill interval. Each flush


the very first sentence is incorrect as we don't know how much each msync is going to flush. We should make it more clear that it applies the full file size at all times. Maybe we should start with that or not have a bw limiter at all for this.

Maybe more clear would be: "bandwidth — consumes tokens equal to the full backing file size for each msync call, effectively limiting how often dirty pages can be flushed to disk."

Updated the first sentence a bit.

what value does bandwidth actually add to the users (compared to ops) if it's imprecise? How would users know how to configure it properly?

ok, I saw the guidance we give in the doc, but still not entirely convinced bandwidth adds a value

our default rate-limiter configuration scheme is to give ability to set both (e.g. we use RateLimiterConfig everywhere) so at least for consistency it makes sense. Also I think bandwidth maybe makes more sense since figuring out how much requests guest sends (for any device at this point) is a questionable business, but bandwidth is something people is more used to thinking about.

Yes, uniformity here is an argument for me. Users can choose whatever they are more comfortable with.

Manciukic · 2026-04-01T10:02:43Z

src/vmm/src/devices/virtio/pmem/device.rs


+        // Rate limit: consume 1 op and file_len bytes for the coalesced msync.
+        // If the rate limiter is blocked, defer processing until the timer fires.
+        if !self.rate_limiter.consume(1, TokenType::Ops) {


we're consuming tokens before even knowing if there's an item in the queue to process

Considering there is only one valid type of request, I think this is fine. Valid guests will only notify if there is something for us to process.

FC notifies on resume irrespectively on whether there's a request or not in the queue

ok, moved these checks past the queue processing step, before the notification.

kalyazin · 2026-04-09T16:17:08Z

src/firecracker/swagger/firecracker.yaml

+        - name: body
+          in: body
+          description: Pmem rate limiter properties
+          required: true


does it mean that users can't remove the rate limiter later on? Other devices seem to allow that.

What do you mean? The description here seems to be same as for other devices. Pmem logic for updating rate-limiter is same as for other devices.

What I mean is, for example, block doesn't make rate limiter required:

PartialDrive: type: object required: - drive_id properties: drive_id: type: string path_on_host: type: string description: Host level path for the guest drive. This field is optional for virtio-block config and should be omitted for vhost-user-block configuration. rate_limiter: $ref: "#/definitions/RateLimiter"

while pmem does:

PartialPmem: type: object description: Defines a partial pmem device structure, used to update the rate limiter for that device, after microvm start. required: - id - rate_limiter <==== here properties: id: type: string rate_limiter: $ref: "#/definitions/RateLimiter"

oh, this is simply because rate-limiter is the only thing that can be changed. There is no reason to PATCH it if not to change the rate-limiter. For other devices I was looking at the ...UpdateConfig types and since there are always a couple things that can change there it makes sense to make them optional. Do you think we should allow empty PATCH requests for pmem?

I'm just thinking it could be the only way to disable the rate limiter later on. Not sure if it's a common use case though. Would probably make sense to keep it on par with other devices in that sense.

ok, made it optional

kalyazin · 2026-04-09T16:18:24Z

src/firecracker/src/api_server/request/pmem.rs

    }
 }

+pub(crate) fn parse_patch_pmem(


we seem to have unittests for parse_put_pmem, but not for parse_patch_pmem

kalyazin · 2026-04-09T16:20:59Z

docs/pmem.md

+available for block devices. It throttles flush requests using two token
+buckets:
+
+- `bandwidth` — limits the total bytes flushed per refill interval. Each flush


ok, I saw the guidance we give in the doc, but still not entirely convinced bandwidth adds a value

kalyazin · 2026-04-09T16:21:31Z

CHANGELOG.md

  information can be found in the
  [docs](docs/vsock.md/#unix-domain-socket-renaming).
+- [#5789](https://github.com/firecracker-microvm/firecracker/pull/5789): Add
+  rate-limiter support to virtio-pmem devie to allow control over I/O bandwidth


s/devie/device/

Head and status descriptors must be 4 bytes long by the spec. Add validation for this. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

There is only one type of request virto-pmem accepts, but guest still can issue many of them and Firecracker will need to process it all. Instead of doing one `msync` per request, it is less resource intensive to do it once on the first valid descriptor and then duplicate the result to other descriptors. This is safe since the guest will only know the result of the execution after Firecracker will signal it, which will only happen after all descriptors are processed. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Add rate-limiter support to the virtio-pmem device to allow users to configure limits of the I/O bandwidth generated by the `msync` call in the device which could be triggered by the guest FLUSH requests. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Add a section in the pmem.md file describing a way of limiting I/O usage of `msync` calls from a virtio-pmem device. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Mention new rate-limiter API for the virtio-pmem device in the CHANGELOG.md Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Fix incorrect NOTE section formatting in memory usage section Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Update device-api.md with rate-limiter for virtio-pmem. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

ShadowCurse self-assigned this Mar 24, 2026

ShadowCurse force-pushed the pmem_fixes branch from 2a06407 to 787ca80 Compare March 24, 2026 17:01

ShadowCurse marked this pull request as ready for review March 24, 2026 17:15

ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Enhancement Indicates new feature requests Type: Fix Indicates a fix to existing code labels Mar 24, 2026

ShadowCurse force-pushed the pmem_fixes branch 2 times, most recently from 46cccd5 to 813efa7 Compare March 25, 2026 10:25

JamesC1305 reviewed Mar 25, 2026

View reviewed changes

src/vmm/src/devices/virtio/pmem/device.rs Show resolved Hide resolved

src/vmm/src/devices/virtio/pmem/device.rs Show resolved Hide resolved

src/vmm/src/devices/virtio/pmem/device.rs Show resolved Hide resolved

src/vmm/src/devices/virtio/pmem/device.rs Show resolved Hide resolved

JamesC1305 reviewed Mar 25, 2026

View reviewed changes

src/vmm/src/devices/virtio/pmem/device.rs Show resolved Hide resolved

ShadowCurse force-pushed the pmem_fixes branch 2 times, most recently from d140771 to e48ab2a Compare March 26, 2026 09:41

ShadowCurse requested review from Manciukic, kalyazin and pb8o as code owners March 27, 2026 11:03

ShadowCurse changed the title ~~Pmem fixes~~ virtio-pmem: fixes and improvements Mar 27, 2026

ShadowCurse force-pushed the pmem_fixes branch 5 times, most recently from e4ccee4 to 24441ed Compare March 31, 2026 11:08

Manciukic reviewed Apr 1, 2026

View reviewed changes

ShadowCurse force-pushed the pmem_fixes branch 7 times, most recently from a0aae35 to a9a3d01 Compare April 7, 2026 13:19

ShadowCurse force-pushed the pmem_fixes branch from a9a3d01 to ed269c0 Compare April 7, 2026 14:06

kalyazin reviewed Apr 9, 2026

View reviewed changes

ShadowCurse force-pushed the pmem_fixes branch from ed269c0 to b56b6e0 Compare April 10, 2026 09:42

ShadowCurse requested a review from micz010 as a code owner April 10, 2026 09:42

ShadowCurse force-pushed the pmem_fixes branch 2 times, most recently from 2c56730 to eb02623 Compare April 10, 2026 10:55

ShadowCurse added 7 commits April 10, 2026 11:55

fix: pmem: add validation for head and status descriptors lengths

89cf13e

Head and status descriptors must be 4 bytes long by the spec. Add validation for this. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

doc: pmem: add a note about limiting msync with cgroupsv2

51e3ad4

Add a section in the pmem.md file describing a way of limiting I/O usage of `msync` calls from a virtio-pmem device. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

changelog: pmem: add a note about rate-limiter support in virtio-pmem

a405fcb

Mention new rate-limiter API for the virtio-pmem device in the CHANGELOG.md Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

doc: pmem: fix incorrect NOTE section formatting

eea9a2f

Fix incorrect NOTE section formatting in memory usage section Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

pmem: docs: mention new rate-limiter option

90f29cf

Update device-api.md with rate-limiter for virtio-pmem. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

ShadowCurse force-pushed the pmem_fixes branch from eb02623 to 90f29cf Compare April 10, 2026 10:55

Conversation

ShadowCurse commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Reason

License Acceptance

PR Checklist

Uh oh!

codecov bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalyazin Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ShadowCurse commented Mar 24, 2026 •

edited

Loading

codecov bot commented Mar 24, 2026 •

edited

Loading

kalyazin Apr 9, 2026 •

edited

Loading