Page MenuHomeFreeBSD

Store the bottom of the shadow chain in OBJ_ANON object->handle member.
ClosedPublic

Authored by kib on Sun, Nov 24, 10:00 PM.

Details

Reviewers
alc
markj
jeff
kib
Summary

The handle value is stable for all shadow objects in the inheritance chain. This allows to avoid descending the shadow chain to get to the bottom of it in vm_map_entry_set_vnode_text() [I did not examined other places yet].

Change vm_object_allocate_anon() and vm_object_shadow() to handle more of the cred/charge initialization for the new shadow object, in addition to set up handle.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 27861

Event Timeline

kib created this revision.Sun, Nov 24, 10:00 PM
markj added inline comments.Mon, Nov 25, 12:53 AM
sys/vm/vm_map.c
526

Indentation is wrong here.

555

Do we need to do this when add is true?

jeff added inline comments.Mon, Nov 25, 2:32 AM
sys/vm/vm_map.c
552

Isn't TMPFS set on creation and not cleared? Do we need the rlock for the swp_tmpfs field? Can the tmpfs vnode go away while it is mapped?

sys/vm/vm_object.c
464–465

Do these need to be coherent with any user of the global object list? If so they should be set before object type to avoid acquiring a lock.

1308–1309

Can we either fix the existing style or switch to modern style?

kib marked 5 inline comments as done.Mon, Nov 25, 3:39 PM
kib added inline comments.
sys/vm/vm_map.c
552

OBJ_TMPFS_NODE is stable during the object lifetime, but OBJ_TMPFS is not. The _NODE means that the object is the v_object for tmpfs node, while OBJ_TMPFS indicates that the vnode was instantiated.

After some bugfix, I started adding a use ref to the tmpfs vnode that is text-mapped. Otherwise, just having a mapping for tmpfs vnode is not enough to prevent recycling, since we do not ref the vnode.

Might be I can remove VM_OBJECT_RLOCK() there, but this would require some thinking about visibility order.

555

Yes, we need, and should unhold properly.

sys/vm/vm_object.c
464–465

No, they are never accessed for whatever global lists iterations we do over the whole set of objects. It is fine to set them unlocked before the object is made visible to other threads in regular ways.

kib updated this revision to Diff 64850.Mon, Nov 25, 3:48 PM
kib marked 3 inline comments as done.

Style.
Unhold vnode for ADD.

jeff accepted this revision.Tue, Nov 26, 1:55 AM
This revision is now accepted and ready to land.Tue, Nov 26, 1:55 AM
jeff added a comment.Tue, Nov 26, 4:44 AM

FWIW I reviewed backing_object users and I think only vm_object_sync() wants these semantics but would also be trivial to convert.

The rest need to look for pages in the chain. It would be nice to rewrite some of these consumers to not iterate down the backing object for every page but instead to do so only for the pages that are missing. You could do this with something like a bitset or array of bools + starting offset. vm_fault_prefault() in particular shows up on profiles. For each bit that is not yet set scan the current object for pages and set bits when found. If there are still unset bits traverse down the chain.

Right now we will lock and unlock 8 times * the backing object chain depth there.

alc added inline comments.Tue, Nov 26, 7:23 AM
sys/vm/vm_map.c
4080–4086

Isn't the newly created shadow object going to have OBJ_ONEMAPPING set? With the introduction of an "else" clause below, I don't see how that OBJ_ONEMAPPING flag is being cleared before this new shadow object is shared with the child process.

kib updated this revision to Diff 64881.Tue, Nov 26, 9:16 AM
kib marked an inline comment as done.

Fix OBJ_ONEMAPPING for shared mapping after shadowing.

This revision now requires review to proceed.Tue, Nov 26, 9:16 AM
kib added a subscriber: pho.Thu, Nov 28, 12:58 PM
alc accepted this revision.Sat, Nov 30, 7:43 PM
alc added inline comments.
sys/vm/vm_fault.c
1745

As an aside, isn't this unsafe without locking because of a possible interaction with the object list scan?

1879–1880

As another aside, this pmap_enter() was the root cause of the last reported "invalid ASID" assertion failure on arm64 before I (for the time being) disabled deferred ASID allocation. The reason being that this pmap_enter() presets the accessed bit on the newly created mapping in the child's page table before the child has ever run. And, on a memory-starved machine, the page daemon might try to clear this accessed bit and invalidate the non-existent TLB entry before the child has ever run and been assigned an ASID.

sys/vm/vm_map.c
529

This would read better as "Mostly, we do not ..."

530

I would drop the word "implicitly".

This revision is now accepted and ready to land.Sat, Nov 30, 7:43 PM
kib marked 2 inline comments as done.Sat, Nov 30, 9:45 PM
kib added inline comments.
sys/vm/vm_fault.c
1745

Do you mean the removed scan in vm_meter.c, or something else ?

1879–1880

So do you want to 'remotely activate' (so to say) the target pmap there, at least allocating asid during vmspace_fork() ?

kib updated this revision to Diff 65079.Sat, Nov 30, 9:46 PM

Update the language in comments.
FIx bugs found by Peter, mostly unneeded checks for object->handle for OBJ_ANON.

This revision now requires review to proceed.Sat, Nov 30, 9:46 PM
alc added inline comments.Sat, Nov 30, 10:38 PM
sys/vm/vm_fault.c
1745

Yes. I completely forgot that this was removed, so nevermind.

1879–1880

No, I think that the "more correct" option would be to not set the accessed bit. However, adding another flag and conditional to pmap_enter() just to say. "don't preset the accessed bit", for this extremely rare case pains me.

pho added a comment.Sun, Dec 1, 8:23 AM

I completed a full stress2 test of r355146+1d4f3fce228-c245581(cpuctl). No problems seen.

kib accepted this revision.Sun, Dec 1, 8:43 PM
This revision is now accepted and ready to land.Sun, Dec 1, 8:43 PM
kib closed this revision.Sun, Dec 1, 8:44 PM