Re: [Xen-devel] [PATCHv9 1/4] gnttab: per-active entry locking

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.05.15 at 17:54, <david.vrabel@xxxxxxxxxx> wrote:
> @@ -505,7 +526,7 @@ 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(spin_is_locked(&rgt->lock));
> @@ -514,18 +535,18 @@ static int grant_map_exists(const struct domain *ld,
>                     nr_grant_entries(rgt));
>      for ( ref = *ref_count; ref < max_iter; ref++ )
>      {
> -        act = &active_entry(rgt, ref);
> +        bool_t exists;

With this I think act's declaration should move into this more narrow
scope too, the more that you need to drop its const anyway.

> @@ -702,6 +729,7 @@ __gnttab_map_grant_ref(
>  
>      cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
>  
> +    active_entry_release(act);
>      spin_unlock(&rgt->lock);

Just for my understanding: The lock isn't meant to also cover *shah?
I.e. it could be dropped ahead of the cache_flags assignment?

> @@ -978,7 +1009,7 @@ __gnttab_unmap_common(
>          if ( (rc = replace_grant_host_mapping(op->host_addr,
>                                                op->frame, op->new_addr, 
>                                                op->flags)) < 0 )
> -            goto unmap_out;
> +            goto act_release_out;
>  
>          ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
>          op->map->flags &= ~GNTMAP_host_map;

act doesn't look to be accessed anymore after the if() this is
contained in - why don't you drop the lock as early as possible?
Or wait - do we need act->pin to remain stable until after that
subsequent if() (in which case dropping the lock before the
final if() in the function would mainly make error handling more
cumbersome without bying us much)?

> @@ -1979,8 +2026,11 @@ __acquire_grant_for_copy(
>                                            &trans_page_off, &trans_length, 0);
>  
>              spin_lock(&rgt->lock);
> +            act = active_entry_acquire(rgt, gref);
> +
>              if ( rc != GNTST_okay ) {
>                  __fixup_status_for_copy_pin(act, status);
> +                active_entry_release(act);
>                  rcu_unlock_domain(td);
>                  spin_unlock(&rgt->lock);
>                  return rc;
> @@ -1994,6 +2044,7 @@ __acquire_grant_for_copy(
>              {
>                  __fixup_status_for_copy_pin(act, status);
>                  rcu_unlock_domain(td);
> +                active_entry_release(act);
>                  spin_unlock(&rgt->lock);

Please make the two sequences of unlocks the same.

> @@ -2074,7 +2126,11 @@ __acquire_grant_for_copy(
>          gnttab_clear_flag(_GTF_reading, status);
>  
>   unlock_out:
> +    active_entry_release(act);
> +
> + gnt_unlock_out:

I think this would better be gt_unlock_out or gnttab_unlock_out.

Jan


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