| Message ID | 1564763985-20312-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 |
On 02.08.2019 18:39, Oleksandr Tyshchenko wrote: > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -610,6 +610,27 @@ void *_xzalloc(unsigned long size, unsigned long align) > return p ? memset(p, 0, size) : p; > } > > +void *_xrealloc(void *p, unsigned long new_size, unsigned long align) > +{ > + void *new_p; > + > + if ( !new_size ) > + { > + xfree(p); > + return NULL; > + } > + > + new_p = _xmalloc(new_size, align); > + > + if ( new_p && p ) > + { > + memcpy(new_p, p, new_size); > + xfree(p); > + } > + > + return new_p; > +} While I can see why having a re-allocation function may be handy, explicit / direct use of _xmalloc() and _xzalloc() are discouraged, in favor of the more type-safe underscore-less variants. I can't see though how a type-safe "realloc" could look like, except for arrays. If resizing arrays is all you're after, I'd like to recommend to go that route rather then the suggested one here. If resizing arbitrary objects is the goal, then what you suggest may be the only route, but I'd still be not overly happy to see such added. Furthermore you don't even use internals of the allocator: It is common practice to avoid re-allocation if the requested size fits within the already allocated block. That's not the least helpful because in such a case you can't possibly suffer any -ENOMEM condition. And finally - please note _xmalloc()'s and _xfree()'s use / special casing of ZERO_BLOCK_PTR: You absolutely would need to mirror this here. Jan
On 05.08.19 13:02, Jan Beulich wrote: Hi, Jan > While I can see why having a re-allocation function may be handy, > explicit / direct use of _xmalloc() and _xzalloc() are discouraged, > in favor of the more type-safe underscore-less variants. took into account > I can't > see though how a type-safe "realloc" could look like, except for > arrays. If resizing arrays is all you're after, I'd like to > recommend to go that route rather then the suggested one here. If > resizing arbitrary objects is the goal, then what you suggest may > be the only route, but I'd still be not overly happy to see such > added. My main goal is to get "ported" from Linux "iommu_fwspec" support (xrealloc user) in [1]. I tried to retain code as much as possible while porting. So, this patch adds almost the same thing what the ported code expects. But, I would be OK to consider modifying a code in a way to resize an array as well as any other variants if present. > > Furthermore you don't even use internals of the allocator: It is > common practice to avoid re-allocation if the requested size fits > within the already allocated block. That's not the least helpful > because in such a case you can't possibly suffer any -ENOMEM > condition. agree, took into account as well. > > And finally - please note _xmalloc()'s and _xfree()'s use / > special casing of ZERO_BLOCK_PTR: You absolutely would need to > mirror this here. got it, will use for zero-size allocation [1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00257.html Thank you.
Hi Jan, Jan Beulich writes: > On 02.08.2019 18:39, Oleksandr Tyshchenko wrote: >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -610,6 +610,27 @@ void *_xzalloc(unsigned long size, unsigned long align) >> return p ? memset(p, 0, size) : p; >> } >> >> +void *_xrealloc(void *p, unsigned long new_size, unsigned long align) >> +{ >> + void *new_p; >> + >> + if ( !new_size ) >> + { >> + xfree(p); >> + return NULL; >> + } >> + >> + new_p = _xmalloc(new_size, align); >> + >> + if ( new_p && p ) >> + { >> + memcpy(new_p, p, new_size); >> + xfree(p); >> + } >> + >> + return new_p; >> +} > > While I can see why having a re-allocation function may be handy, > explicit / direct use of _xmalloc() and _xzalloc() are discouraged, > in favor of the more type-safe underscore-less variants. I can't > see though how a type-safe "realloc" could look like, except for > arrays. If resizing arrays is all you're after, I'd like to > recommend to go that route rather then the suggested one here. If > resizing arbitrary objects is the goal, then what you suggest may > be the only route, but I'd still be not overly happy to see such > added. I can see 3 uses for realloc: a. re-allocate generic data buffer b. re-allocate array c. re-allocate struct with flexible buffer. option c. is about structures like this: struct arrlen { size_t len; int data[1]; }; This is Oleksandr's case. So for option a. we can use _xreallocate(ptr, size, align) For option b. we can use xrealloc_array(_ptr, _type, _num) And for option c. I propose to implement the following macro: #define realloc_flex_struct(_ptr, _type, _field, _len) \ ((_type *)_xrealloc(_ptr, offsetof(_type, _field[_len]) , __alignof__(_type))) It can be used in the following way: newptr = realloc_flex_struct(ptr, struct arrlen, newsize); As you can see, this approach is type-safe and covers Oleksanrd's case.
On 06.08.2019 20:50, Oleksandr wrote: > On 05.08.19 13:02, Jan Beulich wrote: >> I can't >> see though how a type-safe "realloc" could look like, except for >> arrays. If resizing arrays is all you're after, I'd like to >> recommend to go that route rather then the suggested one here. If >> resizing arbitrary objects is the goal, then what you suggest may >> be the only route, but I'd still be not overly happy to see such >> added. > > My main goal is to get "ported" from Linux "iommu_fwspec" support (xrealloc user) in [1]. > > I tried to retain code as much as possible while porting. So, this patch adds almost the same thing what the ported code expects. > > But, I would be OK to consider modifying a code in a way to resize an array as well as any other variants if present. I've looked at the use in patch 4, and it really isn't an array allocation. Even a basic allocation would use _xmalloc() in this case (you'll find examples in the tree if you want). Nevertheless I'd appreciate if the type-unsafe _xrealloc() didn't remain the only re-allocation construct, as to avoiding people using it just because there's no better alternative. Jan
On 06.08.2019 21:51, Volodymyr Babchuk wrote: > > Hi Jan, > > Jan Beulich writes: > >> On 02.08.2019 18:39, Oleksandr Tyshchenko wrote: >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -610,6 +610,27 @@ void *_xzalloc(unsigned long size, unsigned long align) >>> return p ? memset(p, 0, size) : p; >>> } >>> >>> +void *_xrealloc(void *p, unsigned long new_size, unsigned long align) >>> +{ >>> + void *new_p; >>> + >>> + if ( !new_size ) >>> + { >>> + xfree(p); >>> + return NULL; >>> + } >>> + >>> + new_p = _xmalloc(new_size, align); >>> + >>> + if ( new_p && p ) >>> + { >>> + memcpy(new_p, p, new_size); >>> + xfree(p); >>> + } >>> + >>> + return new_p; >>> +} >> >> While I can see why having a re-allocation function may be handy, >> explicit / direct use of _xmalloc() and _xzalloc() are discouraged, >> in favor of the more type-safe underscore-less variants. I can't >> see though how a type-safe "realloc" could look like, except for >> arrays. If resizing arrays is all you're after, I'd like to >> recommend to go that route rather then the suggested one here. If >> resizing arbitrary objects is the goal, then what you suggest may >> be the only route, but I'd still be not overly happy to see such >> added. > > I can see 3 uses for realloc: > > a. re-allocate generic data buffer > b. re-allocate array > c. re-allocate struct with flexible buffer. > > option c. is about structures like this: > > struct arrlen > { > size_t len; > int data[1]; > }; > > This is Oleksandr's case. > > So for option a. we can use _xreallocate(ptr, size, align) > For option b. we can use xrealloc_array(_ptr, _type, _num) > And for option c. I propose to implement the following macro: > > #define realloc_flex_struct(_ptr, _type, _field, _len) \ > ((_type *)_xrealloc(_ptr, offsetof(_type, _field[_len]) , __alignof__(_type))) > > It can be used in the following way: > > newptr = realloc_flex_struct(ptr, struct arrlen, newsize); > > As you can see, this approach is type-safe and covers Oleksanrd's case. This looks fine to me, but then wants to be accompanied by a similar xmalloc_flex_struct(), which could be used right away to replace a number of open-coded instances of the above. There's one more thing for the re-alloc case though (besides cosmetic aspects): The incoming pointer should also be verified to be of correct type. Jan
Hi, > Nevertheless > I'd appreciate if the type-unsafe _xrealloc() didn't remain the > only re-allocation construct, as to avoiding people using it just > because there's no better alternative. I got your point.
Hi, Jan, Volodymyr. >> c. re-allocate struct with flexible buffer. >> >> option c. is about structures like this: >> >> struct arrlen >> { >> size_t len; >> int data[1]; >> }; >> >> This is Oleksandr's case. >> >> So for option a. we can use _xreallocate(ptr, size, align) >> For option b. we can use xrealloc_array(_ptr, _type, _num) >> And for option c. I propose to implement the following macro: >> >> #define realloc_flex_struct(_ptr, _type, _field, >> _len) \ >> ((_type *)_xrealloc(_ptr, offsetof(_type, _field[_len]) , >> __alignof__(_type))) >> >> It can be used in the following way: >> >> newptr = realloc_flex_struct(ptr, struct arrlen, newsize); >> >> As you can see, this approach is type-safe and covers Oleksanrd's case. > > This looks fine to me, but then wants to be accompanied by a > similar xmalloc_flex_struct(), which could be used right away > to replace a number of open-coded instances of the above. Thank you Volodymyr for the idea. Looks like we can get a type-safe approach which looks suitable for my particular case. So, I need to focus on the proper implementation of non type-safe (_xrealloc) variant in the first place taking into the account Jan's comments. Then I will be back to the suggested type-safe marco (realloc_flex_struct). > > There's one more thing for the re-alloc case though (besides > cosmetic aspects): The incoming pointer should also be verified > to be of correct type. Jan, how this could be technically implemented, or are these any existing examples in Xen? > > Jan
On 07.08.2019 20:36, Oleksandr wrote: >> There's one more thing for the re-alloc case though (besides >> cosmetic aspects): The incoming pointer should also be verified >> to be of correct type. > > Jan, how this could be technically implemented, or are these any existing examples in Xen? See x86's copy_to_guest_offset(), for example. To get the compiler to emit a warning (at least), a (typically otherwise dead) comparison of pointers is commonly used. Jan
(I'm sorry if you receive duplicates of this, but I've got a reply back from our mail system that several of the recipients did not have their host names resolved correctly on the first attempt.) On 07.08.2019 20:36, Oleksandr wrote: >> There's one more thing for the re-alloc case though (besides >> cosmetic aspects): The incoming pointer should also be verified >> to be of correct type. > > Jan, how this could be technically implemented, or are these any existing examples in Xen? See x86's copy_to_guest_offset(), for example. To get the compiler to emit a warning (at least), a (typically otherwise dead) comparison of pointers is commonly used. Jan
On 08.08.19 10:05, Jan Beulich wrote: Hi Jan > (I'm sorry if you receive duplicates of this, but I've got a reply > back from our mail system that several of the recipients did not > have their host names resolved correctly on the first attempt.) Absolutely no problem. >> Jan, how this could be technically implemented, or are these any existing examples in Xen? > See x86's copy_to_guest_offset(), for example. To get the compiler > to emit a warning (at least), a (typically otherwise dead) > comparison of pointers is commonly used. Thank you for the pointer. It is clear now.
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index 2076953..c080763 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -610,6 +610,27 @@ void *_xzalloc(unsigned long size, unsigned long align) return p ? memset(p, 0, size) : p; } +void *_xrealloc(void *p, unsigned long new_size, unsigned long align) +{ + void *new_p; + + if ( !new_size ) + { + xfree(p); + return NULL; + } + + new_p = _xmalloc(new_size, align); + + if ( new_p && p ) + { + memcpy(new_p, p, new_size); + xfree(p); + } + + return new_p; +} + void xfree(void *p) { struct bhdr *b; diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h index b486fe4..63961ef 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 *p, unsigned long new_size, unsigned long align); static inline void *_xmalloc_array( unsigned long size, unsigned long align, unsigned long num)