[PATCH] sysctl: adjust XEN_SYSCTL_numainfo behavior

[PATCH] sysctl: adjust XEN_SYSCTL_numainfo behavior

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

... to match XEN_SYSCTL_cputopoinfo, allowing the caller to get what it
needs (if e.g. it's after the data for just one specific node) with
just one hypercall, without caring about the total number of nodes in
the system.

Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -285,15 +285,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
         {
             xen_sysctl_meminfo_t meminfo = { 0 };
 
-            if ( ni->num_nodes < num_nodes )
-            {
-                ret = -ENOBUFS;
-                i = num_nodes;
-            }
-            else
-                i = 0;
-
-            for ( ; i < num_nodes; i++ )
+            if ( num_nodes > ni->num_nodes )
+                num_nodes = ni->num_nodes;
+            for ( i = 0; i < num_nodes; ++i )
             {
                 static uint32_t distance[MAX_NUMNODES];
 
@@ -335,7 +329,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
         else
             i = num_nodes;
 
-        if ( (!ret || (ret == -ENOBUFS)) && (ni->num_nodes != i) )
+        if ( !ret && (ni->num_nodes != i) )
         {
             ni->num_nodes = i;
             if ( __copy_field_to_guest(u_sysctl, op,
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -515,10 +515,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_memin
  *    non-null)
  *
  * OUT:
- *  - If 'num_nodes' is less than the number Xen needs to write, -ENOBUFS shall
- *    be returned and 'num_nodes' updated to reflect the intended number.
- *  - On success, 'num_nodes' shall indicate the number of entries written, which
- *    may be less than the maximum.
+ *  - If 'num_nodes' is less than the number Xen wants to write but either
+ *    handle is not a NULL one, partial data gets returned and 'num_nodes'
+ *    gets updated to reflect the intended number.
+ *  - Otherwise, 'num_nodes' shall indicate the number of entries written, which
+ *    may be less than the input value.
  */
 
 struct xen_sysctl_numainfo {




[-- Attachment #2: sysctl-numainfo-partial.patch --]
[-- Type: text/plain, Size: 2165 bytes --]

sysctl: adjust XEN_SYSCTL_numainfo behavior

... to match XEN_SYSCTL_cputopoinfo, allowing the caller to get what it
needs (if e.g. it's after the data for just one specific node) with
just one hypercall, without caring about the total number of nodes in
the system.

Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -285,15 +285,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
         {
             xen_sysctl_meminfo_t meminfo = { 0 };
 
-            if ( ni->num_nodes < num_nodes )
-            {
-                ret = -ENOBUFS;
-                i = num_nodes;
-            }
-            else
-                i = 0;
-
-            for ( ; i < num_nodes; i++ )
+            if ( num_nodes > ni->num_nodes )
+                num_nodes = ni->num_nodes;
+            for ( i = 0; i < num_nodes; ++i )
             {
                 static uint32_t distance[MAX_NUMNODES];
 
@@ -335,7 +329,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
         else
             i = num_nodes;
 
-        if ( (!ret || (ret == -ENOBUFS)) && (ni->num_nodes != i) )
+        if ( !ret && (ni->num_nodes != i) )
         {
             ni->num_nodes = i;
             if ( __copy_field_to_guest(u_sysctl, op,
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -515,10 +515,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_memin
  *    non-null)
  *
  * OUT:
- *  - If 'num_nodes' is less than the number Xen needs to write, -ENOBUFS shall
- *    be returned and 'num_nodes' updated to reflect the intended number.
- *  - On success, 'num_nodes' shall indicate the number of entries written, which
- *    may be less than the maximum.
+ *  - If 'num_nodes' is less than the number Xen wants to write but either
+ *    handle is not a NULL one, partial data gets returned and 'num_nodes'
+ *    gets updated to reflect the intended number.
+ *  - Otherwise, 'num_nodes' shall indicate the number of entries written, which
+ *    may be less than the input value.
  */
 
 struct xen_sysctl_numainfo {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] sysctl: adjust XEN_SYSCTL_numainfo behavior

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1147 bytes --]

On Tue, 2015-07-14 at 10:52 +0100, Jan Beulich wrote:
> ... to match XEN_SYSCTL_cputopoinfo, allowing the caller to get what it
> needs (if e.g. it's after the data for just one specific node) with
> just one hypercall, without caring about the total number of nodes in
> the system.
> 
> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

One question:

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
>  
> @@ -335,7 +329,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
>          else
>              i = num_nodes;
>  
> -        if ( (!ret || (ret == -ENOBUFS)) && (ni->num_nodes != i) )
> +        if ( !ret && (ni->num_nodes != i) )
>          {
Can't we kill the parentheses around the second argument of the && ?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] sysctl: adjust XEN_SYSCTL_numainfo behavior

[Andrew Cooper]

On 14/07/15 13:35, Dario Faggioli wrote:
> On Tue, 2015-07-14 at 10:52 +0100, Jan Beulich wrote:
>> ... to match XEN_SYSCTL_cputopoinfo, allowing the caller to get what it
>> needs (if e.g. it's after the data for just one specific node) with
>> just one hypercall, without caring about the total number of nodes in
>> the system.
>>
>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>
> One question:
>
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>>  
>> @@ -335,7 +329,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
>>          else
>>              i = num_nodes;
>>  
>> -        if ( (!ret || (ret == -ENOBUFS)) && (ni->num_nodes != i) )
>> +        if ( !ret && (ni->num_nodes != i) )
>>          {
> Can't we kill the parentheses around the second argument of the && ?

No.  The coding style requires binary operators as part of larger
statements to have brackets.

~Andrew

Re: [PATCH] sysctl: adjust XEN_SYSCTL_numainfo behavior

[Jan Beulich]

>>> On 14.07.15 at 14:43, <andrew.cooper3@citrix.com> wrote:
> On 14/07/15 13:35, Dario Faggioli wrote:
>> On Tue, 2015-07-14 at 10:52 +0100, Jan Beulich wrote:
>>> ... to match XEN_SYSCTL_cputopoinfo, allowing the caller to get what it
>>> needs (if e.g. it's after the data for just one specific node) with
>>> just one hypercall, without caring about the total number of nodes in
>>> the system.
>>>
>>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>>
>> One question:
>>
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>>  
>>> @@ -335,7 +329,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
>>>          else
>>>              i = num_nodes;
>>>  
>>> -        if ( (!ret || (ret == -ENOBUFS)) && (ni->num_nodes != i) )
>>> +        if ( !ret && (ni->num_nodes != i) )
>>>          {
>> Can't we kill the parentheses around the second argument of the && ?
> 
> No.  The coding style requires binary operators as part of larger
> statements to have brackets.

I don't see it requires this, and I can't even find it recommending
such - it just so happens that some people prefer using the extra
parentheses. I personally dislike them for all operators where the
precedence is "natural", i.e. here I wouldn't have added them if
I had written the code anew. Since I modified existing code, I
decided to retain previous style.

Jan

Re: [PATCH] sysctl: adjust XEN_SYSCTL_numainfo behavior

[Ian Campbell]

On Tue, 2015-07-14 at 10:52 +0100, Jan Beulich wrote:
> ... to match XEN_SYSCTL_cputopoinfo, allowing the caller to get what it
> needs (if e.g. it's after the data for just one specific node) with
> just one hypercall, without caring about the total number of nodes in
> the system.
> 
> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>