Message ID | 1568388917-7287-5-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 |
On 13.09.2019 17:35, Oleksandr Tyshchenko wrote: > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -598,6 +598,58 @@ 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; While the present MAX_ORDER setting will prevent allocations of 4GiB or above from succeeding, may I ask that you don't introduce latent issues in case MAX_ORDER would ever need bumping? > + else > + { > + 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)); > + } This matches the respective xfree() code fragment, and needs to remain in sync. Which suggests introducing a helper function instead of duplicating the code. And please omit the unnecessary casts to char *. > + curr_size = b->size & BLOCK_SIZE_MASK; _xmalloc() has b->size = pad | FREE_BLOCK; i.e. aiui what you calculate above is the padding size, not the overall block size. > + } > + > + 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 : Stray blanks inside parentheses. > + ROUNDUP_SIZE(tmp_size); > + > + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) > + return ptr; /* the size and alignment fit in already allocated space */ You also don't seem to ever update ptr in case you want to use the (head) padding, i.e. you'd hand back a pointer to a block which the caller would assume extends past its actual end. I think you want to calculate the new tentative pointer (taking the requested alignment into account), and only from that calculate curr_size (which perhaps would better be named "usable" or "space" or some such). Obviously the (head) padding block may need updating, too. > + p = _xmalloc(size, align); > + if ( p ) > + { > + memcpy(p, ptr, min(curr_size, size)); > + xfree(ptr); > + } > + > + return p; > +} As a final remark - did you consider zero(?)-filling the tail portion? While C's realloc() isn't specified to do so, since there's no (not going to be a) zeroing counterpart, doing so may be safer overall. Jan
On 16.09.19 13:13, Jan Beulich wrote: Hi, Jan > On 13.09.2019 17:35, Oleksandr Tyshchenko wrote: >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -598,6 +598,58 @@ 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; > While the present MAX_ORDER setting will prevent allocations of > 4GiB or above from succeeding, may I ask that you don't introduce > latent issues in case MAX_ORDER would ever need bumping? Sure (I assume, you are talking about possible truncation): if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; > >> + else >> + { >> + 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)); >> + } > This matches the respective xfree() code fragment, and needs to > remain in sync. Which suggests introducing a helper function > instead of duplicating the code. And please omit the unnecessary > casts to char *. Sounds reasonable, will do. > >> + curr_size = b->size & BLOCK_SIZE_MASK; > _xmalloc() has > > b->size = pad | FREE_BLOCK; > > i.e. aiui what you calculate above is the padding size, not the > overall block size. I have to admit that I am not familiar with allocator internals enough, but I meant to calculate overall block size (the alignment padding is stripped if present)... >> + } >> + >> + 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 : > Stray blanks inside parentheses. ok > >> + ROUNDUP_SIZE(tmp_size); >> + >> + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) >> + return ptr; /* the size and alignment fit in already allocated space */ > You also don't seem to ever update ptr in case you want to use the > (head) padding, i.e. you'd hand back a pointer to a block which the > caller would assume extends past its actual end. I think you want > to calculate the new tentative pointer (taking the requested > alignment into account), and only from that calculate curr_size > (which perhaps would better be named "usable" or "space" or some > such). Obviously the (head) padding block may need updating, too. I am afraid I don't completely understand your point here. And sorry for the maybe naive question, but what is the "(head) padding" here? >> + p = _xmalloc(size, align); >> + if ( p ) >> + { >> + memcpy(p, ptr, min(curr_size, size)); >> + xfree(ptr); >> + } >> + >> + return p; >> +} > As a final remark - did you consider zero(?)-filling the tail > portion? While C's realloc() isn't specified to do so, since there's > no (not going to be a) zeroing counterpart, doing so may be safer > overall. Probably, worth doing. Will zero it.
On 16.09.2019 17:03, Oleksandr wrote: > On 16.09.19 13:13, Jan Beulich wrote: >> On 13.09.2019 17:35, Oleksandr Tyshchenko wrote: >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -598,6 +598,58 @@ 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; >> While the present MAX_ORDER setting will prevent allocations of >> 4GiB or above from succeeding, may I ask that you don't introduce >> latent issues in case MAX_ORDER would ever need bumping? > Sure (I assume, you are talking about possible truncation): > > if ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) > curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; Yes. >>> + ROUNDUP_SIZE(tmp_size); >>> + >>> + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) >>> + return ptr; /* the size and alignment fit in already allocated space */ >> You also don't seem to ever update ptr in case you want to use the >> (head) padding, i.e. you'd hand back a pointer to a block which the >> caller would assume extends past its actual end. I think you want >> to calculate the new tentative pointer (taking the requested >> alignment into account), and only from that calculate curr_size >> (which perhaps would better be named "usable" or "space" or some >> such). Obviously the (head) padding block may need updating, too. > > I am afraid I don't completely understand your point here. And sorry for > the maybe naive question, but what is the "(head) padding" here? The very padding talked about earlier. I did add "(head)" to clarify it's that specific case - after all tail padding is far more common. Jan
On 16.09.19 18:24, Jan Beulich wrote: Hi, Jan. >>>> + ROUNDUP_SIZE(tmp_size); >>>> + >>>> + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) >>>> + return ptr; /* the size and alignment fit in already allocated space */ >>> You also don't seem to ever update ptr in case you want to use the >>> (head) padding, i.e. you'd hand back a pointer to a block which the >>> caller would assume extends past its actual end. I think you want >>> to calculate the new tentative pointer (taking the requested >>> alignment into account), and only from that calculate curr_size >>> (which perhaps would better be named "usable" or "space" or some >>> such). Obviously the (head) padding block may need updating, too. >> I am afraid I don't completely understand your point here. And sorry for >> the maybe naive question, but what is the "(head) padding" here? > The very padding talked about earlier. I did add "(head)" to clarify > it's that specific case - after all tail padding is far more common. Still unsure, I completely understand your point regarding calculating tentative pointer and then curr_size. ---------- Does the diff below is close to what you meant? --- xen/common/xmalloc_tlsf.c | 113 ++++++++++++++++++++++++++++++++++++++-------- xen/include/xen/xmalloc.h | 1 + 2 files changed, 96 insertions(+), 18 deletions(-) diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index e98ad65..f24c97c 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -554,10 +554,40 @@ static void tlsf_init(void) #define ZERO_BLOCK_PTR ((void *)-1L) #endif +static void *strip_padding(void *p) +{ + struct bhdr *b = (struct bhdr *)(p - BHDR_OVERHEAD); + + if ( b->size & FREE_BLOCK ) + { + p -= b->size & ~FREE_BLOCK; + b = (struct bhdr *)(p - BHDR_OVERHEAD); + ASSERT(!(b->size & FREE_BLOCK)); + } + + return p; +} + +static void *add_padding(void *p, unsigned long align) +{ + u32 pad; + + if ( (pad = -(long)p & (align - 1)) != 0 ) + { + char *q = (char *)p + pad; + struct bhdr *b = (struct bhdr *)(q - BHDR_OVERHEAD); + + ASSERT(q > (char *)p); + b->size = pad | FREE_BLOCK; + p = q; + } + + return p; +} + void *_xmalloc(unsigned long size, unsigned long align) { void *p = NULL; - u32 pad; ASSERT(!in_irq()); @@ -578,14 +608,7 @@ void *_xmalloc(unsigned long size, unsigned long align) return xmalloc_whole_pages(size - align + MEM_ALIGN, align); /* Add alignment padding. */ - if ( (pad = -(long)p & (align - 1)) != 0 ) - { - char *q = (char *)p + pad; - struct bhdr *b = (struct bhdr *)(q - BHDR_OVERHEAD); - ASSERT(q > (char *)p); - b->size = pad | FREE_BLOCK; - p = q; - } + p = add_padding(p, align); ASSERT(((unsigned long)p & (align - 1)) == 0); return p; @@ -598,10 +621,70 @@ void *_xzalloc(unsigned long size, unsigned long align) return p ? memset(p, 0, size) : p; } -void xfree(void *p) +void *_xrealloc(void *ptr, unsigned long size, unsigned long align) { - struct bhdr *b; + 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); + + 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 ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) + { + curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; + + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) + return ptr; + } + else + { + struct bhdr *b; + + /* Strip alignment padding. */ + p = strip_padding(ptr); + + b = (struct bhdr *)(p - BHDR_OVERHEAD); + curr_size = b->size & BLOCK_SIZE_MASK; + + if ( tmp_size <= curr_size ) + { + /* Add alignment padding. */ + p = add_padding(p, align); + + ASSERT(((unsigned long)p & (align - 1)) == 0); + + return p; + } + } + + p = _xzalloc(size, align); + if ( p ) + { + memcpy(p, ptr, min(curr_size, size)); + xfree(ptr); + } + + return p; +} + +void xfree(void *p) +{ if ( p == NULL || p == ZERO_BLOCK_PTR ) return; @@ -626,13 +709,7 @@ void xfree(void *p) } /* Strip alignment padding. */ - b = (struct bhdr *)((char *)p - BHDR_OVERHEAD); - if ( b->size & FREE_BLOCK ) - { - p = (char *)p - (b->size & ~FREE_BLOCK); - b = (struct bhdr *)((char *)p - BHDR_OVERHEAD); - ASSERT(!(b->size & FREE_BLOCK)); - } + p = strip_padding(p); xmem_pool_free(p, xenpool); } 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)
On 23.09.2019 14:50, Oleksandr wrote: > Does the diff below is close to what you meant? Almost. > @@ -598,10 +621,70 @@ void *_xzalloc(unsigned long size, unsigned long align) > return p ? memset(p, 0, size) : p; > } > > -void xfree(void *p) > +void *_xrealloc(void *ptr, unsigned long size, unsigned long align) > { > - struct bhdr *b; > + 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); > + > + 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 ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) > + { > + curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; > + > + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) You mean "size" here I think, not "tmp_size". See how xmalloc_whole_pages() gets called from _xmalloc() with an "adjusted back" value. And as said, please clean up the code you move or add anew: Use casts only where really needed, transform types to appropriate "modern" ones, etc. Jan
On 23.09.19 16:31, Jan Beulich wrote: Hi, Jan > >> + >> + if ( ptr == NULL || ptr == ZERO_BLOCK_PTR ) >> + return _xmalloc(size, align); >> + >> + 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 ( !((unsigned long)ptr & (PAGE_SIZE - 1)) ) >> + { >> + curr_size = (unsigned long)PFN_ORDER(virt_to_page(ptr)) << PAGE_SHIFT; >> + >> + if ( tmp_size <= curr_size && ((unsigned long)ptr & (align - 1)) == 0 ) > You mean "size" here I think, not "tmp_size". See how xmalloc_whole_pages() > gets called from _xmalloc() with an "adjusted back" value. Yes, thank you for pointing this. > And as said, please clean up the code you move or add anew: Use casts > only where really needed, transform types to appropriate "modern" ones, > etc. ok, will double check.
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index e98ad65..2b240b1 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -598,6 +598,58 @@ 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 = (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; + } + + 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 && ((unsigned long)ptr & (align - 1)) == 0 ) + return ptr; /* the size and alignment fit in already allocated space */ + + 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)