| Message ID | 4ea9c4a3-74c0-1722-fa5d-3930be99ef4a@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | assorted replacement of x[mz]alloc_bytes() | expand |
On 08/04/2021 13:21, Jan Beulich wrote: > There is a difference in generated code: xzalloc_bytes() forces > SMP_CACHE_BYTES alignment. I think we not only don't need this here, but > actually don't want it. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> This honestly looks like a backwards step. In particular, ... > > --- 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)); ... this is far more error prone than the code you replaced, seeing as there is now a chance that it can underflow. ~Andrew
On 09.04.2021 14:54, Andrew Cooper wrote: > On 08/04/2021 13:21, Jan Beulich wrote: >> There is a difference in generated code: xzalloc_bytes() forces >> SMP_CACHE_BYTES alignment. I think we not only don't need this here, but >> actually don't want it. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > This honestly looks like a backwards step. In particular, ... > >> >> --- 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)); > > ... this is far more error prone than the code you replaced, seeing as > there is now a chance that it can underflow. You're kidding, aren't you? sizeof_cpu_notes() can't possibly return a value which would allow underflow here. There would be bigger issues than the underflow if any such happened, in particular with any theoretical underflow here simply resulting in the allocation to fail. The original code is type-unsafe and requests more alignment than necessary. Besides objecting to my present way of addressing this, I would find it rather helpful if you could mention some kind of alternative you'd consider acceptable. As said in the cover letter, I think mid-term we ought to do away with xmalloc_bytes(). Hence some alternative would be needed here anyway. Jan
--- 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 = ¬es->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 = ¬es->first; + /* Set up CORE note. */ setup_note(note, "CORE", NT_PRSTATUS, sizeof(ELF_Prstatus)); note = ELFNOTE_NEXT(note);
There is a difference in generated code: xzalloc_bytes() forces SMP_CACHE_BYTES alignment. I think we not only don't need this here, but actually don't want it. Signed-off-by: Jan Beulich <jbeulich@suse.com>