[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 3/3] tools: introduce parameter max_wp_ram_ranges.





On 1/26/2016 7:00 PM, Jan Beulich wrote:
On 26.01.16 at 08:32, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
On 1/22/2016 4:01 PM, Jan Beulich wrote:
On 22.01.16 at 04:20, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -940,6 +940,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
hvm_ioreq_server *s,
   {
       unsigned int i;
       int rc;
+    unsigned int max_wp_ram_ranges =
+        ( s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] > 0 ) 
?
+        s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] :
+        MAX_NR_IO_RANGES;

Besides this having stray blanks inside the parentheses it truncates
the value from 64 to 32 bits and would benefit from using the gcc
extension of omitting the middle operand of ?:. But even better
would imo be if you avoided the local variable and ...

After second thought, how about we define a default value for this
parameter in libx.h, and initialize the parameter when creating the
domain with default value if it's not configured.

No, I don't think the tool stack should be determining the default
here (unless you want the default to be zero, and have zero
indeed mean zero).

Thank you, Jan.
If we do not provide a default value in tool stack, the code above
should be kept, to initialize the local variable with either the one
set in the configuration file, or with MAX_NR_IO_RANGES. Is this OK?

About this local variable, we keep it, and ...

@@ -962,7 +966,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
hvm_ioreq_server *s,
           if ( !s->range[i] )
               goto fail;

-        rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
+        if ( i == HVMOP_IO_RANGE_WP_MEM )
+            rangeset_limit(s->range[i], max_wp_ram_ranges);
+        else
+            rangeset_limit(s->range[i], MAX_NR_IO_RANGES);

... did the entire computation here, using ?: for the second argument
of the function invocation.

... replace the if/else pair with sth. like:
          rangeset_limit(s->range[i],
                         ((i == HVMOP_IO_RANGE_WP_MEM)?
                          max_wp_ram_ranges:
                          MAX_NR_IO_RANGES));
This 'max_wp_ram_ranges' has no particular usages, but the string
"s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES]"
is too lengthy, and can easily break the 80 column limitation. :)
Does this approach sounds OK? :)

Seems better than the original, so okay.

@@ -6009,6 +6016,7 @@ static int hvm_allow_set_param(struct domain *d,
       case HVM_PARAM_IOREQ_SERVER_PFN:
       case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
       case HVM_PARAM_ALTP2M:
+    case HVM_PARAM_MAX_WP_RAM_RANGES:
           if ( value != 0 && a->value != value )
               rc = -EEXIST;
           break;

Is there a particular reason you want this limit to be unchangeable
after having got set once?

Well, not exactly. :)
I added this limit because by now we do not have any approach to
change the max range numbers inside ioreq server during run-time.
I can add another patch to introduce an xl command, which can change
it dynamically. But I doubt the necessity of this new command and
am also wonder if this new command would cause more confusion for
the user...

And I didn't say you need to expose this to the user. All I asked
was whether you really mean the value to be a set-once one. If
yes, the code above is fine. If no, the code above should be
changed, but there's then still no need to expose a way to
"manually" adjust the value until a need for such arises.


I see. The constraint is not necessary. And I'll remove this code. :)

Jan



B.R.
Yu

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.