Re: [Xen-devel] [PATCH v2 4/8] xen: make grant resource limits per domain

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
> Juergen Gross
> Sent: 06 September 2017 09:26
> To: xen-devel@xxxxxxxxxxxxx
> Cc: Juergen Gross <jgross@xxxxxxxx>; sstabellini@xxxxxxxxxx; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>;
> jbeulich@xxxxxxxx
> Subject: [Xen-devel] [PATCH v2 4/8] xen: make grant resource limits per
> domain
> 
> Instead of using the same global resource limits of grant tables (max.
> number of grant frames, max. number of maptrack frames) for all domains
> make these limits per domain. This will allow setting individual limits
> in the future. For now initialize the per domain limits with the global
> values.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  xen/common/grant_table.c | 83 +++++++++++++++++++++++++++----------
> -----------
>  1 file changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index b50c334f70..ae5f1262e8 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -69,6 +69,9 @@ struct grant_table {
>      /* The defined versions are 1 and 2.  Set to 0 if we don't know
>         what version to use yet. */
>      unsigned              gt_version;
> +    /* Resource limits of the domain. */
> +    unsigned int          max_grant_frames;
> +    unsigned int          max_maptrack_frames;
>  };
> 
>  #ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
> @@ -285,8 +288,8 @@ num_act_frames_from_sha_frames(const unsigned
> int num)
>      return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
>  }
> 
> -#define max_nr_active_grant_frames \
> -    num_act_frames_from_sha_frames(max_grant_frames)
> +#define max_nr_active_grant_frames(gt) \
> +    num_act_frames_from_sha_frames(gt->max_grant_frames)
> 
>  static inline unsigned int
>  nr_active_grant_frames(struct grant_table *gt)
> @@ -524,7 +527,7 @@ get_maptrack_handle(
>       * out of memory, try stealing an entry from another VCPU (in case the
>       * guest isn't mapping across its VCPUs evenly).
>       */
> -    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
> +    if ( nr_maptrack_frames(lgt) < lgt->max_maptrack_frames )
>          new_mt = alloc_xenheap_page();
> 
>      if ( !new_mt )
> @@ -1663,7 +1666,7 @@ gnttab_grow_table(struct domain *d, unsigned int
> req_nr_frames)
>      struct grant_table *gt = d->grant_table;
>      unsigned int i, j;
> 
> -    ASSERT(req_nr_frames <= max_grant_frames);
> +    ASSERT(req_nr_frames <= gt->max_grant_frames);
> 
>      gdprintk(XENLOG_INFO,
>              "Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
> @@ -1728,14 +1731,15 @@ grant_table_init(struct domain *d)
>      if ( gt->nr_grant_frames )
>          return 0;
> 
> -    gt->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
> +    gt->nr_grant_frames = min_t(unsigned int,
> INITIAL_NR_GRANT_FRAMES,
> +                                              gt->max_grant_frames);
> 
>      /* Active grant table. */
>      if ( (gt->active = xzalloc_array(struct active_grant_entry *,
> -                                     max_nr_active_grant_frames)) == NULL )
> +                                     max_nr_active_grant_frames(gt))) == 
> NULL )
>          goto no_mem_1;
>      for ( i = 0;
> -          i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES);
> i++ )
> +          i < num_act_frames_from_sha_frames(gt->nr_grant_frames); i++ )
>      {
>          if ( (gt->active[i] = alloc_xenheap_page()) == NULL )
>              goto no_mem_2;
> @@ -1745,14 +1749,14 @@ grant_table_init(struct domain *d)
>      }
> 
>      /* Tracking of mapped foreign frames table */
> -    gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
> +    gt->maptrack = vzalloc(gt->max_maptrack_frames * sizeof(*gt-
> >maptrack));
>      if ( gt->maptrack == NULL )
>          goto no_mem_2;
> 
>      /* Shared grant table. */
> -    if ( (gt->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
> +    if ( (gt->shared_raw = xzalloc_array(void *, gt->max_grant_frames)) ==
> NULL )
>          goto no_mem_3;
> -    for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> +    for ( i = 0; i < gt->nr_grant_frames; i++ )
>      {
>          if ( (gt->shared_raw[i] = alloc_xenheap_page()) == NULL )
>              goto no_mem_4;
> @@ -1761,11 +1765,11 @@ grant_table_init(struct domain *d)
> 
>      /* Status pages for grant table - for version 2 */
>      gt->status = xzalloc_array(grant_status_t *,
> -                               grant_to_status_frames(max_grant_frames));
> +                               grant_to_status_frames(gt->max_grant_frames));
>      if ( gt->status == NULL )
>          goto no_mem_4;
> 
> -    for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> +    for ( i = 0; i < gt->nr_grant_frames; i++ )
>          gnttab_create_shared_page(d, gt, i);
> 
>      gt->nr_status_frames = 0;
> @@ -1773,7 +1777,7 @@ grant_table_init(struct domain *d)
>      return 0;
> 
>   no_mem_4:
> -    for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> +    for ( i = 0; i < gt->nr_grant_frames; i++ )
>          free_xenheap_page(gt->shared_raw[i]);
>      xfree(gt->shared_raw);
>      gt->shared_raw = NULL;
> @@ -1782,7 +1786,7 @@ grant_table_init(struct domain *d)
>      gt->maptrack = NULL;
>   no_mem_2:
>      for ( i = 0;
> -          i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES);
> i++ )
> +          i < num_act_frames_from_sha_frames(gt->nr_grant_frames); i++ )
>          free_xenheap_page(gt->active[i]);
>      xfree(gt->active);
>      gt->active = NULL;
> @@ -1807,15 +1811,6 @@ gnttab_setup_table(
>      if ( unlikely(copy_from_guest(&op, uop, 1)) )
>          return -EFAULT;
> 
> -    if ( unlikely(op.nr_frames > max_grant_frames) )
> -    {
> -        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table
> frames"
> -                " per domain.\n",
> -                max_grant_frames);
> -        op.status = GNTST_general_error;
> -        goto out;
> -    }
> -
>      if ( !guest_handle_okay(op.frame_list, op.nr_frames) )
>          return -EFAULT;
> 
> @@ -1835,6 +1830,15 @@ gnttab_setup_table(
>      gt = d->grant_table;
>      grant_write_lock(gt);
> 
> +    if ( unlikely(op.nr_frames > gt->max_grant_frames) )
> +    {
> +        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table
> frames"
> +                " per domain.\n",
> +                gt->max_grant_frames);

The message should probably now say that the domain has only been provisioned 
with X grant frames, rather than implying that the limit is still global as it 
does.
Otherwise LGTM so with that tweak...

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> +        op.status = GNTST_general_error;
> +        goto unlock;
> +    }
> +
>      if ( gt->gt_version == 0 )
>      {
>          if ( grant_table_init(d) )
> @@ -1852,7 +1856,7 @@ gnttab_setup_table(
>      {
>          gdprintk(XENLOG_INFO,
>                   "Expand grant table to %u failed. Current: %u Max: %u\n",
> -                 op.nr_frames, nr_grant_frames(gt), max_grant_frames);
> +                 op.nr_frames, nr_grant_frames(gt), gt->max_grant_frames);
>          op.status = GNTST_general_error;
>          goto unlock;
>      }
> @@ -1887,6 +1891,7 @@ gnttab_query_size(
>  {
>      struct gnttab_query_size op;
>      struct domain *d;
> +    struct grant_table *gt;
> 
>      if ( count != 1 )
>          return -EINVAL;
> @@ -1907,13 +1912,15 @@ gnttab_query_size(
>          goto out;
>      }
> 
> -    grant_read_lock(d->grant_table);
> +    gt = d->grant_table;
> 
> -    op.nr_frames     = nr_grant_frames(d->grant_table);
> -    op.max_nr_frames = max_grant_frames;
> +    grant_read_lock(gt);
> +
> +    op.nr_frames     = nr_grant_frames(gt);
> +    op.max_nr_frames = gt->max_grant_frames;
>      op.status        = GNTST_okay;
> 
> -    grant_read_unlock(d->grant_table);
> +    grant_read_unlock(gt);
> 
>   out:
>      if ( d )
> @@ -3464,6 +3471,8 @@ grant_table_create(
>      /* Simple stuff. */
>      percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>      spin_lock_init(&t->maptrack_lock);
> +    t->max_grant_frames = max_grant_frames;
> +    t->max_maptrack_frames = max_maptrack_frames;
> 
>      /* Okay, install the structure. */
>      d->grant_table = t;
> @@ -3727,7 +3736,7 @@ int gnttab_map_frame(struct domain *d, unsigned
> long idx, gfn_t gfn,
>      }
>      else
>      {
> -        if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
> +        if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
>              gnttab_grow_table(d, idx + 1);
> 
>          if ( idx < nr_grant_frames(gt) )
> @@ -3754,6 +3763,12 @@ static void gnttab_usage_print(struct domain *rd)
> 
>      grant_read_lock(gt);
> 
> +    printk("grant-table for remote domain:%5d (v%d)\n"
> +           "  %d frames (%d max), %d maptrack frames (%d max)\n",
> +           rd->domain_id, gt->gt_version,
> +           nr_grant_frames(gt), gt->max_grant_frames,
> +           nr_maptrack_frames(gt), gt->max_maptrack_frames);
> +
>      for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
>      {
>          struct active_grant_entry *act;
> @@ -3781,12 +3796,7 @@ static void gnttab_usage_print(struct domain *rd)
>              status = status_entry(gt, ref);
>          }
> 
> -        if ( first )
> -        {
> -            printk("grant-table for remote domain:%5d (v%d)\n",
> -                   rd->domain_id, gt->gt_version);
> -            first = 0;
> -        }
> +        first = 0;
> 
>          /*      [0xXXX]  ddddd 0xXXXXXX 0xXXXXXXXX      ddddd 0xXXXXXX 0xXX
> */
>          printk("[0x%03x]  %5d 0x%06lx 0x%08x      %5d 0x%06"PRIx64"
> 0x%02x\n",
> @@ -3798,8 +3808,7 @@ static void gnttab_usage_print(struct domain *rd)
>      grant_read_unlock(gt);
> 
>      if ( first )
> -        printk("grant-table for remote domain:%5d ... "
> -               "no active grant table entries\n", rd->domain_id);
> +        printk("no active grant table entries\n");
>  }
> 
>  static void gnttab_usage_print_all(unsigned char key)
> --
> 2.12.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel