Re: [Xen-devel] [PATCH v9 09/10] xen: make grant resource limits per domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.17 at 13:41, <jgross@xxxxxxxx> wrote:
> 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. Set those per-domain limits in
> grant_table_set_limits(). The global settings are serving as an upper
> boundary now which must not be exceeded by a per-domain value. The
> default of max_grant_frames is set to the maximum default xl will use.
> 
> While updating the semantics of the boot parameters remove the
> documentation of the no longer existing gnttab_max_nr_frames.

"... and correct the default gnttab_max_maptrack_frames uses" (or
some such).

> @@ -1672,8 +1670,8 @@ gnttab_grow_table(struct domain *d, unsigned int 
> req_nr_frames)
>      ASSERT(gt->active);
>  
>      if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
> -        req_nr_frames = INITIAL_NR_GRANT_FRAMES;
> -    ASSERT(req_nr_frames <= max_grant_frames);
> +        req_nr_frames = min(INITIAL_NR_GRANT_FRAMES, gt->max_grant_frames);

I'm not convinced of this: You effectively allowing a zero size grant
table this way. I'd prefer if the "initial" constant stayed the lower
bound. I'm open to lowering that initial value, though.

> @@ -1824,6 +1818,21 @@ gnttab_setup_table(
>      gt = d->grant_table;
>      grant_write_lock(gt);
>  
> +    if ( unlikely(op.nr_frames > gt->max_grant_frames) )
> +    {
> +        gdprintk(XENLOG_INFO, "d%u is limited to %u grant-table frames.\n",

You've switched to %u one too many times - domain IDs want
printing with %d (also below).

> @@ -2970,14 +2983,14 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>  
>  static long
>  gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
> uop,
> -                         int count)
> +                         unsigned int count, unsigned int limit_max)
> {
>       gnttab_get_status_frames_t op;
>      struct domain *d;
>      struct grant_table *gt;
>      uint64_t       gmfn;
>      int i;
> -    int rc;
> +    int rc, ret = 0;

This variable doesn't look to be necessary anymore (also in
gnttab_setup_table(), as I notice only now).

> @@ -3010,9 +3023,19 @@ 
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
> uop,
>  
>      if ( unlikely(op.nr_frames > nr_status_frames(gt)) )
>      {
> -        gdprintk(XENLOG_INFO, "Guest requested addresses for %d grant status 
> "
> -                 "frames, but only %d are available.\n",
> -                 op.nr_frames, nr_status_frames(gt));
> +        gdprintk(XENLOG_INFO, "Guest requested addresses of d%u for %u grant 
> "
> +                 "status frames, but only %u are available.\n",

Drop "Guest" and make the end ", has only %u\n"?

> @@ -3665,7 +3694,11 @@ int grant_table_set_limits(struct domain *d, unsigned 
> int grant_frames,
>  
>      /* Set limits. */
>      if ( !gt->active )
> +    {
> +        gt->max_grant_frames = grant_frames;

As per above I think you want to silently apply a lower bound here.

> @@ -3769,6 +3802,12 @@ static void gnttab_usage_print(struct domain *rd)
>  
>      grant_read_lock(gt);
>  
> +    printk("grant-table for remote domain:%5d (v%d)\n"

"grant table for d%d (v%u)\n"?

> +           "  %d frames (%d max), %d maptrack frames (%d max)\n",

%u (four times)

Jan


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