Re: [Xen-devel] [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Jan Beulich]

>>> On 01.12.17 at 16:31,  wrote:
> On 30/11/17 14:32, Jan Beulich wrote:
>> Dropping the lock before returning from grant_map_exists() means handing
>> possibly stale information back to the caller. Return back the pointer
>> to the active entry instead, for the caller to release the lock once
>> done.
> 
> I don't know enough about grant tables to reason about the deeper
> meaning of this patch, but at least I can confirm that the amended
> locking scheme seems to be correct (now).
> I just wonder if it's worthwhile to add a comment that the function
> takes a lock, but leaves it up to the caller to drop it. Since there is
> only one caller, this might be overkill, though.

Well, the function returning an active entry pointer is imo
sufficient documentation of that fact.

>> Signed-off-by: Jan Beulich 
> 
> Reviewed-by: Andre Przywara 

Thanks, Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] gnttab: improve GNTTABOP_cache_flush locking

[Andre Przywara]

Hi,

On 30/11/17 14:32, Jan Beulich wrote:
> Dropping the lock before returning from grant_map_exists() means handing
> possibly stale information back to the caller. Return back the pointer
> to the active entry instead, for the caller to release the lock once
> done.

I don't know enough about grant tables to reason about the deeper
meaning of this patch, but at least I can confirm that the amended
locking scheme seems to be correct (now).
I just wonder if it's worthwhile to add a comment that the function
takes a lock, but leaves it up to the caller to drop it. Since there is
only one caller, this might be overkill, though.

> Signed-off-by: Jan Beulich 

Reviewed-by: Andre Przywara 

Cheers,
Andre.

> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -786,10 +786,10 @@ static int _set_status(unsigned gt_versi
>  return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> -static int grant_map_exists(const struct domain *ld,
> -struct grant_table *rgt,
> -unsigned long mfn,
> -grant_ref_t *cur_ref)
> +static struct active_grant_entry *grant_map_exists(const struct domain *ld,
> +   struct grant_table *rgt,
> +   unsigned long mfn,
> +   grant_ref_t *cur_ref)
>  {
>  grant_ref_t ref, max_iter;
>  
> @@ -805,28 +805,20 @@ static int grant_map_exists(const struct
> nr_grant_entries(rgt));
>  for ( ref = *cur_ref; ref < max_iter; ref++ )
>  {
> -struct active_grant_entry *act;
> -bool_t exists;
> -
> -act = active_entry_acquire(rgt, ref);
> -
> -exists = act->pin
> -&& act->domid == ld->domain_id
> -&& act->frame == mfn;
> +struct active_grant_entry *act = active_entry_acquire(rgt, ref);
>  
> +if ( act->pin && act->domid == ld->domain_id && act->frame == mfn )
> +return act;
>  active_entry_release(act);
> -
> -if ( exists )
> -return 0;
>  }
>  
>  if ( ref < nr_grant_entries(rgt) )
>  {
>  *cur_ref = ref;
> -return 1;
> +return NULL;
>  }
>  
> -return -EINVAL;
> +return ERR_PTR(-EINVAL);
>  }
>  
>  #define MAPKIND_READ 1
> @@ -3213,6 +3205,7 @@ static int cache_flush(const gnttab_cach
>  struct domain *d, *owner;
>  struct page_info *page;
>  unsigned long mfn;
> +struct active_grant_entry *act = NULL;
>  void *v;
>  int ret;
>  
> @@ -3250,13 +3243,13 @@ static int cache_flush(const gnttab_cach
>  {
>  grant_read_lock(owner->grant_table);
>  
> -ret = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
> -if ( ret != 0 )
> +act = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
> +if ( IS_ERR_OR_NULL(act) )
>  {
>  grant_read_unlock(owner->grant_table);
>  rcu_unlock_domain(d);
>  put_page(page);
> -return ret;
> +return act ? PTR_ERR(act) : 1;
>  }
>  }
>  
> @@ -3273,7 +3266,11 @@ static int cache_flush(const gnttab_cach
>  ret = 0;
>  
>  if ( d != owner )
> +{
> +active_entry_release(act);
>  grant_read_unlock(owner->grant_table);
> +}
> +
>  unmap_domain_page(v);
>  put_page(page);
>  

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel