| Message ID | 1566324587-3442-4-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu/arm: Add Renesas IPMMU-VMSA support + Linux's iommu_fwspec | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko > Sent: 20 August 2019 19:10 > To: xen-devel@lists.xenproject.org > Cc: sstabellini@kernel.org; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Oleksandr Tyshchenko > <oleksandr_tyshchenko@epam.com>; julien.grall@arm.com; Jan Beulich <jbeulich@suse.com>; > Volodymyr_Babchuk@epam.com > Subject: [Xen-devel] [PATCH V3 3/8] xen/common: Introduce _xrealloc function > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > This patch introduces type-unsafe function which besides > re-allocation handles the following corner cases: > 1. if requested size is zero, it will behave like xfree > 2. if incoming pointer is not valid (NULL or ZERO_BLOCK_PTR), > it will behave like xmalloc > > If both pointer and size are valid the function will re-allocate and > copy only if requested size doesn't fit in already allocated space. > > Subsequent patch will add type-safe helper macros. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien.grall@arm.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Tim Deegan <tim@xen.org> > CC: Wei Liu <wl@xen.org> > > --- > Changes since RFC: > - behave like xmalloc if incoming pointer is ZERO_BLOCK_PTR or NULL > - return ZERO_BLOCK_PTR after xfree if requested size is zero > - add patch description > - use allocator internals to recognize current size of > the incoming pointer > - do not re-allocate and copy if requested size fits in already > allocated space > > ... > > Original patch was initially posted by Sameer Goel: > https://lists.xen.org/archives/html/xen-devel/2017-06/msg00858.html > > This could be considered as another attempt to add it: > https://www.mail-archive.com/kexec@lists.infradead.org/msg21335.html > > [As it was previously discussed with Julien in IRC] > > The reason for this patch to be an RFC is that patch itself is not > completely correct and I don't fully understand what/how should > be done for this patch to be accepted. Or whether community even > wants this to go in. So, to avoid bike shedding, the first target is > to collect feedback. > > For everyone who wants more details why this is needed and > where used, please see next patch of this thread: > "iommu/arm: Add lightweight iommu_fwspec support" > > In a nutshell, the upcoming "iommu_fwspec" support on ARM > is going to use xrealloc to expand an array for device IDs. > We really want to have "iommu_fwspec" support which will give us > a generic abstract way to add new device to the IOMMU based on > the generic IOMMU DT binding. > --- > xen/common/xmalloc_tlsf.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/xmalloc.h | 1 + > 2 files changed, 46 insertions(+) > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > index e98ad65..eecae2e 100644 > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -598,6 +598,51 @@ void *_xzalloc(unsigned long size, unsigned long align) > return p ? memset(p, 0, size) : p; > } > > +void *_xrealloc(void *ptr, unsigned long size, unsigned long align) > +{ > + unsigned long curr_size, tmp_size; > + void *p; > + > + if ( !size ) > + { > + xfree(ptr); > + return ZERO_BLOCK_PTR; > + } > + > + if ( ptr == NULL || ptr == ZERO_BLOCK_PTR ) > + return _xmalloc(size, align); > + > + if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) > + curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; > + else > + { > + struct bhdr *b; > + b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); > + curr_size = b->size & BLOCK_SIZE_MASK; > + } That seconds clause is not going to give you the block size if the previous allocation had alignment padding. You'll need to check the FREE_BLOCK bit to tell whether it's a real block header or the 'fake' alignment header and then maybe walk backwards onto the real header. See the code in xfree(). You should also check whether the new requested alignment is compatible with the existing block alignment Paul > + > + ASSERT((align & (align - 1)) == 0); > + if ( align < MEM_ALIGN ) > + align = MEM_ALIGN; > + tmp_size = size + align - MEM_ALIGN; > + > + if ( tmp_size < PAGE_SIZE ) > + tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE : > + ROUNDUP_SIZE(tmp_size); > + > + if ( tmp_size <= curr_size ) /* fits in current block */ > + return ptr; > + > + p = _xmalloc(size, align); > + if ( p ) > + { > + memcpy(p, ptr, min(curr_size, size)); > + xfree(ptr); > + } > + > + return p; > +} > + > void xfree(void *p) > { > struct bhdr *b; > diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h > index f075d2d..831152f 100644 > --- a/xen/include/xen/xmalloc.h > +++ b/xen/include/xen/xmalloc.h > @@ -51,6 +51,7 @@ extern void xfree(void *); > /* Underlying functions */ > extern void *_xmalloc(unsigned long size, unsigned long align); > extern void *_xzalloc(unsigned long size, unsigned long align); > +extern void *_xrealloc(void *ptr, unsigned long size, unsigned long align); > > static inline void *_xmalloc_array( > unsigned long size, unsigned long align, unsigned long num) > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 21.08.19 11:09, Paul Durrant wrote: Hi, Paul >> } >> >> +void *_xrealloc(void *ptr, unsigned long size, unsigned long align) >> +{ >> + unsigned long curr_size, tmp_size; >> + void *p; >> + >> + if ( !size ) >> + { >> + xfree(ptr); >> + return ZERO_BLOCK_PTR; >> + } >> + >> + if ( ptr == NULL || ptr == ZERO_BLOCK_PTR ) >> + return _xmalloc(size, align); >> + >> + if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) >> + curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; >> + else >> + { >> + struct bhdr *b; >> + b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); >> + curr_size = b->size & BLOCK_SIZE_MASK; >> + } > That seconds clause is not going to give you the block size if the previous allocation had alignment padding. You'll need to check the FREE_BLOCK bit to tell whether it's a real block header or the 'fake' alignment header and then maybe walk backwards onto the real header. See the code in xfree(). Indeed. Thank you for the pointer. > You should also check whether the new requested alignment is compatible with the existing block alignment I am wondering: In case when we use only type-safe construction for the basic allocation (xmalloc) and type-safe construction for the re-allocation (xrealloc_flex_struct) over the same pointer of the correct type, will we get a possible alignment incompatibility? This is how I see it: diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index eecae2e..f90f453 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -616,8 +616,15 @@ void *_xrealloc(void *ptr, unsigned long size, unsigned long align) curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; else { - struct bhdr *b; - b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); + struct bhdr *b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); + + if ( b->size & FREE_BLOCK ) + { + p = (char *)ptr - (b->size & ~FREE_BLOCK); + b = (struct bhdr *)((char *)p - BHDR_OVERHEAD); + ASSERT(!(b->size & FREE_BLOCK)); + } + curr_size = b->size & BLOCK_SIZE_MASK; } @@ -630,8 +637,8 @@ void *_xrealloc(void *ptr, unsigned long size, unsigned long align) tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE : ROUNDUP_SIZE(tmp_size); - if ( tmp_size <= curr_size ) /* fits in current block */ - return ptr; + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) + return ptr; /* fits in current block */ p = _xmalloc(size, align); if ( p ) (END) Did I get your point correct?
> -----Original Message----- > From: Oleksandr <olekstysh@gmail.com> > Sent: 21 August 2019 15:36 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: sstabellini@kernel.org; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Oleksandr Tyshchenko > <oleksandr_tyshchenko@epam.com>; julien.grall@arm.com; Jan Beulich <jbeulich@suse.com>; > Volodymyr_Babchuk@epam.com > Subject: Re: [Xen-devel] [PATCH V3 3/8] xen/common: Introduce _xrealloc function > > > On 21.08.19 11:09, Paul Durrant wrote: > > Hi, Paul > > >> } > >> > >> +void *_xrealloc(void *ptr, unsigned long size, unsigned long align) > >> +{ > >> + unsigned long curr_size, tmp_size; > >> + void *p; > >> + > >> + if ( !size ) > >> + { > >> + xfree(ptr); > >> + return ZERO_BLOCK_PTR; > >> + } > >> + > >> + if ( ptr == NULL || ptr == ZERO_BLOCK_PTR ) > >> + return _xmalloc(size, align); > >> + > >> + if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) > >> + curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; > >> + else > >> + { > >> + struct bhdr *b; > >> + b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); > >> + curr_size = b->size & BLOCK_SIZE_MASK; > >> + } > > That seconds clause is not going to give you the block size if the previous allocation had alignment > padding. You'll need to check the FREE_BLOCK bit to tell whether it's a real block header or the > 'fake' alignment header and then maybe walk backwards onto the real header. See the code in xfree(). > > Indeed. Thank you for the pointer. > > > > You should also check whether the new requested alignment is compatible with the existing block > alignment > > > I am wondering: > > In case when we use only type-safe construction for the basic allocation > (xmalloc) and type-safe construction for the re-allocation > (xrealloc_flex_struct) over the same pointer of the correct type, will > we get a possible alignment incompatibility? You shouldn't but you're trusting that no-one will want to call the function directly with a higher alignment. > > > This is how I see it: > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > index eecae2e..f90f453 100644 > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -616,8 +616,15 @@ void *_xrealloc(void *ptr, unsigned long size, > unsigned long align) > curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; > else > { > - struct bhdr *b; > - b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); > + struct bhdr *b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); > + > + if ( b->size & FREE_BLOCK ) > + { > + p = (char *)ptr - (b->size & ~FREE_BLOCK); > + b = (struct bhdr *)((char *)p - BHDR_OVERHEAD); > + ASSERT(!(b->size & FREE_BLOCK)); > + } > + > curr_size = b->size & BLOCK_SIZE_MASK; Yes, that looks ok for getting the size right... > } > > @@ -630,8 +637,8 @@ void *_xrealloc(void *ptr, unsigned long size, > unsigned long align) > tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE : > ROUNDUP_SIZE(tmp_size); > > - if ( tmp_size <= curr_size ) /* fits in current block */ > - return ptr; > + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) > + return ptr; /* fits in current block */ ...and that should deal with the alignment too (although you may want to mention the alignment part in the comment too) > > p = _xmalloc(size, align); > if ( p ) > > (END) > > > Did I get your point correct? > Yes, with those changes I think it will look ok. Paul > > -- > Regards, > > Oleksandr Tyshchenko
On 21.08.19 18:47, Paul Durrant wrote: Hi >> >> You should also check whether the new requested alignment is compatible with the existing block >> alignment >> >> >> I am wondering: >> >> In case when we use only type-safe construction for the basic allocation >> (xmalloc) and type-safe construction for the re-allocation >> (xrealloc_flex_struct) over the same pointer of the correct type, will >> we get a possible alignment incompatibility? > You shouldn't but you're trusting that no-one will want to call the function directly with a higher alignment. Yes, it is best to be on the safe side. > >> >> This is how I see it: >> >> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c >> index eecae2e..f90f453 100644 >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -616,8 +616,15 @@ void *_xrealloc(void *ptr, unsigned long size, >> unsigned long align) >> curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; >> else >> { >> - struct bhdr *b; >> - b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); >> + struct bhdr *b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); >> + >> + if ( b->size & FREE_BLOCK ) >> + { >> + p = (char *)ptr - (b->size & ~FREE_BLOCK); >> + b = (struct bhdr *)((char *)p - BHDR_OVERHEAD); >> + ASSERT(!(b->size & FREE_BLOCK)); >> + } >> + >> curr_size = b->size & BLOCK_SIZE_MASK; > Yes, that looks ok for getting the size right... > >> } >> >> @@ -630,8 +637,8 @@ void *_xrealloc(void *ptr, unsigned long size, >> unsigned long align) >> tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE : >> ROUNDUP_SIZE(tmp_size); >> >> - if ( tmp_size <= curr_size ) /* fits in current block */ >> - return ptr; >> + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) >> + return ptr; /* fits in current block */ > ...and that should deal with the alignment too (although you may want to mention the alignment part in the comment too) > >> p = _xmalloc(size, align); >> if ( p ) >> >> (END) >> >> >> Did I get your point correct? >> > Yes, with those changes I think it will look ok. Thank you.
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index e98ad65..eecae2e 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -598,6 +598,51 @@ void *_xzalloc(unsigned long size, unsigned long align) return p ? memset(p, 0, size) : p; } +void *_xrealloc(void *ptr, unsigned long size, unsigned long align) +{ + unsigned long curr_size, tmp_size; + void *p; + + if ( !size ) + { + xfree(ptr); + return ZERO_BLOCK_PTR; + } + + if ( ptr == NULL || ptr == ZERO_BLOCK_PTR ) + return _xmalloc(size, align); + + if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) + curr_size = PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; + else + { + struct bhdr *b; + b = (struct bhdr *)((char *)ptr - BHDR_OVERHEAD); + curr_size = b->size & BLOCK_SIZE_MASK; + } + + ASSERT((align & (align - 1)) == 0); + if ( align < MEM_ALIGN ) + align = MEM_ALIGN; + tmp_size = size + align - MEM_ALIGN; + + if ( tmp_size < PAGE_SIZE ) + tmp_size = ( tmp_size < MIN_BLOCK_SIZE ) ? MIN_BLOCK_SIZE : + ROUNDUP_SIZE(tmp_size); + + if ( tmp_size <= curr_size ) /* fits in current block */ + return ptr; + + p = _xmalloc(size, align); + if ( p ) + { + memcpy(p, ptr, min(curr_size, size)); + xfree(ptr); + } + + return p; +} + void xfree(void *p) { struct bhdr *b; diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h index f075d2d..831152f 100644 --- a/xen/include/xen/xmalloc.h +++ b/xen/include/xen/xmalloc.h @@ -51,6 +51,7 @@ extern void xfree(void *); /* Underlying functions */ extern void *_xmalloc(unsigned long size, unsigned long align); extern void *_xzalloc(unsigned long size, unsigned long align); +extern void *_xrealloc(void *ptr, unsigned long size, unsigned long align); static inline void *_xmalloc_array( unsigned long size, unsigned long align, unsigned long num)