On Mon, Jul 14, 2025 at 10:14 AM Vlastimil Babka <vba...@suse.cz> wrote: > > On 7/12/25 14:43, Vitaly Wool wrote: > > > > > >> On Jul 11, 2025, at 5:43 PM, Vlastimil Babka <vba...@suse.cz> wrote: > >> > >> On 7/11/25 10:58, Harry Yoo wrote: > >>> On Wed, Jul 09, 2025 at 07:24:41PM +0200, Vitaly Wool wrote: > >>>> static __always_inline __realloc_size(2) void * > >>>> -__do_krealloc(const void *p, size_t new_size, gfp_t flags) > >>>> +__do_krealloc(const void *p, size_t new_size, unsigned long align, > >>>> gfp_t flags, int nid) > >>>> { > >>>> void *ret; > >>>> size_t ks = 0; > >>>> @@ -4859,6 +4859,20 @@ __do_krealloc(const void *p, size_t new_size, > >>>> gfp_t flags) > >>>> if (!kasan_check_byte(p)) > >>>> return NULL; > >>>> > >>>> + /* refuse to proceed if alignment is bigger than what kmalloc() > >>>> provides */ > >>>> + if (!IS_ALIGNED((unsigned long)p, align) || new_size < align) > >>>> + return NULL; > >>> > >>> Hmm but what happens if `p` is aligned to `align`, but the new object is > >>> not? > >>> > >>> For example, what will happen if we allocate object with size=64, > >>> align=64 > >>> and then do krealloc with size=96, align=64... > >>> > >>> Or am I missing something? > >> > >> Good point. We extended the alignment guarantees in commit ad59baa31695 > >> ("slab, rust: extend kmalloc() alignment guarantees to remove Rust > >> padding") > >> for rust in a way that size 96 gives you alignment of 32. It assumes that > >> rust side will ask for alignments that are power-of-two and sizes that are > >> multiples of alignment. I think if that assumption is still honored than > >> this will keep working, but the check added above (is it just a sanity > >> check > >> or something the rust side relies on?) doesn't seem correct? > >> > > > > It is a sanity check and it should have looked like this: > > > > if (!IS_ALIGNED((unsigned long)p, align) && new_size <= ks) > > return NULL; > > > > and the reasoning for this is the following: if we don’t intend to > > reallocate (new size is not bigger than the original size), but the user > > requests a larger alignment, it’s a miss. Does that sound reasonable? > > So taking a step back indeed the align passed to krealloc is indeed used > only for this check. If it's really just a sanity check, then I'd rather not > add this parameter to krealloc functions at all - kmalloc() itself also > doesn't have it, so it's inconsistent that krealloc() would have it - but > only to return NULL and not e.g. try to reallocate for alignment. > > If it's not just a sanity check, it means it's expected that for some > sequence of valid kvrealloc_node_align() calls it can return NULL and then > rely on the fallback to vmalloc. That would be rather wasteful for the cases > like going from 64 to 96 bytes etc. So in that case it would be better if > krealloc did the reallocation, same as in cases when size increases. Of > course it would still have to rely on the documented alignment guarantees > only and not provide anything arbitrary. aligned_size() in > rust/kernel/alloc/allocator.rs is responsible for that, AFAIK. > > And I think it's not a sanity check but the latter - if the following is a > valid k(v)realloc sequence (from Rust POV). The individual size+align > combinations AFAIK are, but if it's valid to make them follow one another > them like this, I don't know. > > krealloc(size=96, align=32) -> can give object with 32 alignment only > krealloc(size=64, align=64) -> doesn't increase size but wants alignment 64
On the Rust side, you specify what the old size/align was, and what you which size/align you want after the call, and they can be anything including that combination. Alice