From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C1806469710 for ; Sat, 16 May 2020 02:22:52 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <2bb14550-b2f0-5f0f-1ea4-400d7cbd48bf@tarantool.org> Date: Sat, 16 May 2020 01:22:50 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH small 0/2] Aligned lsregion List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , tarantool-patches@dev.tarantool.org, korablev@tarantool.org, tsafin@tarantool.org, gorcunov@gmail.com Hi! Thanks for the review! On 15/05/2020 14:26, Aleksandr Lyapunov wrote: > Thanks for the patch! see comments below. > > On 5/15/20 2:31 AM, Vladislav Shpilevoy wrote: >> The patchset introduces a new function - lsregion_aligned_alloc(). > > Actually you don't need the patch for fixing #4609. > If you want to align vy_mem tuple by 4 bytes (due to uint32_t bsize), all you need is: > > diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c > index adc3ba4..2de4a28 100644 > --- a/src/box/vy_stmt.c > +++ b/src/box/vy_stmt.c > @@ -228,7 +228,7 @@ vy_stmt_dup_lsregion(struct tuple *stmt, struct lsregion *lsregion, >         if (type == IPROTO_UPSERT) >                 alloc_size++; >   > -       mem_stmt = lsregion_alloc(lsregion, alloc_size, alloc_id); > +       mem_stmt = lsregion_alloc(lsregion, small_align(alloc_size, 4), alloc_id); >         if (mem_stmt == NULL) { >                 diag_set(OutOfMemory, size, "lsregion_alloc", "mem_stmt"); >                 return NULL; > > lsregion will allocate aligned addresses as long as you request only aligned sizes, > up to sizeof(uintptr_t) (see lslab_sizeof()). > This property is not documented (maybe it should be!). It is also similar to mempool > property, see mempool_create description. Yeah, here are 3 problems: - lsregion is used not only for tuples, but for bps tree extents too. We can't rely on their alignment being the same. Sanitizer revealed, that both extents and tuples are not aligned; - sanitizer revealed, that mempool_alloc returns unaligned addresses too. I am going to look into that after lsregion; - slab first position can be aligned not enough. So we can't assume anything about first address in a slab. And therefore about all the other addresses too. > I like one-line change more. > > If you still want _reserve and _aligned_alloc method for further purposes, please > read my by-commit comments. I want lsregion be consistent with other allocators, and provide basic API for aligned allocations. Like region does.