Re: [Xen-devel] [PATCH v4 2/2] gnttab: refactor locking for scalability

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.01.15 at 16:12, <chegger@xxxxxxxxx> wrote:
> @@ -188,6 +191,26 @@ 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 */

When coding style issues are being pointed out, please make sure
you go through your patch series and address _all_ of them, even
if not each and every violation was explicitly named.

> @@ -514,17 +513,22 @@ 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);
> +        struct active_grant_entry *act;
> +        act = active_entry_acquire(rgt, ref);
>  
> -        if ( !act->pin )
> -            continue;
> +#define CONTINUE_IF(cond) \
> +if ((cond)) { \
> +    active_entry_release(act); \
> +    continue; \
> +}
>  
> -        if ( act->domid != ld->domain_id )
> -            continue;
> +        CONTINUE_IF(!act->pin);
> +        CONTINUE_IF(act->domid != ld->domain_id);
> +        CONTINUE_IF(act->frame != mfn);

I highly question this is in any way better than the v3 code. Is there
a clear reason not to follow the advice of putting the
active_entry_release(act) into the third of the for() expressions?

And if you _really_ want/need it this way, please again get the coding
style right (and drop the pointless redundant parentheses).

> @@ -545,16 +555,32 @@ static void mapcount(
>      grant_handle_t handle;
>  
>      *wrc = *rdc = 0;
> +    ASSERT(rw_is_locked(&rd->grant_table->lock));
> +
> +    /* N.B.: while taking the left side maptrack spinlock prevents
> +     * any mapping changes, the right side active entries could be
> +     * changing while we are counting. To be perfectly correct, the
> +     * caller should hold the grant table write lock, or some other
> +     * mechanism should be used to prevent concurrent changes during
> +     * this operation. This is tricky because we can't promote a read
> +     * lock into a write lock.
> +     */
> +    spin_lock(&lgt->maptrack_lock);

So you added the suggested ASSERT(), but you didn't adjust the
comment (which actually should precede the ASSERT() imo) in any
way.

> @@ -698,12 +723,9 @@ __gnttab_map_grant_ref(
>              GNTPIN_hstr_inc : GNTPIN_hstw_inc;
>  
>      frame = act->frame;
> -    act_pin = act->pin;
>  
>      cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
>  
> -    read_unlock(&rgt->lock);
> -
>      /* pg may be set, with a refcount included, from __get_paged_frame */
>      if ( !pg )
>      {
> @@ -778,8 +800,6 @@ __gnttab_map_grant_ref(
>          goto undo_out;
>      }
>  
> -    double_maptrack_lock(lgt, rgt);

Again an un-addressed comment from v3: "Taking these two together
- isn't patch 1 wrong then, in that it acquires both maptrack locks in
double_gt_lock()?"

> @@ -941,18 +960,19 @@ __gnttab_unmap_common(
>  
>      rgt = rd->grant_table;
>      read_lock(&rgt->lock);
> -    double_maptrack_lock(lgt, rgt);
> +
> +    read_lock(&rgt->lock);

Acquiring the same read lock twice in a row?

> @@ -3045,9 +3097,11 @@ static void gnttab_usage_print(struct domain *rd)
>          uint16_t status;
>          uint64_t frame;
>  
> -        act = &active_entry(gt, ref);
> -        if ( !act->pin )
> +        act = active_entry_acquire(gt, ref);
> +        if ( !act->pin ) {

Coding style again.

Jan


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