Page MenuHomeFreeBSD

Clean up ARM per-cpu qmap fields
ClosedPublic

Authored by jah on Jan 24 2017, 5:23 AM.

Details

Summary

Remove qmap PTE and address fields for armv4, which I left unused
after r286296.

pc_qmap_addr for armv6 is a leftover from the old armv6 pmap, so might
as well rename it and put it to use in the new one.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jah updated this revision to Diff 24366.Jan 24 2017, 5:23 AM
jah retitled this revision from to Clean up ARM per-cpu qmap fields.
jah updated this object.
jah edited the test plan for this revision. (Show Details)
jah added a reviewer: skra.Jan 24 2017, 5:24 AM
skra added inline comments.Jan 24 2017, 10:24 AM
sys/arm/arm/pmap-v6.c
5894 ↗(On Diff #24366)

Hmm. PCPU_GET() calls get_pcpu() internally, which is implemented as cp15 operation in SMP case. And cp15 operations may be slow. As an optimalization, I suggest to use get_pcpu() here and use obtained pointer.

Further, I checked other related functions. They use pcpu_find() when current struct pcpu* is wanted. While pcpu_find() is a function which needs to get current CPU id (one cp15 operation) to get struct pcpu*, get_pcpu() is a macro which gets struct pcpu* (one cp15 operation) directly. So, I suggest to replace pcpu_find() by get_pcpu() in these functions in a different commit.

jah added inline comments.Jan 24 2017, 4:20 PM
sys/arm/arm/pmap-v6.c
5894 ↗(On Diff #24366)

Thank you for reminding me that we have get_pcpu() on arm. That was what I really wanted to use on i386, but unfortunately we only have the thread pointer in %gs there. BTW, concern over relative slowness of %gs and cp15 access was exactly the reason I chose to use pcpu_find() in the other functions: https://reviews.freebsd.org/D8833?id=23041#inline-52317

jah added inline comments.Jan 24 2017, 4:31 PM
sys/arm/arm/pmap-v6.c
5894 ↗(On Diff #24366)

%gs->%fs

skra added inline comments.Jan 24 2017, 10:25 PM
sys/arm/arm/pmap-v6.c
5894 ↗(On Diff #24366)

To clarify my note. The following line is used to get current pcpu in the other functions:
pc = pcpu_find(curcpu);
where curcpu is defined as
#define curcpu PCPU_GET(cpuid)

Thus, pcpu_find(curcpu) can be broken down as:
(1) get current pcpu
(2) get current cpu id from current pcpu
(3) call pcpu_find()
(4) get current pcpu according to current cpu id.

So, steps (2),(3), and (4) are pointless.

jah added inline comments.Jan 25 2017, 5:29 PM
sys/arm/arm/pmap-v6.c
5894 ↗(On Diff #24366)

I understood your original note, I noted the same thing while working on the i386 pmap.
Looking at the code for each architecture, I don't see why we can't add get_pcpu() for the other architectures besides arm/arm64/riscv where it currently exists. That might be of some benefit since there are quite a few places in MI code where we use the pcpu_find(curcpu) trick.

BTW, I decided to include the switch to get_pcpu() with this commit since the changes are few and $(WORK) is leaving me with little time to do more freebsd work this week.

jah updated this revision to Diff 24437.Jan 25 2017, 5:29 PM

Replace pcpu_find with get_pcpu()

skra accepted this revision.Jan 25 2017, 9:25 PM
skra edited edge metadata.

I suggest that the main reason for this change should be the replacement of pcpu_find() by get_pcpu() now. I mean the text in commit message. The cleanup should be stated as secondary.

It was my reason for separate commit. To clearly describe what was wrong with pcpu_find() .

sys/arm/arm/pmap-v6.c
5894 ↗(On Diff #24366)

I just wanted to be correct. The first note about pcpu_find() was not accurate.

5895 ↗(On Diff #24437)

Type of pc_qmap_addr is vm_offset_t, and qmap_addr is used only once in this function. What about to not use qmap_addr variable here?

This revision is now accepted and ready to land.Jan 25 2017, 9:25 PM
This revision was automatically updated to reflect the committed changes.