Page MenuHomeFreeBSD
Differential D8453

Change the device shared memory allocation routines to use the busdma API. One side effect is that all memory, including the PRP segments, are now allocated in a single slab.
ClosedPublic
Actions

Authored by scottl on Nov 6 2016, 4:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 12:01 AM
Unknown Object (File)
Tue, Nov 19, 2:01 AM
Unknown Object (File)
Sun, Nov 17, 3:17 PM
Unknown Object (File)
Thu, Nov 14, 1:24 AM
Unknown Object (File)
Sat, Nov 9, 1:46 AM
Unknown Object (File)
Tue, Nov 5, 7:47 PM
Unknown Object (File)
Oct 4 2024, 2:02 AM
Unknown Object (File)
Oct 3 2024, 4:58 PM
View All Files
Subscribers
None

Details

Reviewers
gallatin
jimharris
imp
Commits
rS308431: Convert the Q-Pair and PRP list memory allocations to use BUSDMA. Add a

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

scottl updated this revision to Diff 22027.Nov 6 2016, 4:46 AM
scottl retitled this revision from to Change the device shared memory allocation routines to use the busdma API. One side effect is that all memory, including the PRP segments, are now allocated in a single slab..
scottl updated this object.
scottl edited the test plan for this revision. (Show Details)
scottl added reviewers: imp, gallatin, jimharris.
scottl updated this revision to Diff 22034.Nov 6 2016, 4:01 PM

Remove a spurious printf

Updating D8453: Change the device shared memory allocation routines to use the busdma API.

One side effect is that all memory, including the PRP segments, are now
allocated in a single slab.

jimharris added inline comments.Nov 6 2016, 4:10 PM
sys/dev/nvme/nvme_qpair.c
492 ↗(On Diff #22027)

Nit - prplistsz would be a more accurate variable name here.

525 ↗(On Diff #22027)

This should be removed before committing.

571 ↗(On Diff #22027)

Should you still unload the dmamap here?

scottl updated this revision to Diff 22036.Nov 6 2016, 4:21 PM

Address feedback from Jim

Updating D8453: Change the device shared memory allocation routines to use the busdma API.

One side effect is that all memory, including the PRP segments, are now
allocated in a single slab.

scottl marked 3 inline comments as done.Nov 6 2016, 4:23 PM
scottl added inline comments.
sys/dev/nvme/nvme_qpair.c
492 ↗(On Diff #22027)

Decided to remove the variable, it wasn't necessary.

571 ↗(On Diff #22027)

Good catch, thanks.

jimharris added inline comments.Nov 6 2016, 4:50 PM
sys/dev/nvme/nvme_qpair.c
496 ↗(On Diff #22036)

4 needs to change to PAGE_SIZE. See below.

518 ↗(On Diff #22036)

Both cmd and cpl need to be page-aligned. Previously the contigmalloc call took care of this. Since these are allocated in one buffer now, you'll also need to assert that cmd is a PAGE_SIZE multiple. The default number of queue entries for admin and io pairs (128 and 256 respectively) will ensure the cmd queue is a PAGE_SIZE multiple but would be good to double-check that here.

538 ↗(On Diff #22036)

A PRP list cannot span a page boundary, so you'll need to make sure you account for that here when allocating chunks of the bigger buffer for each tracker's PRP list.

scottl updated this revision to Diff 22039.Nov 6 2016, 5:33 PM
scottl marked 2 inline comments as done.

Add safety belts for page alignment and page crossing.

Updating D8453: Change the device shared memory allocation routines to use the busdma API.

One side effect is that all memory, including the PRP segments, are now
allocated in a single slab.

scottl marked 2 inline comments as done.Nov 6 2016, 5:35 PM
scottl added inline comments.
sys/dev/nvme/nvme_qpair.c
518 ↗(On Diff #22036)

Thanks for the insight. I've added seats belts to the next revision

jimharris added inline comments.Nov 7 2016, 1:52 PM
sys/dev/nvme/nvme_qpair.c
547 ↗(On Diff #22039)

This is probably fine, since realistically we probably won't have a PRP list size that's not even divisible into a page. If anyone ever set MAXPHYS to something like 1.5MB, this would abort out after the first tracker and we'd be stuck with just one tracker per qpair though.

Maybe instead you could do something like this above:

prpmemsz = PAGE_SIZE * (qpair->num_trackers / (PAGE_SIZE / perps));

This should allocate enough space for PRP lists that when you trunc_page check here fails, you can do a roundup2 to the next page boundary.

The other option would be to punt on this for now and just assert that the PRP list divides evenly into PAGE_SIZE.

scottl updated this revision to Diff 22066.Nov 7 2016, 11:46 PM
scottl marked an inline comment as done.

Updating D8453: Change the device shared memory allocation routines to use the busdma API.

One side effect is that all memory, including the PRP segments, are now
allocated in a single slab.

jimharris accepted this revision.Nov 7 2016, 11:52 PM
jimharris edited edge metadata.

Looks good!

This revision is now accepted and ready to land.Nov 7 2016, 11:52 PM
Closed by commit rS308431: Convert the Q-Pair and PRP list memory allocations to use BUSDMA. Add a (authored by scottl). · Explain WhyNov 8 2016, 12:25 AM
This revision was automatically updated to reflect the committed changes.
scottl mentioned this in rS308431: Convert the Q-Pair and PRP list memory allocations to use BUSDMA. Add a.

Revision Contents
Changeset List

PathSize
head/
sys/
dev/
nvme/
25 lines
9 lines
154 lines

Diff 22067

View Options

head/sys/dev/nvme/nvme_ctrlr.c

Loading...
View Options

head/sys/dev/nvme/nvme_private.h

Loading...
View Options

head/sys/dev/nvme/nvme_qpair.c

Loading...