Re: [PATCH v5 2/2] gnttab: refactor locking for scalability

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.02.15 at 13:51, <chegger@amazon.de> wrote:
> @@ -188,6 +191,28 @@ nr_active_grant_frames(struct grant_table *gt)
>      return num_act_frames_from_sha_frames(nr_grant_frames(gt));
>  }
>  
> +static inline struct active_grant_entry *
> +active_entry_acquire(struct grant_table *t, grant_ref_t e)
> +{
> +    struct active_grant_entry *act;
> +
> +    /* 
> +     * not perfect, but better than nothing for a debug build
> +     * sanity check
> +     */

Better, but still not matching ./CODING_STYLE.

> @@ -316,7 +317,10 @@ get_maptrack_handle(
>      return handle;
>  }
>  
> -/* Number of grant table entries. Caller must hold d's grant table lock. */
> +/* 
> + * Number of grant table entries. Caller must hold d's grant table
> + * read lock.
> + */
>  static unsigned int nr_grant_entries(struct grant_table *gt)

Doesn't this belong in patch 1?

> @@ -505,26 +509,23 @@ static int grant_map_exists(const struct domain *ld,
>                              unsigned long mfn,
>                              unsigned int *ref_count)
>  {
> -    const struct active_grant_entry *act;
> +    struct active_grant_entry *act;
>      unsigned int ref, max_iter;
>      
>      ASSERT(rw_is_locked(&rgt->lock));
>  
>      max_iter = min(*ref_count + (1 << GNTTABOP_CONTINUATION_ARG_SHIFT),
>                     nr_grant_entries(rgt));
> -    for ( ref = *ref_count; ref < max_iter; ref++ )
> +    for ( ref = *ref_count; ref < max_iter; active_entry_release(act), ref++ )
>      {
> -        act = &active_entry(rgt, ref);
> +        act = active_entry_acquire(rgt, ref);
>  
> -        if ( !act->pin )
> -            continue;
> -
> -        if ( act->domid != ld->domain_id )
> -            continue;
> -
> -        if ( act->frame != mfn )
> +        if ( !act->pin ||
> +             act->domid != ld->domain_id ||
> +             act->frame != mfn )
>              continue;

I don't mind this consolidation, but it does grow the patch size
without real need.

>      for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
>      {
> +        struct active_grant_entry *act;
> +
>          map = &maptrack_entry(lgt, handle);
>          if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
>               map->domid != rd->domain_id )
>              continue;
> -        if ( active_entry(rd->grant_table, map->ref).frame == mfn )
> +        act = &_active_entry(rd->grant_table, map->ref);
> +        if ( act->frame == mfn )
>              (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++;
>      }

Yet here I don't understand at all what the change is good for - the
only thing you appear to need to change is to insert an underscore.

> @@ -2601,9 +2637,17 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
>  {
>      struct domain *d = rcu_lock_current_domain();
>      struct grant_table *gt = d->grant_table;
> -    struct active_grant_entry *act;
> +    struct active_grant_entry *act_a = NULL;
> +    struct active_grant_entry *act_b = NULL;
>      s16 rc = GNTST_okay;
>  
> +    if ( ref_a == ref_b )
> +        /*
> +         * noop, so avoid acquiring the same active entry
> +         * twice which would case a deadlock.

cause

> @@ -2953,7 +3001,7 @@ grant_table_create(
>      struct domain *d)
>  {
>      struct grant_table *t;
> -    int                 i;
> +    int                 i, j;

unsigned int

> @@ -3195,9 +3244,12 @@ static void gnttab_usage_print(struct domain *rd)
>          uint16_t status;
>          uint64_t frame;
>  
> -        act = &active_entry(gt, ref);
> +        act = active_entry_acquire(gt, ref);

As (again in a different context) said recently, debug key handlers
would better use try-lock variants, skipping the respective printing
if the lock is being held.

And to come back to a remark on v3 and v4: Considering that you
drop double_maptrack_lock() here altogether, is patch 1 really
correct/sensible in introducing that function in place of
double_gt_lock()? Wouldn't the new functionality want the local
maptrack and the remote grant table locked in such cases? I.e.
isn't one of the locks taken at least pointless?

Jan