Re: [PATCH v2 1/3] vm: Make vm_object_coalesce return new object and offset

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 05 juil. 2023 17:16:37 +0300, a ecrit:
> vm_object_coalesce() callers used to rely on the fact that it always
> merged the next_object into prev_object, potentially destroying
> next_object and leaving prev_object the result of the whole operation.
> 
> After ee65849bec5da261be90f565bee096abb4117bdd
> "vm: Allow coalescing null object with an internal object", this is no
> longer true, since in case of prev_object == VM_OBJECT_NULL and
> next_object != VM_OBJECT_NULL, the overall result is next_object, not
> prev_object. The current callers are prepared to deal with this since
> they handle this case seprately anyway, but the following commit will
> introduce another caller that handles both cases in the same code path.
> 
> So, declare the way vm_object_coalesce() coalesces the two objects its
> implementation detail, and make it return the resulting object and the
> offset into it explicitly. This simplifies the callers, too.
> ---
>  vm/vm_map.c    | 11 ++++++-----
>  vm/vm_object.c | 53 +++++++++++++++++++++++++++++++++++++-------------
>  vm/vm_object.h |  4 +++-
>  3 files changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/vm/vm_map.c b/vm/vm_map.c
> index 97fc09ce..d147d4b2 100644
> --- a/vm/vm_map.c
> +++ b/vm/vm_map.c
> @@ -1089,7 +1089,9 @@ kern_return_t vm_map_enter(
>                               entry->offset,
>                               offset,
>                               (vm_size_t)(entry->vme_end - entry->vme_start),
> -                             size)) {
> +                             size,
> +                             &entry->object.vm_object,
> +                             &entry->offset)) {
>  
>                       /*
>                        *      Coalesced the two objects - can extend
> @@ -1099,7 +1101,6 @@ kern_return_t vm_map_enter(
>                       map->size += size;
>                       entry->vme_end = end;
>                       vm_map_gap_update(&map->hdr, entry);
> -                     vm_object_deallocate(object);
>                       RETURN(KERN_SUCCESS);
>               }
>       }
> @@ -1117,7 +1118,9 @@ kern_return_t vm_map_enter(
>                       offset,
>                       next_entry->offset,
>                       size,
> -                     (vm_size_t)(next_entry->vme_end - 
> next_entry->vme_start))) {
> +                     (vm_size_t)(next_entry->vme_end - 
> next_entry->vme_start),
> +                     &next_entry->object.vm_object,
> +                     &next_entry->offset)) {
>  
>                       /*
>                        *      Coalesced the two objects - can extend
> @@ -1126,9 +1129,7 @@ kern_return_t vm_map_enter(
>                        */
>                       map->size += size;
>                       next_entry->vme_start = start;
> -                     next_entry->offset -= size;
>                       vm_map_gap_update(&map->hdr, entry);
> -                     vm_object_deallocate(object);
>                       RETURN(KERN_SUCCESS);
>               }
>       }
> diff --git a/vm/vm_object.c b/vm/vm_object.c
> index e01c1856..c238cce4 100644
> --- a/vm/vm_object.c
> +++ b/vm/vm_object.c
> @@ -2687,15 +2687,16 @@ void vm_object_page_remove(
>  
>  /*
>   *   Routine:        vm_object_coalesce
> - *   Function:       Coalesces two objects backing up adjoining
> - *                   regions of memory into a single object.
> - *
> - *   returns TRUE if objects were combined.
> - *
> - *   NOTE:   Only works at the moment if one of the objects is NULL
> - *           or if the objects are the same - otherwise, which
> - *           object do we lock first?
> - *
> + *   Purpose:
> + *           Tries to coalesce two objects backing up adjoining
> + *           regions of memory into a single object.
> + *
> + *           NOTE: Only works at the moment if one of the objects
> + *           is NULL or if the objects are the same - otherwise,
> + *           which object do we lock first?
> + *   Returns:
> + *           TRUE    if objects have been coalesced.
> + *           FALSE   the objects could not be coalesced.
>   *   Parameters:
>   *           prev_object     First object to coalesce
>   *           prev_offset     Offset into prev_object
> @@ -2705,8 +2706,14 @@ void vm_object_page_remove(
>   *           prev_size       Size of reference to prev_object
>   *           next_size       Size of reference to next_object
>   *
> + *           new_object      Resulting colesced object
> + *           new_offset      Offset into the resulting object
>   *   Conditions:
> - *   The object must *not* be locked.
> + *           The objects must *not* be locked.
> + *
> + *           If the objects are coalesced successfully, the caller's
> + *           references for both objects are consumed, and the caller
> + *           gains a reference for the new object.
>   */
>  
>  boolean_t vm_object_coalesce(
> @@ -2715,7 +2722,9 @@ boolean_t vm_object_coalesce(
>       vm_offset_t     prev_offset,
>       vm_offset_t     next_offset,
>       vm_size_t       prev_size,
> -     vm_size_t       next_size)
> +     vm_size_t       next_size,
> +     vm_object_t     *new_object,    /* OUT */
> +     vm_offset_t     *new_offset)    /* OUT */
>  {
>       vm_object_t     object;
>       vm_size_t       newsize;
> @@ -2725,10 +2734,23 @@ boolean_t vm_object_coalesce(
>                *      If neither object actually exists,
>                *      the offsets don't matter.
>                */
> -             if (prev_object == VM_OBJECT_NULL)
> +             if (prev_object == VM_OBJECT_NULL) {
> +                     *new_object = VM_OBJECT_NULL;
> +                     *new_offset = 0;
>                       return TRUE;
> +             }
>  
> -             return prev_offset + prev_size == next_offset;
> +             if (prev_offset + prev_size == next_offset) {
> +                     *new_object = prev_object;
> +                     *new_offset = prev_offset;
> +                     /*
> +                      *      Deallocate one of the two references.
> +                      */
> +                     vm_object_deallocate(prev_object);
> +                     return TRUE;
> +             }
> +
> +             return FALSE;
>       }
>  
>       if (next_object != VM_OBJECT_NULL) {
> @@ -2785,6 +2807,8 @@ boolean_t vm_object_coalesce(
>               newsize = prev_offset + prev_size + next_size;
>               if (newsize > object->size)
>                       object->size = newsize;
> +
> +             *new_offset = prev_offset;
>       } else {
>               /*
>                *      Check if we have enough space in the object
> @@ -2802,9 +2826,12 @@ boolean_t vm_object_coalesce(
>               vm_object_page_remove(object,
>                       next_offset - prev_size,
>                       next_offset);
> +
> +             *new_offset = next_offset - prev_size;
>       }
>  
>       vm_object_unlock(object);
> +     *new_object = object;
>       return TRUE;
>  }
>  
> diff --git a/vm/vm_object.h b/vm/vm_object.h
> index 46328a38..9c17541f 100644
> --- a/vm/vm_object.h
> +++ b/vm/vm_object.h
> @@ -247,7 +247,9 @@ extern boolean_t vm_object_coalesce(
>     vm_offset_t prev_offset,
>     vm_offset_t next_offset,
>     vm_size_t   prev_size,
> -   vm_size_t   next_size);
> +   vm_size_t   next_size,
> +   vm_object_t *new_object,  /* OUT */
> +   vm_offset_t *new_offset); /* OUT */
>  
>  extern void vm_object_pager_wakeup(ipc_port_t  pager);
>  
> -- 
> 2.41.0
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.