From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 CA327469710 for ; Sun, 17 May 2020 16:56:35 +0300 (MSK) References: <2bb14550-b2f0-5f0f-1ea4-400d7cbd48bf@tarantool.org> <3f222d25-64d0-17c7-6c75-63cd1711f147@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 17 May 2020 15:56:32 +0200 MIME-Version: 1.0 In-Reply-To: <3f222d25-64d0-17c7-6c75-63cd1711f147@tarantool.org> 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 On 16/05/2020 21:09, Aleksandr Lyapunov wrote: > > On 5/16/20 2:22 AM, Vladislav Shpilevoy wrote: >> >>> 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; > Since you decided to implement _aligned_alloc anyway, it's not very important, but > I want to find the truth. It could help us one day. > Extents' sizes are also (very) aligned, all you need is to align tuple sizes, as I proposed. > Try it and you will see: if you align sizes (up to 8) - you'll get aligned addresses. This is based on an assumption, that extent and tuples need the same alignment. I don't like making such obscure assumptions, because they tend to break, when any tiny detail changes. >> - sanitizer revealed, that mempool_alloc returns unaligned addresses too. I am >>    going to look into that after lsregion; > I checked the code and I confirm that if mempool item size is aligned, the addresses > will be also aligned. Could you suggest me how to get those sanitizer results? > I also see that small_alloc aligns all allocations with 8 bytes. If mempool item size is not aligned, this won't work. I got the unaligned access crash somewhere in box/port.c around port_c_entry structure. I didn't check where removal of 'PACKED' from this structure helps yet. But anyway this looks like a mempool bug. It should not have problems with any object size, regardless of its alignment. Because the crash was inside of mempool, when it tried to write something into a block passed to mempool_free(). So mempool assumes, that object size is aligned at least like mempool internal entries. And if one day we will add uint64 into a mempool entry, but will use the pool to allocate 4 byte aligned objects, this will break too. You can try the sanitizer, if you have clang, with this diff: ==================== diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake index ce3e7e506..373bcd3b0 100644 --- a/cmake/compiler.cmake +++ b/cmake/compiler.cmake @@ -238,6 +238,8 @@ endif() option(ENABLE_WERROR "Make all compiler warnings into errors" OFF) +option(ENABLE_UB_SANITIZER "Make the compiler generate runtime code to perform undefined behaviour checks" OFF) + macro(enable_tnt_compile_flags) # Tarantool code is written in GNU C dialect. # Additionally, compile it with more strict flags than the rest @@ -263,6 +265,14 @@ macro(enable_tnt_compile_flags) "-Wno-strict-aliasing" ) + if (ENABLE_UB_SANITIZER) + if (NOT CMAKE_COMPILER_IS_CLANG) + message(FATAL_ERROR "Undefined behaviour sanitizer only available for clang") + else() + add_compile_flags("C;CXX" "-fsanitize=alignment -fno-sanitize-recover=alignment") + endif() + endif() + if (CMAKE_COMPILER_IS_CLANG AND CC_HAS_WNO_UNUSED_VALUE) # False-positive warnings for ({ xx = ...; x; }) macroses add_compile_flags("C;CXX" "-Wno-unused-value") ==================== >> - 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. > Yes, that position is aligned with 8 bytes now. Are there any cases when we need bigger > align factor? As I said, I don't like making such assumptions. >>> 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. > Ok, that's a great idea, doesn't it earn a special ticket on github? No. I didn't create an issue for that, since I consider it a part of 4609.