[PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()

[PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()

[Jan Beulich]

In the long run I think we want to do away with these type-unsafe
interfaces, the more that they also request (typically) excess
alignment. This series of entirely independent patches is
eliminating the instances where it's relatively clear that they're
not just "blob" allocations.

v2 only has commit messages extended.

1: x86/MCE: avoid effectively open-coding xmalloc_array()
2: x86/HVM: avoid effectively open-coding xmalloc_array()
3: x86/oprofile: avoid effectively open-coding xmalloc_array()
4: x86/IRQ: avoid over-alignment in alloc_pirq_struct()
5: EFI/runtime: avoid effectively open-coding xmalloc_array()
6: kexec: avoid effectively open-coding xzalloc_flex_struct()
7: video/lfb: avoid effectively open-coding xzalloc_array()
8: Arm/optee: don't open-code xzalloc_flex_struct()

Jan

[PATCH v2 1/8] x86/MCE: avoid effectively open-coding xmalloc_array()

[Jan Beulich]

There is a difference in generated code: xmalloc_bytes() forces
SMP_CACHE_BYTES alignment. But if code really cared about such higher
than default alignment, it should request so explicitly rather than
using a type-unsafe interface. And if e.g. cache line sharing was a
concern, the allocator itself should arrange to avoid such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mcheck/mctelem.c
+++ b/xen/arch/x86/cpu/mcheck/mctelem.c
@@ -345,7 +345,7 @@ void __init mctelem_init(unsigned int da
 
 	if ((mctctl.mctc_elems = xmalloc_array(struct mctelem_ent,
 	    MC_NENT)) == NULL ||
-	    (datarr = xmalloc_bytes(MC_NENT * datasz)) == NULL) {
+	    (datarr = xmalloc_array(char, MC_NENT * datasz)) == NULL) {
 		xfree(mctctl.mctc_elems);
 		printk("Allocations for MCA telemetry failed\n");
 		return;

[PATCH v2 2/8] x86/HVM: avoid effectively open-coding xmalloc_array()

[Jan Beulich]

There is a difference in generated code: xmalloc_bytes() forces
SMP_CACHE_BYTES alignment. But if code really cared about such higher
than default alignment, it should request so explicitly rather than
using a type-unsafe interface. And if e.g. cache line sharing was a
concern, the allocator itself should arrange to avoid such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1924,7 +1924,7 @@ static int hvmemul_rep_movs(
         dgpa -= bytes - bytes_per_rep;
 
     /* Allocate temporary buffer. Fall back to slow emulation if this fails. */
-    buf = xmalloc_bytes(bytes);
+    buf = xmalloc_array(char, bytes);
     if ( buf == NULL )
         return X86EMUL_UNHANDLEABLE;
 
@@ -2037,7 +2037,7 @@ static int hvmemul_rep_stos(
         for ( ; ; )
         {
             bytes = *reps * bytes_per_rep;
-            buf = xmalloc_bytes(bytes);
+            buf = xmalloc_array(char, bytes);
             if ( buf || *reps <= 1 )
                 break;
             *reps >>= 1;

[PATCH v2 3/8] x86/oprofile: avoid effectively open-coding xmalloc_array()

[Jan Beulich]

There is a difference in generated code: xmalloc_bytes() forces
SMP_CACHE_BYTES alignment. But if code really cared about such higher
than default alignment, it should request so explicitly rather than
using a type-unsafe interface. And if e.g. cache line sharing was a
concern, the allocator itself should arrange to avoid such.

While at it also use XFREE() instead of open-coding it and change loop
induction variable types.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/oprofile/nmi_int.c
+++ b/xen/arch/x86/oprofile/nmi_int.c
@@ -142,30 +142,29 @@ static void nmi_save_registers(void * du
 
 static void free_msrs(void)
 {
-	int i;
+	unsigned int i;
+
 	for (i = 0; i < nr_cpu_ids; ++i) {
-		xfree(cpu_msrs[i].counters);
-		cpu_msrs[i].counters = NULL;
-		xfree(cpu_msrs[i].controls);
-		cpu_msrs[i].controls = NULL;
+		XFREE(cpu_msrs[i].counters);
+		XFREE(cpu_msrs[i].controls);
 	}
 }
 
 
 static int allocate_msrs(void)
 {
+	unsigned int i;
 	int success = 1;
-	size_t controls_size = sizeof(struct op_msr) * model->num_controls;
-	size_t counters_size = sizeof(struct op_msr) * model->num_counters;
 
-	int i;
 	for_each_online_cpu (i) {
-		cpu_msrs[i].counters = xmalloc_bytes(counters_size);
+		cpu_msrs[i].counters = xmalloc_array(struct op_msr,
+						     model->num_counters);
 		if (!cpu_msrs[i].counters) {
 			success = 0;
 			break;
 		}
-		cpu_msrs[i].controls = xmalloc_bytes(controls_size);
+		cpu_msrs[i].controls = xmalloc_array(struct op_msr,
+						     model->num_controls);
 		if (!cpu_msrs[i].controls) {
 			success = 0;
 			break;

[PATCH v2 4/8] x86/IRQ: avoid over-alignment in alloc_pirq_struct()

[Jan Beulich]

In particular in the PV case xzalloc_bytes() forcing SMP_CACHE_BYTES
alignment is counterproductive, as the allocation size there is only 40
bytes. And if the code really cared about such higher than default
alignment, it should request so explicitly rather than using a type-
unsafe interface. Plus if e.g. cache line sharing was a concern, the
allocator itself should arrange to avoid such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1313,9 +1313,12 @@ void cleanup_domain_irq_mapping(struct d
 
 struct pirq *alloc_pirq_struct(struct domain *d)
 {
-    size_t sz = is_hvm_domain(d) ? sizeof(struct pirq) :
-                                   offsetof(struct pirq, arch.hvm);
-    struct pirq *pirq = xzalloc_bytes(sz);
+    union pirq_pv {
+        char space[offsetof(struct pirq, arch.hvm)];
+        void *align;
+    };
+    struct pirq *pirq = is_hvm_domain(d) ? xzalloc(struct pirq)
+                                         : (void *)xzalloc(union pirq_pv);
 
     if ( pirq )
     {

[PATCH v2 5/8] EFI/runtime: avoid effectively open-coding x{m,z}alloc_array()

[Jan Beulich]

There is a difference in generated code: x{m,z}alloc_bytes() forces
SMP_CACHE_BYTES alignment. But if code really cared about such higher
than default alignment, it should request so explicitly rather than
using a type-unsafe interface. And if e.g. cache line sharing was a
concern, the allocator itself should arrange to avoid such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -486,7 +486,7 @@ int efi_runtime_call(struct xenpf_efi_ru
         size = op->u.get_variable.size;
         if ( size )
         {
-            data = xmalloc_bytes(size);
+            data = xmalloc_array(unsigned char, size);
             if ( !data )
             {
                 xfree(name);
@@ -536,7 +536,7 @@ int efi_runtime_call(struct xenpf_efi_ru
             return -EIO;
         }
 
-        data = xmalloc_bytes(op->u.set_variable.size);
+        data = xmalloc_array(unsigned char, op->u.set_variable.size);
         if ( !data )
             rc = -ENOMEM;
         else if ( copy_from_guest(data, op->u.set_variable.data,
@@ -571,7 +571,7 @@ int efi_runtime_call(struct xenpf_efi_ru
             return -EINVAL;
 
         size = op->u.get_next_variable_name.size;
-        name.raw = xzalloc_bytes(size);
+        name.raw = xzalloc_array(unsigned char, size);
         if ( !name.raw )
             return -ENOMEM;
         if ( copy_from_guest(name.raw, op->u.get_next_variable_name.name,

[PATCH v2 6/8] kexec: avoid effectively open-coding xzalloc_flex_struct()

[Jan Beulich]

There is a difference in generated code: xzalloc_bytes() forces
SMP_CACHE_BYTES alignment. But if code really cared about such higher
than default alignment, it should request so explicitly rather than
using a type-unsafe interface. And if e.g. cache line sharing was a
concern, the allocator itself should arrange to avoid such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -463,7 +463,10 @@ static void * alloc_from_crash_heap(cons
 /* Allocate a crash note buffer for a newly onlined cpu. */
 static int kexec_init_cpu_notes(const unsigned long cpu)
 {
-    Elf_Note * note = NULL;
+    struct elf_notes {
+        Elf_Note first;
+        unsigned char more[];
+    } *notes = NULL;
     int ret = 0;
     int nr_bytes = 0;
 
@@ -477,7 +480,8 @@ static int kexec_init_cpu_notes(const un
 
     /* If we dont care about the position of allocation, malloc. */
     if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
-        note = xzalloc_bytes(nr_bytes);
+        notes = xzalloc_flex_struct(struct elf_notes, more,
+                                    nr_bytes - sizeof(notes->first));
 
     /* Protect the write into crash_notes[] with a spinlock, as this function
      * is on a hotplug path and a hypercall path. */
@@ -490,26 +494,28 @@ static int kexec_init_cpu_notes(const un
         spin_unlock(&crash_notes_lock);
         /* Always return ok, because whether we successfully allocated or not,
          * another CPU has successfully allocated. */
-        xfree(note);
+        xfree(notes);
     }
     else
     {
         /* If we care about memory possition, alloc from the crash heap,
          * also protected by the crash_notes_lock. */
         if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
-            note = alloc_from_crash_heap(nr_bytes);
+            notes = alloc_from_crash_heap(nr_bytes);
 
-        crash_notes[cpu].start = note;
+        crash_notes[cpu].start = &notes->first;
         crash_notes[cpu].size = nr_bytes;
         spin_unlock(&crash_notes_lock);
 
         /* If the allocation failed, and another CPU did not beat us, give
          * up with ENOMEM. */
-        if ( ! note )
+        if ( ! notes )
             ret = -ENOMEM;
         /* else all is good so lets set up the notes. */
         else
         {
+            Elf_Note *note = &notes->first;
+
             /* Set up CORE note. */
             setup_note(note, "CORE", NT_PRSTATUS, sizeof(ELF_Prstatus));
             note = ELFNOTE_NEXT(note);

[PATCH v2 7/8] video/lfb: avoid effectively open-coding xzalloc_array()

[Jan Beulich]

There is a difference in generated code: xzalloc_bytes() forces
SMP_CACHE_BYTES alignment. But if code really cared about such higher
than default alignment, it should request so explicitly rather than
using a type-unsafe interface. And if e.g. cache line sharing was a
concern, the allocator itself should arrange to avoid such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/video/lfb.c
+++ b/xen/drivers/video/lfb.c
@@ -147,8 +147,9 @@ int __init lfb_init(struct lfb_prop *lfb
 {
     lfb.lfbp = *lfbp;
 
-    lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
-    lfb.text_buf = xzalloc_bytes(lfb.lfbp.text_columns * lfb.lfbp.text_rows);
+    lfb.lbuf = xmalloc_array(unsigned char, lfb.lfbp.bytes_per_line);
+    lfb.text_buf = xzalloc_array(unsigned char,
+                                 lfb.lfbp.text_columns * lfb.lfbp.text_rows);
     lfb.line_len = xzalloc_array(unsigned int, lfb.lfbp.text_columns);
 
     if ( !lfb.lbuf || !lfb.text_buf || !lfb.line_len )

[PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct()

[Jan Beulich]

The current use of xzalloc_bytes() in optee is nearly an open-coded
version  of xzalloc_flex_struct(), which was introduced after the driver
was merged.

The main difference is xzalloc_bytes() will also force the allocation to
be SMP_CACHE_BYTES aligned and therefore avoid sharing the cache line.
While sharing the cache line can have an impact on the performance, this
is also true for most of the other users of x*alloc(), x*alloc_array(),
and x*alloc_flex_struct(). So if we want to prevent sharing cache lines,
arranging for this should be done in the allocator itself.

In this case, we don't need stricter alignment than what the allocator 
provides. Hence replace the call to xzalloc_bytes() with one of
xzalloc_flex_struct().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Julien Grall <julien@xen.org>
---
v2: Use commit message very close to what was suggested by Julien.

--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -529,8 +529,7 @@ static struct optee_shm_buf *allocate_op
     while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
                                            old, new)) );
 
-    optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
-                                  pages_cnt * sizeof(struct page *));
+    optee_shm_buf = xzalloc_flex_struct(struct optee_shm_buf, pages, pages_cnt);
     if ( !optee_shm_buf )
     {
         err_code = -ENOMEM;

Re: [PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()

[Andrew Cooper]

On 21/04/2021 15:54, Jan Beulich wrote:
> In the long run I think we want to do away with these type-unsafe
> interfaces, the more that they also request (typically) excess
> alignment. This series of entirely independent patches is
> eliminating the instances where it's relatively clear that they're
> not just "blob" allocations.
>
> v2 only has commit messages extended.
>
> 1: x86/MCE: avoid effectively open-coding xmalloc_array()
> 2: x86/HVM: avoid effectively open-coding xmalloc_array()
> 3: x86/oprofile: avoid effectively open-coding xmalloc_array()
> 4: x86/IRQ: avoid over-alignment in alloc_pirq_struct()
> 5: EFI/runtime: avoid effectively open-coding xmalloc_array()
> 6: kexec: avoid effectively open-coding xzalloc_flex_struct()
> 7: video/lfb: avoid effectively open-coding xzalloc_array()
> 8: Arm/optee: don't open-code xzalloc_flex_struct()

I'm tempted to nack this, but for now will go with a firm -2 to the
whole series.

It is unreasonable, at an API level, for *lloc_bytes(...) to not be
interchangeable *alloc_array(char, ...), and the former is the clearer
way of writing code.

The alignment details are internal properties, dubious at best, and
totally unreasonable for maintainer to know or care about as far as the
API is concerned.  There is also no type safety to be gained by making
the transformation.

If you want to improve the alignment, fix the allocator and the
behind-the-scenes semantics.  Don't make every callsite more complicated
to follow, and definitely don't introduce perf problems from cacheline
sharing in the name of typesafey.

~Andrew

Re: [PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()

[Jan Beulich]

On 21.04.2021 17:23, Andrew Cooper wrote:
> On 21/04/2021 15:54, Jan Beulich wrote:
>> In the long run I think we want to do away with these type-unsafe
>> interfaces, the more that they also request (typically) excess
>> alignment. This series of entirely independent patches is
>> eliminating the instances where it's relatively clear that they're
>> not just "blob" allocations.
>>
>> v2 only has commit messages extended.
>>
>> 1: x86/MCE: avoid effectively open-coding xmalloc_array()
>> 2: x86/HVM: avoid effectively open-coding xmalloc_array()
>> 3: x86/oprofile: avoid effectively open-coding xmalloc_array()
>> 4: x86/IRQ: avoid over-alignment in alloc_pirq_struct()
>> 5: EFI/runtime: avoid effectively open-coding xmalloc_array()
>> 6: kexec: avoid effectively open-coding xzalloc_flex_struct()
>> 7: video/lfb: avoid effectively open-coding xzalloc_array()
>> 8: Arm/optee: don't open-code xzalloc_flex_struct()
> 
> I'm tempted to nack this, but for now will go with a firm -2 to the
> whole series.
> 
> It is unreasonable, at an API level, for *lloc_bytes(...) to not be
> interchangeable *alloc_array(char, ...), and the former is the clearer
> way of writing code.
> 
> The alignment details are internal properties, dubious at best, and
> totally unreasonable for maintainer to know or care about as far as the
> API is concerned.  There is also no type safety to be gained by making
> the transformation.
> 
> If you want to improve the alignment, fix the allocator and the
> behind-the-scenes semantics.  Don't make every callsite more complicated
> to follow, and definitely don't introduce perf problems from cacheline
> sharing in the name of typesafey.

So you firmly think x*alloc_bytes() is a good interface to have and to
keep? As said above, I'm of the clear opinion that we should get rid
of it (with what you say in the first sentence of the second from last
paragraph being one of the reasons). The fact that it may be a
shorthand when allocating char[] is really, really minor (and violates
consistency of the code base as a whole).

Also, just to make this explicit, patch 4 really is somewhat different
from the others, and hence would better not fall into a general "I don't
like this and won't look at it" bucket. Granted I'm not sure you'll
flame me for other reasons there ...

Jan

Re: [PATCH v2 0/8] assorted replacement of x[mz]alloc_bytes()

[Jan Beulich]

On 21.04.2021 17:23, Andrew Cooper wrote:
> On 21/04/2021 15:54, Jan Beulich wrote:
>> In the long run I think we want to do away with these type-unsafe
>> interfaces, the more that they also request (typically) excess
>> alignment. This series of entirely independent patches is
>> eliminating the instances where it's relatively clear that they're
>> not just "blob" allocations.
>>
>> v2 only has commit messages extended.
>>
>> 1: x86/MCE: avoid effectively open-coding xmalloc_array()
>> 2: x86/HVM: avoid effectively open-coding xmalloc_array()
>> 3: x86/oprofile: avoid effectively open-coding xmalloc_array()
>> 4: x86/IRQ: avoid over-alignment in alloc_pirq_struct()
>> 5: EFI/runtime: avoid effectively open-coding xmalloc_array()
>> 6: kexec: avoid effectively open-coding xzalloc_flex_struct()
>> 7: video/lfb: avoid effectively open-coding xzalloc_array()
>> 8: Arm/optee: don't open-code xzalloc_flex_struct()
> 
> I'm tempted to nack this, but for now will go with a firm -2 to the
> whole series.

I wonder whether you really mean the whole series - patch 8 clearly
matches the pattern of two of the v1 patches that you did ack yourself.

Jan

Ping: [PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct()

[Jan Beulich]

Volodymyr,

On 21.04.2021 16:59, Jan Beulich wrote:
> The current use of xzalloc_bytes() in optee is nearly an open-coded
> version  of xzalloc_flex_struct(), which was introduced after the driver
> was merged.
> 
> The main difference is xzalloc_bytes() will also force the allocation to
> be SMP_CACHE_BYTES aligned and therefore avoid sharing the cache line.
> While sharing the cache line can have an impact on the performance, this
> is also true for most of the other users of x*alloc(), x*alloc_array(),
> and x*alloc_flex_struct(). So if we want to prevent sharing cache lines,
> arranging for this should be done in the allocator itself.
> 
> In this case, we don't need stricter alignment than what the allocator 
> provides. Hence replace the call to xzalloc_bytes() with one of
> xzalloc_flex_struct().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Julien Grall <julien@xen.org>
> ---
> v2: Use commit message very close to what was suggested by Julien.

I realize I forgot to Cc you on the v2 submission, but I didn't hear
back on v1 from you either. May I ask for an ack or otherwise?

Thanks, Jan

> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -529,8 +529,7 @@ static struct optee_shm_buf *allocate_op
>      while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
>                                             old, new)) );
>  
> -    optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
> -                                  pages_cnt * sizeof(struct page *));
> +    optee_shm_buf = xzalloc_flex_struct(struct optee_shm_buf, pages, pages_cnt);
>      if ( !optee_shm_buf )
>      {
>          err_code = -ENOMEM;
> 
> 

Re: Ping: [PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct()

[Volodymyr Babchuk]

Hi Jan,

Jan Beulich writes:

> On 21.04.2021 16:59, Jan Beulich wrote:
>> The current use of xzalloc_bytes() in optee is nearly an open-coded
>> version  of xzalloc_flex_struct(), which was introduced after the driver
>> was merged.
>> 
>> The main difference is xzalloc_bytes() will also force the allocation to
>> be SMP_CACHE_BYTES aligned and therefore avoid sharing the cache line.
>> While sharing the cache line can have an impact on the performance, this
>> is also true for most of the other users of x*alloc(), x*alloc_array(),
>> and x*alloc_flex_struct(). So if we want to prevent sharing cache lines,
>> arranging for this should be done in the allocator itself.
>> 
>> In this case, we don't need stricter alignment than what the allocator 
>> provides. Hence replace the call to xzalloc_bytes() with one of
>> xzalloc_flex_struct().
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Julien Grall <julien@xen.org>
>> ---
>> v2: Use commit message very close to what was suggested by Julien.
>
> I realize I forgot to Cc you on the v2 submission, but I didn't hear
> back on v1 from you either. May I ask for an ack or otherwise?

You already had discussion on v1, so I just wanted to see how it will
conclude before stepping in.

I'm fine with this change:

Acked-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -529,8 +529,7 @@ static struct optee_shm_buf *allocate_op
>>      while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
>>                                             old, new)) );
>>  
>> -    optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
>> -                                  pages_cnt * sizeof(struct page *));
>> +    optee_shm_buf = xzalloc_flex_struct(struct optee_shm_buf, pages, pages_cnt);
>>      if ( !optee_shm_buf )
>>      {
>>          err_code = -ENOMEM;
>> 
>> 


-- 
Volodymyr Babchuk at EPAM