Quoting Samuel Thibault (2015-08-27 00:56:23)
> Justus Winter, le Mon 17 Aug 2015 17:51:09 +0200, a écrit :
> > @@ -2157,10 +2162,13 @@ start_pass_1:
> > /*
> > * Check for permanent objects in the destination.
> > */
> > -
> > - if ((entry->object.vm_object != VM_OBJECT_NULL) &&
> > - !entry->object.vm_object->temporary)
> > - contains_permanent_objects = TRUE;
> > + object = entry->object.vm_object;
> > + if ((object != VM_OBJECT_NULL)
> > + && ! contains_permanent_objects) {
> > + vm_object_lock(object);
> > + contains_permanent_objects = object->temporary;
>
> This looks wrong, shouldn't it be !object->temporary?
Yes.
> Also, taking the lock just to read a boolean usually means there's
> something bigger wrong somewhere. Either the boolean was not meant to
> be protected (e.g. because it is constant), or more than just reading it
> should be locked, for the whole coherency. Here it never changes except
> when memory_object_set_attributes_common is called, but then there is
> nothing that will prevent memory_object_set_attributes_common from doing
> it between the time you take the lock, get the temporary field, release
> the lock, and the time when you actually do something about it, e.g.
> here return KERN_FAILURE because you believe that there's a permanent
> object while it has just been turned temporary actually. That's probably
> not a problem in practice because the two calls will probably have some
> dependency, but that's the actual issue here.
Right.
>
> > @@ -2275,8 +2284,15 @@ start_pass_1:
> > */
> >
> > object = entry->object.vm_object;
> > + temporary = 0;
> > + if (object != VM_OBJECT_NULL) {
> > + vm_object_lock(object);
> > + temporary = object->temporary;
> > + vm_object_unlock(object);
> > + }
> > +
>
> Here it's worse: the copy strategy changes. Perhaps we could think that
> it doesn't pose problem in practice because the two calls would probably
> have a dependency, I don't know about memory objects to know.
Right, I will look more closely at the fields and how they are meant
to be used, not how it is documented in the header.
> > diff --git a/vm/vm_object.c b/vm/vm_object.c
> > index deac0c2..1d3e727 100644
> > --- a/vm/vm_object.c
> > +++ b/vm/vm_object.c
> > @@ -217,9 +217,11 @@ static void _vm_object_setup(
> > vm_size_t size)
> > {
> > *object = vm_object_template;
> > - queue_init(&object->memq);
> > vm_object_lock_init(object);
> > + vm_object_lock(object);
> > + queue_init(&object->memq);
> > object->size = size;
> > + vm_object_unlock(object);
>
> Here it's a fresh object, that's why there's no need to lock, but oh
> well.
Yes. I pondered enclosing these with #if MACH_LDEBUG, but that's ugly
and so I postponed it until it turns out to be a bottleneck.
> > @@ -1474,14 +1491,11 @@ vm_object_t vm_object_copy_delayed(
> > * must be done carefully, to avoid deadlock.
> > */
> >
> > - /*
> > - * Allocate a new copy object before locking, even
> > - * though we may not need it later.
> > - */
> > + vm_object_lock(src_object);
> >
> > new_copy = vm_object_allocate(src_object->size);
> >
> > - vm_object_lock(src_object);
>
> Better make a copy of src_object->size to avoid keeping the object
> locked while we allocate stuff.
Ok.
> > @@ -252,6 +252,8 @@ vm_pageout_setup(
> > vm_object_unlock(new_object);
> > }
> >
> > + vm_object_lock(old_object);
> > +
> > if (flush) {
> > /*
> > * Create a place-holder page where the old one was,
> > @@ -262,7 +264,6 @@ vm_pageout_setup(
> > == VM_PAGE_NULL)
> > vm_page_more_fictitious();
> > - vm_object_lock(old_object);
>
> Why keeping the lock while allocating a page?
>
> > vm_page_lock_queues();
> > vm_page_remove(m);
> > vm_page_unlock_queues();
> > @@ -281,8 +282,6 @@ vm_pageout_setup(
> > VM_EXTERNAL_STATE_EXISTS);
> > #endif /* MACH_PAGEMAP */
> >
> > - vm_object_unlock(old_object);
>
> Why keeping it locked here? For accessing old_object->internal? It is
> constant across the life of an object, thus does not need to be
> protected by the lock.
Ok. I wish that kind of knowledge was stated in the comments
describing the objects.
Cheers,
Justus
