Re: [Xen-devel] [PATCH v6 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.10.14 at 16:45, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> @@ -40,17 +41,26 @@
>  #include <xsm/xsm.h>
>  #include <asm/flushtlb.h>
>  
> +/* this option is deprecated, use gnttab_max_frames and
> + * gnttab_max_maptrack_frames instead */
>  #ifndef max_nr_grant_frames
> -unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> +unsigned int max_nr_grant_frames = 0;

I suppose this could now be static? And its as well as the other zero
initializers can be dropped too.

>  integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
>  #endif
>  
> +unsigned int max_grant_frames = 0;
> +integer_param("gnttab_max_frames", max_grant_frames);
> +
>  /* The maximum number of grant mappings is defined as a multiplier of the
>   * maximum number of grant table entries. This defines the multiplier used.
>   * Pretty arbitrary. [POLICY]
>   */
>  #define MAX_MAPTRACK_TO_GRANTS_RATIO 8
>  
> +static unsigned int max_maptrack_frames = 0;
> +integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
> +
> +

No double blank lines please.

> @@ -2930,6 +2935,24 @@ static struct keyhandler 
> gnttab_usage_print_all_keyhandler = {
>  
>  static int __init gnttab_usage_init(void)
>  {
> +    if ( max_nr_grant_frames )
> +    {
> +        printk(XENLOG_WARNING "gnttab_max_nr_frames is deprecated, use 
> gnttab_max_frames instead\n");

Long line (only keep the string literal together).

> +        if ( !max_grant_frames && !max_maptrack_frames )
> +        {
> +            max_grant_frames = max_nr_grant_frames;
> +            max_maptrack_frames = max_nr_grant_frames * 
> MAX_MAPTRACK_TO_GRANTS_RATIO;

Each unspecified field should be set independently when the
respective new option wasn't present, yet the deprecated one
was.

> +        } else {

Coding style.

> +            printk(XENLOG_WARNING "dropping gnttab_max_nr_frames as 
> superseding options are present\n");

"ignoring ..."

Also please make sure to issue at most one warning message.

> +        }
> +    }
> +
> +    if ( !max_grant_frames )
> +        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> +
> +    if ( !max_maptrack_frames )
> +        max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * 
> max_grant_frames;

As said in the earlier reply, this is specifically what you shouldn't do
when you lower the current default. This has to remain at no lower
a default than what we currently have (and should also no longer
be tied to max_grant_frames). I.e. you'll need a separate new
default value here if you really want to reduce the other one (which,
as also said, I'm not certain is a good idea).

Jan


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