[PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[Olaf Hering]

While looking for the cause of errors returnd by
XEN_DOMCTL_getpageframeinfo3 I came across this hardcoded value. Replace
it with a constant and use that constant also in libxc.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_domain.c     | 9 +++++----
 xen/arch/x86/domctl.c       | 2 +-
 xen/include/public/domctl.h | 2 ++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index ea3df1ef31..20dc4e2bf5 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2035,14 +2035,15 @@ int xc_map_domain_meminfo(xc_interface *xch, uint32_t domid,
                                            minfo->guest_width);
 
     /* Retrieve PFN types in batches */
-    for ( i = 0; i < minfo->p2m_size ; i+=1024 )
+    for ( i = 0; i < minfo->p2m_size ; i+=XEN_GETPAGEFRAMEINFO3_MAX_SIZE )
     {
-        int count = ((minfo->p2m_size - i ) > 1024 ) ?
-                        1024: (minfo->p2m_size - i);
+        int count = ((minfo->p2m_size - i ) > XEN_GETPAGEFRAMEINFO3_MAX_SIZE ) ?
+                        XEN_GETPAGEFRAMEINFO3_MAX_SIZE: (minfo->p2m_size - i);
 
         if ( xc_get_pfn_type_batch(xch, domid, count, minfo->pfn_type + i) )
         {
-            PERROR("Could not get %d-eth batch of PFN types", (i+1)/1024);
+            PERROR("Could not get %d-eth batch of PFN types",
+                  (i+1)/XEN_GETPAGEFRAMEINFO3_MAX_SIZE);
             goto failed;
         }
     }
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 8fbbf3aeb3..daf733de4f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -421,7 +421,7 @@ long arch_do_domctl(
         /* Games to allow this code block to handle a compat guest. */
         void __user *guest_handle = domctl->u.getpageframeinfo3.array.p;
 
-        if ( unlikely(num > 1024) ||
+        if ( unlikely(num > XEN_GETPAGEFRAMEINFO3_MAX_SIZE) ||
              unlikely(num != domctl->u.getpageframeinfo3.num) )
         {
             ret = -E2BIG;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ec7a860afc..8d39cc8f74 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -137,6 +137,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
 #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
 #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
 
+#define XEN_GETPAGEFRAMEINFO3_MAX_SIZE 1024U
+
 /* XEN_DOMCTL_getpageframeinfo3 */
 struct xen_domctl_getpageframeinfo3 {
     /* IN variables. */

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[Jan Beulich]

>>> On 09.03.18 at 17:17, <olaf@aepfle.de> wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -137,6 +137,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
>  #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
>  #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
>  
> +#define XEN_GETPAGEFRAMEINFO3_MAX_SIZE 1024U

This is an implementation detail; it shouldn't be made part of the
public interface. If there's a need for user land to know the value,
and if there's currently no way to query it, that's what you
would want to add.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[George Dunlap]

On 03/09/2018 04:25 PM, Jan Beulich wrote:
>>>> On 09.03.18 at 17:17, <olaf@aepfle.de> wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -137,6 +137,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
>>  #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
>>  #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
>>  
>> +#define XEN_GETPAGEFRAMEINFO3_MAX_SIZE 1024U
> 
> This is an implementation detail; it shouldn't be made part of the
> public interface. If there's a need for user land to know the value,
> and if there's currently no way to query it, that's what you
> would want to add.

But the domctl interface isn't stable, right?  There's no need to make a
flexible backwards-compatible interface between libxc and Xen; sharing a
#define should be fine, as long as there's only one place to change it.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[Jan Beulich]

>>> On 09.03.18 at 17:30, <george.dunlap@citrix.com> wrote:
> On 03/09/2018 04:25 PM, Jan Beulich wrote:
>>>>> On 09.03.18 at 17:17, <olaf@aepfle.de> wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -137,6 +137,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
>>>  #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
>>>  #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
>>>  
>>> +#define XEN_GETPAGEFRAMEINFO3_MAX_SIZE 1024U
>> 
>> This is an implementation detail; it shouldn't be made part of the
>> public interface. If there's a need for user land to know the value,
>> and if there's currently no way to query it, that's what you
>> would want to add.
> 
> But the domctl interface isn't stable, right?  There's no need to make a
> flexible backwards-compatible interface between libxc and Xen; sharing a
> #define should be fine, as long as there's only one place to change it.

Well, strictly speaking this is an option. But I would prefer if we didn't
abuse the "is not a stable interface" property, which this changes
feels like it would. 

Furthermore it hasn't become clear to me why, if this hard coded
number is deemed a problem, we don't get rid of it altogether.
Domctl-s have long gained the ability to be preemptible - there
various examples.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[George Dunlap]

On 03/09/2018 04:43 PM, Jan Beulich wrote:
>>>> On 09.03.18 at 17:30, <george.dunlap@citrix.com> wrote:
>> On 03/09/2018 04:25 PM, Jan Beulich wrote:
>>>>>> On 09.03.18 at 17:17, <olaf@aepfle.de> wrote:
>>>> --- a/xen/include/public/domctl.h
>>>> +++ b/xen/include/public/domctl.h
>>>> @@ -137,6 +137,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
>>>>  #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
>>>>  #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
>>>>  
>>>> +#define XEN_GETPAGEFRAMEINFO3_MAX_SIZE 1024U
>>>
>>> This is an implementation detail; it shouldn't be made part of the
>>> public interface. If there's a need for user land to know the value,
>>> and if there's currently no way to query it, that's what you
>>> would want to add.
>>
>> But the domctl interface isn't stable, right?  There's no need to make a
>> flexible backwards-compatible interface between libxc and Xen; sharing a
>> #define should be fine, as long as there's only one place to change it.
> 
> Well, strictly speaking this is an option. But I would prefer if we didn't
> abuse the "is not a stable interface" property, which this changes
> feels like it would. 
> 
> Furthermore it hasn't become clear to me why, if this hard coded
> number is deemed a problem, we don't get rid of it altogether.
> Domctl-s have long gained the ability to be preemptible - there
> various examples.

Yes, making it preemptible and removing the limit is probably a better
option.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[Olaf Hering]

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

The point was to document the counter part. Grep -w 1024 is not helpful.

Olaf

[-- Attachment #1.2: Type: text/html, Size: 88 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[Andrew Cooper]

On 09/03/18 16:49, George Dunlap wrote:
> On 03/09/2018 04:43 PM, Jan Beulich wrote:
>>>>> On 09.03.18 at 17:30, <george.dunlap@citrix.com> wrote:
>>> On 03/09/2018 04:25 PM, Jan Beulich wrote:
>>>>>>> On 09.03.18 at 17:17, <olaf@aepfle.de> wrote:
>>>>> --- a/xen/include/public/domctl.h
>>>>> +++ b/xen/include/public/domctl.h
>>>>> @@ -137,6 +137,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
>>>>>  #define XEN_DOMCTL_PFINFO_BROKEN  (0xdU<<28) /* broken page */
>>>>>  #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
>>>>>  
>>>>> +#define XEN_GETPAGEFRAMEINFO3_MAX_SIZE 1024U
>>>> This is an implementation detail; it shouldn't be made part of the
>>>> public interface. If there's a need for user land to know the value,
>>>> and if there's currently no way to query it, that's what you
>>>> would want to add.
>>> But the domctl interface isn't stable, right?  There's no need to make a
>>> flexible backwards-compatible interface between libxc and Xen; sharing a
>>> #define should be fine, as long as there's only one place to change it.
>> Well, strictly speaking this is an option. But I would prefer if we didn't
>> abuse the "is not a stable interface" property, which this changes
>> feels like it would. 
>>
>> Furthermore it hasn't become clear to me why, if this hard coded
>> number is deemed a problem, we don't get rid of it altogether.
>> Domctl-s have long gained the ability to be preemptible - there
>> various examples.
> Yes, making it preemptible and removing the limit is probably a better
> option.

The 1024 limit was from the 32bit days, where there was a single domheap
page allocated as a bounce buffer.

The logic nowadays is trivial to make continuable, and would remove the
fixed arbitrary upper limit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] getpageframeinfo3: replace hardcoded batchsize with constant

[George Dunlap]

On 03/09/2018 04:55 PM, Olaf Hering wrote:
> The point was to document the counter part. Grep -w 1024 is not helpful.

Of course -- your patch certainly makes the codebase better by making
this number set in only one place, and in making it easier to find who
else is depending on this number with grep.

Jan is saying, however, it's not clear why we have such an arbitrary
limit in the first place.  Andy claims it's a remnant from the days when
we were passing a single page rather than an arbitrary array; it also
serves to  make sure the hypercall doesn't run too long.  But if we use
normal hypercall preemption, we don't need to have this limit at all.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel