[PATCH 0/5] various clean-ups

[PATCH 0/5] various clean-ups

[Juergen Gross]

Doing some clean-ups all somehow related to xen/include/xen/sched.h.


Juergen Gross (5):
  libelf: use enum instead of hard coded values in elf_dom_parms.pae
  xen: make use of new pae enum in hypervisor
  libxc: make use of new pae enum in libxc
  xen: remove unused macros from sched.h
  xen: clean up VPF flags macros

 tools/libxc/xc_dom_binloader.c     |  2 +-
 tools/libxc/xc_dom_elfloader.c     |  8 ++++----
 tools/libxc/xc_dom_x86.c           |  6 +++---
 tools/libxc/xg_private.h           |  6 ------
 xen/arch/x86/domain_build.c        |  6 +++---
 xen/common/domctl.c                |  2 +-
 xen/common/libelf/libelf-dominfo.c |  8 ++++----
 xen/include/xen/libelf.h           |  9 ++++++++-
 xen/include/xen/sched.h            | 17 +----------------
 9 files changed, 25 insertions(+), 39 deletions(-)

-- 
2.1.4

[PATCH 1/5] libelf: use enum instead of hard coded values in elf_dom_parms.pae

[Juergen Gross]

Use an enum for elf_dom_parms.pae instead of just hard coding the
values when setting the information.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/libelf/libelf-dominfo.c | 8 ++++----
 xen/include/xen/libelf.h           | 9 ++++++++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index f929968..3de1c23 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -172,9 +172,9 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
         break;
     case XEN_ELFNOTE_PAE_MODE:
         if ( !strcmp(str, "yes") )
-            parms->pae = 2 /* extended_cr3 */;
+            parms->pae = XEN_PAE_EXTCR3;
         if ( strstr(str, "bimodal") )
-            parms->pae = 3 /* bimodal */;
+            parms->pae = XEN_PAE_BIMODAL;
         break;
     case XEN_ELFNOTE_BSD_SYMTAB:
         if ( !strcmp(str, "yes") )
@@ -317,9 +317,9 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
         if ( !strcmp(name, "PAE") )
         {
             if ( !strcmp(value, "yes[extended-cr3]") )
-                parms->pae = 2 /* extended_cr3 */;
+                parms->pae = XEN_PAE_EXTCR3;
             else if ( !strncmp(value, "yes", 3) )
-                parms->pae = 1 /* yes */;
+                parms->pae = XEN_PAE_YES;
         }
         if ( !strcmp(name, "BSD_SYMTAB") )
             parms->bsd_symtab = 1;
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 6393040..d7045f6 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -393,6 +393,13 @@ enum xen_elfnote_type {
     XEN_ENT_STR  = 2
 };
 
+enum xen_pae_type {
+    XEN_PAE_NO      = 0,
+    XEN_PAE_YES     = 1,
+    XEN_PAE_EXTCR3  = 2,
+    XEN_PAE_BIMODAL = 3
+};
+
 struct xen_elfnote {
     enum xen_elfnote_type type;
     const char *name;
@@ -414,7 +421,7 @@ struct elf_dom_parms {
     char guest_ver[16];
     char xen_ver[16];
     char loader[16];
-    int pae; /* some kind of enum apparently */
+    enum xen_pae_type pae;
     bool bsd_symtab;
     uint64_t virt_base;
     uint64_t virt_entry;
-- 
2.1.4

[PATCH 2/5] xen: make use of new pae enum in hypervisor

[Juergen Gross]

Instead of using own defines for the possible values of pae_kernel
make use of the new libelf enum.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/domain_build.c | 6 +++---
 xen/include/xen/sched.h     | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 18cf6aa..cd27864 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -953,8 +953,8 @@ int __init construct_dom0(
     compat32   = 0;
     machine = elf_uval(&elf, elf.ehdr, e_machine);
     printk(" Xen  kernel: 64-bit, lsb, compat32\n");
-    if (elf_32bit(&elf) && parms.pae == PAEKERN_bimodal)
-        parms.pae = PAEKERN_extended_cr3;
+    if (elf_32bit(&elf) && parms.pae == XEN_PAE_BIMODAL)
+        parms.pae = XEN_PAE_EXTCR3;
     if (elf_32bit(&elf) && parms.pae && machine == EM_386)
     {
         compat32 = 1;
@@ -1005,7 +1005,7 @@ int __init construct_dom0(
 
     nr_pages = compute_dom0_nr_pages(d, &parms, initrd_len);
 
-    if ( parms.pae == PAEKERN_extended_cr3 )
+    if ( parms.pae == XEN_PAE_EXTCR3 )
             set_bit(VMASST_TYPE_pae_extended_cr3, &d->vm_assist);
 
     if ( (parms.virt_hv_start_low != UNSET_ADDR) && elf_32bit(&elf) )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 8053b5a..20d3865 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -475,10 +475,6 @@ struct domain_setup_info
     unsigned long v_kernstart;
     unsigned long v_kernend;
     unsigned long v_kernentry;
-#define PAEKERN_no           0
-#define PAEKERN_yes          1
-#define PAEKERN_extended_cr3 2
-#define PAEKERN_bimodal      3
     unsigned int  pae_kernel;
     /* Initialised by loader: Private. */
     unsigned long elf_paddr_offset;
-- 
2.1.4

Re: [PATCH 2/5] xen: make use of new pae enum in hypervisor

[Jan Beulich]

>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -953,8 +953,8 @@ int __init construct_dom0(
>      compat32   = 0;
>      machine = elf_uval(&elf, elf.ehdr, e_machine);
>      printk(" Xen  kernel: 64-bit, lsb, compat32\n");
> -    if (elf_32bit(&elf) && parms.pae == PAEKERN_bimodal)
> -        parms.pae = PAEKERN_extended_cr3;
> +    if (elf_32bit(&elf) && parms.pae == XEN_PAE_BIMODAL)
> +        parms.pae = XEN_PAE_EXTCR3;

I think this and the first patch can't validly be split: You're altering
the values stored in parms.pae in the first one, but taking this code
together with ...

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -475,10 +475,6 @@ struct domain_setup_info
>      unsigned long v_kernstart;
>      unsigned long v_kernend;
>      unsigned long v_kernentry;
> -#define PAEKERN_no           0
> -#define PAEKERN_yes          1
> -#define PAEKERN_extended_cr3 2
> -#define PAEKERN_bimodal      3
>      unsigned int  pae_kernel;

... this, the PAEKERN_* values were already meant to be in sync
with the literal numbers used in libelf code.

Furthermore there's no single reference to a pae_kernel field
throughout the xen/ subtree, which suggests the field was already
dead. Even more - it looks like the whole structure is unused; the
only references are apparently dead function declarations in
xen/elf.h. So the two patches should be folded, and the resulting
one extended.

Jan

[PATCH 3/5] libxc: make use of new pae enum in libxc

[Juergen Gross]

Instead of using hard coded values for the possible values of parms.pae
make use of the new libelf enum.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_dom_binloader.c | 2 +-
 tools/libxc/xc_dom_elfloader.c | 8 ++++----
 tools/libxc/xc_dom_x86.c       | 6 +++---
 tools/libxc/xg_private.h       | 6 ------
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
index 740601a..d6f7f2a 100644
--- a/tools/libxc/xc_dom_binloader.c
+++ b/tools/libxc/xc_dom_binloader.c
@@ -238,7 +238,7 @@ static int xc_dom_parse_bin_kernel(struct xc_dom_image *dom)
         {
             DOMPRINTF("%s: PAE fixup", __FUNCTION__);
             dom->guest_type = "xen-3.0-x86_32p";
-            dom->parms.pae  = 2;
+            dom->parms.pae  = XEN_PAE_EXTCR3;
         }
         break;
     }
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 66ea9d6..82524c9 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -61,14 +61,14 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
     case EM_386:
         switch ( dom->parms.pae )
         {
-        case 3 /* PAEKERN_bimodal */:
+        case XEN_PAE_BIMODAL:
             if ( strstr(dom->xen_caps, "xen-3.0-x86_32p") )
                 return "xen-3.0-x86_32p";
             return "xen-3.0-x86_32";
-        case PAEKERN_extended_cr3:
-        case PAEKERN_yes:
+        case XEN_PAE_EXTCR3:
+        case XEN_PAE_YES:
             return "xen-3.0-x86_32p";
-        case PAEKERN_no:
+        case XEN_PAE_NO:
         default:
             return "xen-3.0-x86_32";
         }
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 05fb0ce..e2f3792 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -249,7 +249,7 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
     xen_pfn_t pgpfn;
     xen_pfn_t l3mfn = xc_dom_p2m_guest(dom, l3pfn);
 
-    if ( dom->parms.pae == 1 )
+    if ( dom->parms.pae == XEN_PAE_YES )
     {
         if ( l3mfn >= 0x100000 )
             l3mfn = move_l3_below_4G(dom, l3pfn, l3mfn);
@@ -602,8 +602,8 @@ static int vcpu_x86_32(struct xc_dom_image *dom, void *ptr)
     ctxt->user_regs.eflags = 1 << 9; /* Interrupt Enable */
 
     ctxt->flags = VGCF_in_kernel_X86_32 | VGCF_online_X86_32;
-    if ( dom->parms.pae == 2 /* extended_cr3 */ ||
-         dom->parms.pae == 3 /* bimodal */ )
+    if ( dom->parms.pae == XEN_PAE_EXTCR3 ||
+         dom->parms.pae == XEN_PAE_BIMODAL )
         ctxt->vm_assist |= (1UL << VMASST_TYPE_pae_extended_cr3);
 
     cr3_pfn = xc_dom_p2m_guest(dom, dom->pgtables_seg.pfn);
diff --git a/tools/libxc/xg_private.h b/tools/libxc/xg_private.h
index 07eeb67..5544897 100644
--- a/tools/libxc/xg_private.h
+++ b/tools/libxc/xg_private.h
@@ -165,12 +165,6 @@ static inline xen_pfn_t xc_pfn_to_mfn(xen_pfn_t pfn, xen_pfn_t *p2m,
 #define MFN_MASK_X86    ((1ULL << (MADDR_BITS_X86 - PAGE_SHIFT_X86)) - 1)
 #define MADDR_MASK_X86  (MFN_MASK_X86 << PAGE_SHIFT_X86)
 
-
-#define PAEKERN_no           0
-#define PAEKERN_yes          1
-#define PAEKERN_extended_cr3 2
-#define PAEKERN_bimodal      3
-
 int pin_table(xc_interface *xch, unsigned int type, unsigned long mfn,
               domid_t dom);
 
-- 
2.1.4

Re: [PATCH 3/5] libxc: make use of new pae enum in libxc

[Ian Campbell]

On Fri, 2015-09-25 at 13:54 +0200, Juergen Gross wrote:
> Instead of using hard coded values for the possible values of parms.pae
> make use of the new libelf enum.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

I'll assume this will be picked up by Jan along with the x86 hypervisor
side once he's happy with it.

Re: [PATCH 3/5] libxc: make use of new pae enum in libxc

[Jan Beulich]

>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
> Instead of using hard coded values for the possible values of parms.pae
> make use of the new libelf enum.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Along the same lines as just said in reply to patch 2, I don't think the
changes here should be split off of that one either.

Jan

[PATCH 4/5] xen: remove unused macros from sched.h

[Juergen Gross]

The macros num_cpupool_cpus() and domain_is_locked() aren't used by
anyone. Remove them.

Signed-off-by: Juergen Gross <jgross@ssue.com>
---
 xen/include/xen/sched.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 20d3865..b5190b5 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -260,7 +260,6 @@ struct vcpu
 /* Per-domain lock can be recursively acquired in fault handlers. */
 #define domain_lock(d) spin_lock_recursive(&(d)->domain_lock)
 #define domain_unlock(d) spin_unlock_recursive(&(d)->domain_lock)
-#define domain_is_locked(d) spin_is_locked(&(d)->domain_lock)
 
 /* VM event */
 struct vm_event_domain
@@ -890,8 +889,6 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
 void schedule_dump(struct cpupool *c);
 extern void dump_runq(unsigned char key);
 
-#define num_cpupool_cpus(c) cpumask_weight((c)->cpu_valid)
-
 void arch_do_physinfo(xen_sysctl_physinfo_t *pi);
 
 #endif /* __SCHED_H__ */
-- 
2.1.4

Re: [PATCH 4/5] xen: remove unused macros from sched.h

[George Dunlap]

On Fri, Sep 25, 2015 at 12:54 PM, Juergen Gross <jgross@suse.com> wrote:
> The macros num_cpupool_cpus() and domain_is_locked() aren't used by
> anyone. Remove them.
>
> Signed-off-by: Juergen Gross <jgross@ssue.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

[PATCH 5/5] xen: clean up VPF flags macros

[Juergen Gross]

Per-VCPU pause flags in sched.h are defined as bit positions and as
values derived from the bit defines. There is only one user of a value
which can be easily converted to use a bit number as well.

Remove the value definitions and do the conversion for the only user.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/domctl.c     |  2 +-
 xen/include/xen/sched.h | 10 +---------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 9e0fef5..e44bc4c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
         info->max_vcpu_id = v->vcpu_id;
         if ( !test_bit(_VPF_down, &v->pause_flags) )
         {
-            if ( !(v->pause_flags & VPF_blocked) )
+            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
                 flags &= ~XEN_DOMINF_blocked;
             if ( v->is_running )
                 flags |= XEN_DOMINF_running;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b5190b5..bc7d5cb 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -745,32 +745,24 @@ static inline struct domain *next_domain_in_cpupool(
        (_v) = (_v)->next_in_list )
 
 /*
- * Per-VCPU pause flags.
+ * Per-VCPU pause flags (bit positions).
  */
  /* Domain is blocked waiting for an event. */
 #define _VPF_blocked         0
-#define VPF_blocked          (1UL<<_VPF_blocked)
  /* VCPU is offline. */
 #define _VPF_down            1
-#define VPF_down             (1UL<<_VPF_down)
  /* VCPU is blocked awaiting an event to be consumed by Xen. */
 #define _VPF_blocked_in_xen  2
-#define VPF_blocked_in_xen   (1UL<<_VPF_blocked_in_xen)
  /* VCPU affinity has changed: migrating to a new CPU. */
 #define _VPF_migrating       3
-#define VPF_migrating        (1UL<<_VPF_migrating)
  /* VCPU is blocked due to missing mem_paging ring. */
 #define _VPF_mem_paging      4
-#define VPF_mem_paging       (1UL<<_VPF_mem_paging)
  /* VCPU is blocked due to missing mem_access ring. */
 #define _VPF_mem_access      5
-#define VPF_mem_access       (1UL<<_VPF_mem_access)
  /* VCPU is blocked due to missing mem_sharing ring. */
 #define _VPF_mem_sharing     6
-#define VPF_mem_sharing      (1UL<<_VPF_mem_sharing)
  /* VCPU is being reset. */
 #define _VPF_in_reset        7
-#define VPF_in_reset         (1UL<<_VPF_in_reset)
 
 static inline int vcpu_runnable(struct vcpu *v)
 {
-- 
2.1.4

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Jan Beulich]

>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
> Per-VCPU pause flags in sched.h are defined as bit positions and as
> values derived from the bit defines. There is only one user of a value
> which can be easily converted to use a bit number as well.

I'm not convinced:

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>          info->max_vcpu_id = v->vcpu_id;
>          if ( !test_bit(_VPF_down, &v->pause_flags) )
>          {
> -            if ( !(v->pause_flags & VPF_blocked) )
> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )

test_bit() is quite a bit more complex an operation than a simple &,
and with (on x86) even constant_test_bit() involving a cast to
a pointer to volatile I'm afraid we can't even hope that compilers
would produce identical code for both in cases like this one (as that
casts limits freedom of the compiler). IOW I'd rather see other
test_bit(_VPF_...) uses converted the inverse way (which as a nice
but minor side effect would yield slightly smaller source code).

Jan

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Juergen Gross]

On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>> Per-VCPU pause flags in sched.h are defined as bit positions and as
>> values derived from the bit defines. There is only one user of a value
>> which can be easily converted to use a bit number as well.
>
> I'm not convinced:
>
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>           info->max_vcpu_id = v->vcpu_id;
>>           if ( !test_bit(_VPF_down, &v->pause_flags) )
>>           {
>> -            if ( !(v->pause_flags & VPF_blocked) )
>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>
> test_bit() is quite a bit more complex an operation than a simple &,
> and with (on x86) even constant_test_bit() involving a cast to
> a pointer to volatile I'm afraid we can't even hope that compilers
> would produce identical code for both in cases like this one (as that
> casts limits freedom of the compiler). IOW I'd rather see other
> test_bit(_VPF_...) uses converted the inverse way (which as a nice
> but minor side effect would yield slightly smaller source code).

What about introducing __test_bit() being a variant which can be
reordered by omitting the volatile modifier? I think this would have
the same effect. And we could still get rid of many double definitions
of the same bit. Even if the mask definition of a bit is not error prone
by relying on the definition of the bit position, it makes it harder to
find all users of this bit.


Juergen

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Jan Beulich]

>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>> Per-VCPU pause flags in sched.h are defined as bit positions and as
>>> values derived from the bit defines. There is only one user of a value
>>> which can be easily converted to use a bit number as well.
>>
>> I'm not convinced:
>>
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>>>           info->max_vcpu_id = v->vcpu_id;
>>>           if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>           {
>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>
>> test_bit() is quite a bit more complex an operation than a simple &,
>> and with (on x86) even constant_test_bit() involving a cast to
>> a pointer to volatile I'm afraid we can't even hope that compilers
>> would produce identical code for both in cases like this one (as that
>> casts limits freedom of the compiler). IOW I'd rather see other
>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>> but minor side effect would yield slightly smaller source code).
> 
> What about introducing __test_bit() being a variant which can be
> reordered by omitting the volatile modifier? I think this would have
> the same effect.

I'm not convinced it always would - the inline function is still more
complex than the plain operation.

> And we could still get rid of many double definitions
> of the same bit. Even if the mask definition of a bit is not error prone
> by relying on the definition of the bit position, it makes it harder to
> find all users of this bit.

Why so? Just omit the leading underscore when grep-ing, and you'll
find all instances (less preprocessor token concatenation, but that's
orthogonal).

Jan

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Juergen Gross]

On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>>> Per-VCPU pause flags in sched.h are defined as bit positions and as
>>>> values derived from the bit defines. There is only one user of a value
>>>> which can be easily converted to use a bit number as well.
>>>
>>> I'm not convinced:
>>>
>>>> --- a/xen/common/domctl.c
>>>> +++ b/xen/common/domctl.c
>>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct
>> xen_domctl_getdomaininfo *info)
>>>>            info->max_vcpu_id = v->vcpu_id;
>>>>            if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>>            {
>>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>>
>>> test_bit() is quite a bit more complex an operation than a simple &,
>>> and with (on x86) even constant_test_bit() involving a cast to
>>> a pointer to volatile I'm afraid we can't even hope that compilers
>>> would produce identical code for both in cases like this one (as that
>>> casts limits freedom of the compiler). IOW I'd rather see other
>>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>>> but minor side effect would yield slightly smaller source code).
>>
>> What about introducing __test_bit() being a variant which can be
>> reordered by omitting the volatile modifier? I think this would have
>> the same effect.
>
> I'm not convinced it always would - the inline function is still more
> complex than the plain operation.

Depends on the way it is done. What about:

#define __test_bit(nr, addr) ({                         \
     if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
     (__builtin_constant_p(nr) ?                         \
      !!(*(addr) & ((typeof)(*(addr))1 << (nr))) :       \
      __variable_test_bit((nr),(addr)));                 \
})

It would even be possible to drop the test for bitop_bad_size(addr) in
the constant case.

>> And we could still get rid of many double definitions
>> of the same bit. Even if the mask definition of a bit is not error prone
>> by relying on the definition of the bit position, it makes it harder to
>> find all users of this bit.
>
> Why so? Just omit the leading underscore when grep-ing, and you'll
> find all instances (less preprocessor token concatenation, but that's
> orthogonal).

I do use grep for this purpose occasionally, but I prefer tools like
cscope. BTW: IMO using grep the way you are suggesting here is annoying
for cases where the search string is contained in other items.

Juergen

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Jan Beulich]

>>> On 28.09.15 at 09:29, <JGross@suse.com> wrote:
> On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
>>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>>>> Per-VCPU pause flags in sched.h are defined as bit positions and as
>>>>> values derived from the bit defines. There is only one user of a value
>>>>> which can be easily converted to use a bit number as well.
>>>>
>>>> I'm not convinced:
>>>>
>>>>> --- a/xen/common/domctl.c
>>>>> +++ b/xen/common/domctl.c
>>>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct
>>> xen_domctl_getdomaininfo *info)
>>>>>            info->max_vcpu_id = v->vcpu_id;
>>>>>            if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>>>            {
>>>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>>>
>>>> test_bit() is quite a bit more complex an operation than a simple &,
>>>> and with (on x86) even constant_test_bit() involving a cast to
>>>> a pointer to volatile I'm afraid we can't even hope that compilers
>>>> would produce identical code for both in cases like this one (as that
>>>> casts limits freedom of the compiler). IOW I'd rather see other
>>>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>>>> but minor side effect would yield slightly smaller source code).
>>>
>>> What about introducing __test_bit() being a variant which can be
>>> reordered by omitting the volatile modifier? I think this would have
>>> the same effect.
>>
>> I'm not convinced it always would - the inline function is still more
>> complex than the plain operation.
> 
> Depends on the way it is done. What about:
> 
> #define __test_bit(nr, addr) ({                         \
>      if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>      (__builtin_constant_p(nr) ?                         \
>       !!(*(addr) & ((typeof)(*(addr))1 << (nr))) :       \
>       __variable_test_bit((nr),(addr)));                 \
> })

But that's not correct - addr may point to wider than a single entry
array, irrespective of whether nr is a compile time constant.

> It would even be possible to drop the test for bitop_bad_size(addr) in
> the constant case.

In which case 1 << nr may reference a bit beyond the type
of *addr.

>>> And we could still get rid of many double definitions
>>> of the same bit. Even if the mask definition of a bit is not error prone
>>> by relying on the definition of the bit position, it makes it harder to
>>> find all users of this bit.
>>
>> Why so? Just omit the leading underscore when grep-ing, and you'll
>> find all instances (less preprocessor token concatenation, but that's
>> orthogonal).
> 
> I do use grep for this purpose occasionally, but I prefer tools like
> cscope. BTW: IMO using grep the way you are suggesting here is annoying
> for cases where the search string is contained in other items.

There may be cases, yes, but surely not this one: How likely is it for
some other variable name to include, say, VPF_blocked?

Jan

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Juergen Gross]

On 09/28/2015 09:52 AM, Jan Beulich wrote:
>>>> On 28.09.15 at 09:29, <JGross@suse.com> wrote:
>> On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
>>>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>>>>> Per-VCPU pause flags in sched.h are defined as bit positions and as
>>>>>> values derived from the bit defines. There is only one user of a value
>>>>>> which can be easily converted to use a bit number as well.
>>>>>
>>>>> I'm not convinced:
>>>>>
>>>>>> --- a/xen/common/domctl.c
>>>>>> +++ b/xen/common/domctl.c
>>>>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct
>>>> xen_domctl_getdomaininfo *info)
>>>>>>             info->max_vcpu_id = v->vcpu_id;
>>>>>>             if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>>>>             {
>>>>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>>>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>>>>
>>>>> test_bit() is quite a bit more complex an operation than a simple &,
>>>>> and with (on x86) even constant_test_bit() involving a cast to
>>>>> a pointer to volatile I'm afraid we can't even hope that compilers
>>>>> would produce identical code for both in cases like this one (as that
>>>>> casts limits freedom of the compiler). IOW I'd rather see other
>>>>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>>>>> but minor side effect would yield slightly smaller source code).
>>>>
>>>> What about introducing __test_bit() being a variant which can be
>>>> reordered by omitting the volatile modifier? I think this would have
>>>> the same effect.
>>>
>>> I'm not convinced it always would - the inline function is still more
>>> complex than the plain operation.
>>
>> Depends on the way it is done. What about:
>>
>> #define __test_bit(nr, addr) ({                         \
>>       if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>>       (__builtin_constant_p(nr) ?                         \
>>        !!(*(addr) & ((typeof)(*(addr))1 << (nr))) :       \
>>        __variable_test_bit((nr),(addr)));                 \
>> })
>
> But that's not correct - addr may point to wider than a single entry
> array, irrespective of whether nr is a compile time constant.
>
>> It would even be possible to drop the test for bitop_bad_size(addr) in
>> the constant case.
>
> In which case 1 << nr may reference a bit beyond the type
> of *addr.

Hmm, yes, you are right, of course.

It could be fixed, however.

The question is: does it make sense to follow this path any longer,
or would you reject it even in case of correctness? I wouldn't mind
either way, I just don't want to waste time (mine and yours).

>>>> And we could still get rid of many double definitions
>>>> of the same bit. Even if the mask definition of a bit is not error prone
>>>> by relying on the definition of the bit position, it makes it harder to
>>>> find all users of this bit.
>>>
>>> Why so? Just omit the leading underscore when grep-ing, and you'll
>>> find all instances (less preprocessor token concatenation, but that's
>>> orthogonal).
>>
>> I do use grep for this purpose occasionally, but I prefer tools like
>> cscope. BTW: IMO using grep the way you are suggesting here is annoying
>> for cases where the search string is contained in other items.
>
> There may be cases, yes, but surely not this one: How likely is it for
> some other variable name to include, say, VPF_blocked?

I think we both agree that [_]VPF_blocked poses no problem using grep.

It's more a matter of taste and what people are used to. If someone
is using cscope for that purpose on a regular basis he will either have
to search for two different variables or he will have to use another
tool, possibly after seeing the definition of VPF_blocked depending on
_VPF_blocked. Both cases will be annoying as an extra action is required
for him.


Juergen

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Jan Beulich]

>>> On 28.09.15 at 10:15, <JGross@suse.com> wrote:
> On 09/28/2015 09:52 AM, Jan Beulich wrote:
>>>>> On 28.09.15 at 09:29, <JGross@suse.com> wrote:
>>> On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>>>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
>>>>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>>>>>> --- a/xen/common/domctl.c
>>>>>>> +++ b/xen/common/domctl.c
>>>>>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct
>>>>> xen_domctl_getdomaininfo *info)
>>>>>>>             info->max_vcpu_id = v->vcpu_id;
>>>>>>>             if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>>>>>             {
>>>>>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>>>>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>>>>>
>>>>>> test_bit() is quite a bit more complex an operation than a simple &,
>>>>>> and with (on x86) even constant_test_bit() involving a cast to
>>>>>> a pointer to volatile I'm afraid we can't even hope that compilers
>>>>>> would produce identical code for both in cases like this one (as that
>>>>>> casts limits freedom of the compiler). IOW I'd rather see other
>>>>>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>>>>>> but minor side effect would yield slightly smaller source code).
>>>>>
>>>>> What about introducing __test_bit() being a variant which can be
>>>>> reordered by omitting the volatile modifier? I think this would have
>>>>> the same effect.
>>>>
>>>> I'm not convinced it always would - the inline function is still more
>>>> complex than the plain operation.
>>>
>>> Depends on the way it is done. What about:
>>>
>>> #define __test_bit(nr, addr) ({                         \
>>>       if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>>>       (__builtin_constant_p(nr) ?                         \
>>>        !!(*(addr) & ((typeof)(*(addr))1 << (nr))) :       \
>>>        __variable_test_bit((nr),(addr)));                 \
>>> })
>>
>> But that's not correct - addr may point to wider than a single entry
>> array, irrespective of whether nr is a compile time constant.
>>
>>> It would even be possible to drop the test for bitop_bad_size(addr) in
>>> the constant case.
>>
>> In which case 1 << nr may reference a bit beyond the type
>> of *addr.
> 
> Hmm, yes, you are right, of course.
> 
> It could be fixed, however.
> 
> The question is: does it make sense to follow this path any longer,
> or would you reject it even in case of correctness? I wouldn't mind
> either way, I just don't want to waste time (mine and yours).

I continue to think that the better route would be to get rid of the
unnecessary test_bit() uses in favor of the shorter and less
restrictive (to the compiler) & operation.

Jan

Re: [PATCH 5/5] xen: clean up VPF flags macros

[Juergen Gross]

On 09/28/2015 10:34 AM, Jan Beulich wrote:
>>>> On 28.09.15 at 10:15, <JGross@suse.com> wrote:
>> On 09/28/2015 09:52 AM, Jan Beulich wrote:
>>>>>> On 28.09.15 at 09:29, <JGross@suse.com> wrote:
>>>> On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>>>>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
>>>>>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>>>>>>> --- a/xen/common/domctl.c
>>>>>>>> +++ b/xen/common/domctl.c
>>>>>>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct
>>>>>> xen_domctl_getdomaininfo *info)
>>>>>>>>              info->max_vcpu_id = v->vcpu_id;
>>>>>>>>              if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>>>>>>              {
>>>>>>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>>>>>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>>>>>>
>>>>>>> test_bit() is quite a bit more complex an operation than a simple &,
>>>>>>> and with (on x86) even constant_test_bit() involving a cast to
>>>>>>> a pointer to volatile I'm afraid we can't even hope that compilers
>>>>>>> would produce identical code for both in cases like this one (as that
>>>>>>> casts limits freedom of the compiler). IOW I'd rather see other
>>>>>>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>>>>>>> but minor side effect would yield slightly smaller source code).
>>>>>>
>>>>>> What about introducing __test_bit() being a variant which can be
>>>>>> reordered by omitting the volatile modifier? I think this would have
>>>>>> the same effect.
>>>>>
>>>>> I'm not convinced it always would - the inline function is still more
>>>>> complex than the plain operation.
>>>>
>>>> Depends on the way it is done. What about:
>>>>
>>>> #define __test_bit(nr, addr) ({                         \
>>>>        if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>>>>        (__builtin_constant_p(nr) ?                         \
>>>>         !!(*(addr) & ((typeof)(*(addr))1 << (nr))) :       \
>>>>         __variable_test_bit((nr),(addr)));                 \
>>>> })
>>>
>>> But that's not correct - addr may point to wider than a single entry
>>> array, irrespective of whether nr is a compile time constant.
>>>
>>>> It would even be possible to drop the test for bitop_bad_size(addr) in
>>>> the constant case.
>>>
>>> In which case 1 << nr may reference a bit beyond the type
>>> of *addr.
>>
>> Hmm, yes, you are right, of course.
>>
>> It could be fixed, however.
>>
>> The question is: does it make sense to follow this path any longer,
>> or would you reject it even in case of correctness? I wouldn't mind
>> either way, I just don't want to waste time (mine and yours).
>
> I continue to think that the better route would be to get rid of the
> unnecessary test_bit() uses in favor of the shorter and less
> restrictive (to the compiler) & operation.

Okay, I'll follow that route then.


Juergen

Re: [PATCH 0/5] various clean-ups

[Andrew Cooper]

On 25/09/15 12:54, Juergen Gross wrote:
> Doing some clean-ups all somehow related to xen/include/xen/sched.h.
>
>
> Juergen Gross (5):
>    libelf: use enum instead of hard coded values in elf_dom_parms.pae
>    xen: make use of new pae enum in hypervisor
>    libxc: make use of new pae enum in libxc
>    xen: remove unused macros from sched.h
>    xen: clean up VPF flags macros

All changes look good.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>