Page MenuHomeFreeBSD

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

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



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

rS FreeBSD src repository
Lint Skipped
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

Indentation is wrong here.


Do we need to do this when add is true?

jeff added inline comments.Mon, Nov 25, 2:32 AM

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?


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.


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.

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.


Yes, we need, and should unhold properly.


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.

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

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.

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


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.


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


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.

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


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

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


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