Page MenuHomeFreeBSD

Add a constant OBJ_ANONYMOUS flag to optimize out some locking.
ClosedPublic

Authored by jeff on Oct 22 2019, 11:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 28, 11:20 PM
Unknown Object (File)
Sun, Jan 22, 11:17 PM
Unknown Object (File)
Sun, Jan 22, 11:17 PM
Unknown Object (File)
Sun, Jan 22, 11:15 PM
Unknown Object (File)
Sun, Jan 22, 11:15 PM
Unknown Object (File)
Sun, Jan 22, 11:15 PM
Unknown Object (File)
Sun, Jan 22, 11:07 PM
Unknown Object (File)
Fri, Jan 6, 1:25 AM

Details

Summary

The intent is to make it easier to check for conditions where we want to maintain an accurate ONEMAPPING and do splits and coalescing. There are now more callsites that create non-anonymous mappings than there are anonymous. This head lead to a buggy proliferation of code that likely wants to set NOSPLIT but does not and which would probably benefit from OBJ_COLOR but does not set it. I changed the default flags for swap and default objects to be those suitable for non-anonymous mappings. I added a new function, vm_object_anonymous(), that is used for the few cases that do want anonymous mappings. I also could've eliminated NOSPLIT and replaced those with checks for ANONYMOUS but I have left it for now.

I am a little concerned about OBJT_DEAD vs OBJ_DEAD. It may be possible that the exact behavior with destruction races will change but I don't believe any of the code I changed yet is affected. It is just another complexity to evaluate if we convert more (type == default || type == swap) checks.

pho is testing this along with my other recent object locking work.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27265
Build 25529: arc lint + arc unit

Event Timeline

jeff added reviewers: kib, markj, alc, dougm.
sys/vm/vm_object.c
2043

This line is wrong. I will fix and update later.

sys/vm/vm_object.c
286

Doesn't this break the colouring mechanism? vm_object_color() does nothing if OBJ_COLORED is already set. So, this change makes it impossible to set pg_color to a non-zero value in the fault path.

456

Extra newline.

548

This looks like it's part of a future diff.

sys/vm/vm_object.h
190

Why not OBJ_ANON?

343

Extra space after the function name.

sys/vm/vm_object.c
286

No, because vm_object_anonymous() clears the flag. That said, there is a related problem. To give an extreme example, when tmpfs creates a 4KB file, we are now going to allocate a reservation just for that one 4KB, and pretty soon we will run out of free 2MB chunks.

sys/vm/vm_object.h
190

In an unusual move for me, I would also argue for the shorter name. :-)

343

For consistency with the general approach to naming these functions, I would prefer vm_object_allocate_anon().

sys/vm/vm_object.c
286

We don't consult the object size?

Does this mean that every consumer needs to set color only when the object becomes large enough?

548

The refcount was approved but not yet committed.

sys/vm/vm_object.h
343

Ok I will happily adjust flag names.

Do you have comments on keeping/remove OBJ_NOSPLIT? It should become !ANON

If we are going to start setting a color by default on DEFAULT and SWAP objects, then the following code that appears in two places in vm_reserv.c needs to change. Specifically, it should test for the absence of the OBJ_ANON rather than the object being OBJT_VNODE.

/*                                                                                                                                                              
 * Would the last new reservation extend past the end of the object?                                                                                            
 */
if (first + maxpages > object->size) {
        /*                                                                                                                                                      
         * Don't allocate the last new reservation if the object is a                                                                                           
         * vnode or backed by another object that is a vnode.                                                                                                   
         */
        if (object->type == OBJT_VNODE ||
            (object->backing_object != NULL &&
            object->backing_object->type == OBJT_VNODE)) {
                if (maxpages == VM_LEVEL_0_NPAGES)
                        return (NULL);
                allocpages = minpages;
        }
        /* Speculate that the object may grow. */
}

It is really your call Alan. I was trying to unify the flags among the various non-anonymous swap object users. We also saw with tmpfs that it performs much better with reservations enabled. I can defer this part of the patch or I can fix the above code.

sys/vm/vm_object.c
286

See my followup message. I think that it answers your question.

sys/vm/vm_object.h
343

I would gladly see OBJ_NOSPLIT eliminated. Stylistically, I think that the flag OBJ_ANON is better because it explains *why* it is okay to split.

Handle more cases. Fix reservations limit. Other feedback.

sys/vm/vm_map.c
3451

No need for goto.

if ((entry->eflags & MAP_ENTRY_IS_SUB_MAP) != 0 || object == NULL)

entry->object.vm_object = NULL;

else if ((object->flags & OBJ_ANON) != 0 || object == kernel_object) {
...
}

Are there any further comments? This passed pho testing as well as some other ad-hoc workload testing. I would like to commit soon. I have a number of smaller object lock contention reduction patches I would like to move forward with.

sys/vm/vm_object.c
450–451

Because of the global object list and manipulation of the flags by code that iterates over that list, this is not safe.

Perhaps, a cleaner and safer approach would be to introduce a vm_object_allocate_flags() and move the switch state that computes the default flags to vm_object_allocate().

sys/vm/vm_map.c
3451

You may have failed to observe this comment.

sys/vm/vm_map.c
3451

I saw thanks. I will fix these two comments and post an updated patch.

Address both pieces of review feedback.

markj added inline comments.
sys/vm/vm_object.c
432

I think the cast is not needed.

548

I just mean that I haven't seen a patch that changes vm_object so that ref_count is manipulated using atomics.

sys/vm/vm_object.h
188

Shouldn't it be "is backed by"?

This revision is now accepted and ready to land.Nov 18 2019, 5:08 PM
kib added inline comments.
sys/vm/vm_map.c
1478

BTW, I do not think that we can observe ref_count == 1 and then ref_count > 1 there.

sys/vm/vm_object.c
470

It might make sense to add charge and ucred args. I might look at this as a followup.

sys/vm/vm_object.h
188

I think both 'backs' and 'backed' are not right there. OBJ_ANON object contains anonymous memory, backed usually refers to the shadow chain.

sys/vm/vm_object.c
470

I would like to see more encapsulating of that functionality. As is it is handled directly everywhere with near identical copies of code.

sys/vm/vm_object.h
188

I will update the comment before I commit.