Page MenuHomeFreeBSD

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

Authored by jeff on Oct 22 2019, 11:06 PM.

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

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jeff added reviewers: kib, markj, alc, dougm.
sys/vm/vm_object.c
2050 ↗(On Diff #63560)

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

sys/vm/vm_object.c
286 ↗(On Diff #63560)

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 ↗(On Diff #63560)

Extra newline.

548 ↗(On Diff #63560)

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

sys/vm/vm_object.h
190 ↗(On Diff #63560)

Why not OBJ_ANON?

343 ↗(On Diff #63560)

Extra space after the function name.

sys/vm/vm_object.c
286 ↗(On Diff #63560)

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 ↗(On Diff #63560)

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

343 ↗(On Diff #63560)

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

sys/vm/vm_object.c
286 ↗(On Diff #63560)

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 ↗(On Diff #63560)

The refcount was approved but not yet committed.

sys/vm/vm_object.h
343 ↗(On Diff #63560)

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 ↗(On Diff #63560)

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

sys/vm/vm_object.h
343 ↗(On Diff #63560)

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 ↗(On Diff #63768)

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 ↗(On Diff #63768)

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 ↗(On Diff #63768)

You may have failed to observe this comment.

sys/vm/vm_map.c
3451 ↗(On Diff #63768)

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
453 ↗(On Diff #64475)

I think the cast is not needed.

548 ↗(On Diff #63560)

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 ↗(On Diff #64475)

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
1508 ↗(On Diff #64475)

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

sys/vm/vm_object.c
449 ↗(On Diff #64475)

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

sys/vm/vm_object.h
188 ↗(On Diff #64475)

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
449 ↗(On Diff #64475)

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 ↗(On Diff #64475)

I will update the comment before I commit.