Re: [Xen-devel] [PATCHv6 4/5] gnttab: remove unnecessary grant table locks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.15 at 18:00, <david.vrabel@xxxxxxxxxx> wrote:
> This is safe because: a) the grant table version only changes once
> from 0 to 1 or 2;

gnttab_set_version() also allows it to transition between 1 and 2
afaics.

> @@ -919,18 +914,15 @@ __gnttab_unmap_common(
>      }
>  
>      op->map = &maptrack_entry(lgt, op->handle);
> -    spin_lock(&lgt->lock);
>  
>      if ( unlikely(!op->map->flags) )
>      {
> -        spin_unlock(&lgt->lock);
>          gdprintk(XENLOG_INFO, "Zero flags for handle (%d).\n", op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>  
>      dom = op->map->domid;
> -    spin_unlock(&lgt->lock);

Don't you need a lock here to see a consistent
op->map->{dom,ref,flags} tuple (with the writing side also holding
a lock while updating them)?

> @@ -952,7 +944,6 @@ __gnttab_unmap_common(
>  
>      rgt = rd->grant_table;
>  
> -    spin_lock(&rgt->lock);
>      act = active_entry_acquire(rgt, op->map->ref);
>  
>      op->flags = op->map->flags;

Same here then.

> @@ -1423,7 +1410,17 @@ gnttab_setup_table(
>      }
>  
>      gt = d->grant_table;
> -    spin_lock(&gt->lock);
> +
> +    /* Tracking of mapped foreign frames table */
> +    if ( (gt->maptrack = xzalloc_array(struct grant_mapping *,
> +                                       max_maptrack_frames * d->max_vcpus)) 
> == NULL )
> +        goto out2;

This provides no error indication to the caller. And is this allocation
guaranteed to be no larger than a page? Finally - does this belong
here (and not instead into the last patch)?

> @@ -114,13 +114,13 @@ gnttab_grow_table(struct domain *d, unsigned int 
> req_nr_frames);
>  /* Number of grant table frames. Caller must hold d's grant table lock. */
>  static inline unsigned int nr_grant_frames(struct grant_table *gt)
>  {
> -    return gt->nr_grant_frames;
> +    return atomic_read(&gt->nr_grant_frames);
>  }
>  
>  /* Number of status grant table frames. Caller must hold d's gr. table 
> lock.*/
>  static inline unsigned int nr_status_frames(struct grant_table *gt)
>  {
> -    return gt->nr_status_frames;
> +    return atomic_read(&gt->nr_status_frames);
>  }

Both comments need changing.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel