Page MenuHomeFreeBSD
Differential D57688

uiomove_*: centralize the copy function selection
ClosedPublic
Actions

Authored by brooks on Fri, Jun 19, 9:27 PM.
Tags
None
Referenced Files
F160251566: D57688.id180197.diff
Mon, Jun 22, 2:04 PM
F160187240: D57688.id180233.diff
Mon, Jun 22, 12:28 AM
Unknown Object (File)
Sun, Jun 21, 6:43 PM
Unknown Object (File)
Sun, Jun 21, 6:21 PM
Unknown Object (File)
Sat, Jun 20, 3:30 AM
Subscribers
jhibbits
olce
riscv

Details

Reviewers
emaste
jhb
imp
kib
andrew
manu
Commits
rGfc9b28879e89: uiomove_*: centralize the copy function selection
Summary

Add an inline uiomove_docopy() for the central set of switch statements
which choose between userspace and kernel and if data is going to or
from the iovec.

While here, switch from bcopy to memcpy.

Sponsored by: Innovate UK

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 74074
Build 70957: arc lint + arc unit

Event Timeline

brooks created this revision.Fri, Jun 19, 9:27 PM
Herald added a reviewer: andrew. · View Herald TranscriptFri, Jun 19, 9:27 PM
Herald added a reviewer: andrew. · View Herald Transcript
Herald added a reviewer: manu. · View Herald Transcript
Herald added subscribers: riscv, olce, jhibbits. · View Herald Transcript
brooks requested review of this revision.Fri, Jun 19, 9:27 PM
Harbormaster completed remote builds in B74034: Diff 180110.Fri, Jun 19, 9:27 PM
brooks added a parent revision: D57687: pmap: prefer memcpy over bcopy.Fri, Jun 19, 9:27 PM
brooks added a child revision: D57689: uipc_socket: prefer memcpy to bcopy.
brooks added a comment.Fri, Jun 19, 9:31 PM

With CHERI we'll be adding to variants of UIO_READ/UIO_WRITE that preserved pointer provenance and centralizing the definitions will make adding that a lot cleaner.

sys/sys/_uio_common.h
43 ↗(On Diff #180110)

I'm not totally convinced this needs to be inlined. It might be better to just make it a function in subr_uio.c.

kib added inline comments.Sat, Jun 20, 12:08 PM
sys/sys/_uio_common.h
43 ↗(On Diff #180110)

I support this.

Also I think that the name might be changed to reflect that this is a iovec component processing, like uiomove_step() or something better.

brooks updated this revision to Diff 180166.Sat, Jun 20, 2:18 PM
  • Rename to uiomove_step to move to subr_uio.c
  • Pass struct uio * as the last argument (similar to other functions) instead of separate segflg and rw arguments
Harbormaster completed remote builds in B74060: Diff 180166.Sat, Jun 20, 2:18 PM
brooks marked an inline comment as done.Sat, Jun 20, 2:19 PM
kib added inline comments.Sat, Jun 20, 2:27 PM
sys/arm/arm/uio_machdep.c
96

Don't 'goto out' leak sfbuf? For arm this matter.

sys/i386/i386/uio_machdep.c
96

Same leak, it seems.

sys/powerpc/powerpc/uio_machdep.c
102

sfbuf leak

sys/riscv/riscv/uio_machdep.c
123

Now it is not sfbuf leak, but the transient map leak, which is pre-existing bug in the code.

brooks updated this revision to Diff 180197.Sat, Jun 20, 7:44 PM

Fix sfbuf and transient map leaks

Harbormaster completed remote builds in B74069: Diff 180197.Sat, Jun 20, 7:44 PM
kib added inline comments.Sat, Jun 20, 7:53 PM
sys/amd64/amd64/uio_machdep.c
106

Could 'goto out' be replaced by break?

113–114

With the new organization, do we still need the unmap on the return path?

brooks updated this revision to Diff 180203.Sat, Jun 20, 8:33 PM

Drop gotos.

Remove the gratutious post-loop pmap_unmap_io_transient's.

Harbormaster completed remote builds in B74074: Diff 180203.Sat, Jun 20, 8:33 PM
kib accepted this revision.Sat, Jun 20, 8:39 PM
This revision is now accepted and ready to land.Sat, Jun 20, 8:39 PM
Closed by commit rGfc9b28879e89: uiomove_*: centralize the copy function selection (authored by brooks). · Explain WhySun, Jun 21, 4:15 PM
This revision was automatically updated to reflect the committed changes.
brooks added a commit: rGfc9b28879e89: uiomove_*: centralize the copy function selection.

Revision Contents
Changeset List

PathSize
sys/
amd64/
amd64/
34 lines
arm/
arm/
33 lines
arm64/
arm64/
36 lines
i386/
i386/
33 lines
kern/
65 lines
powerpc/
powerpc/
33 lines
riscv/
riscv/
36 lines
sys/
1 line

Diff 180203

View Options

sys/amd64/amd64/uio_machdep.c

Loading...
View Options

sys/arm/arm/uio_machdep.c

Loading...
View Options

sys/arm64/arm64/uio_machdep.c

Loading...
View Options

sys/i386/i386/uio_machdep.c

Loading...
View Options

sys/kern/subr_uio.c

Loading...
View Options

sys/powerpc/powerpc/uio_machdep.c

Loading...
View Options

sys/riscv/riscv/uio_machdep.c

Loading...
View Options

sys/sys/uio.h

Loading...