Re: [Tarantool-patches] [PATCH v2 09/10] port: make port_c_entry not PACKED

["Timur Safin" <tsafin@tarantool.org>]

LGTM!

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 09/10] port: make port_c_entry not PACKED
: 
: PACKED structures don't have padding between their members and
: after the structure (needed to be able to store them in an array).
: 
: Port_c_entry was PACKED, since it does not have padding between
: its members anyway, and the padding in the end was not needed,
: because these objects are never stored in an array. As a result,
: sizeof(port_c_entry) was not aligned too.
: 
: Appeared, that mempool, used to allocate port_c_entry objects,
: can't work correctly, when object size is not aligned at least by
: 8 bytes. Because mempool does not do any alignment internally, and
: uses the free objects as a temporary storage for some metadata,
: requiring 8 byte alignment.
: 
: The patch removes PACKED attribute from port_c_entry, so now its
: size is aligned by 8 bytes, and mempool works fine.
: 
: Part of #4609
: ---
:  src/box/lua/schema.lua |  2 +-
:  src/box/port.h         | 10 +---------
:  src/lib/core/port.h    |  2 +-
:  3 files changed, 3 insertions(+), 11 deletions(-)
: 
: diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
: index cdfbb65f7..e6844b45f 100644
: --- a/src/box/lua/schema.lua
: +++ b/src/box/lua/schema.lua
: @@ -79,7 +79,7 @@ ffi.cdef[[
:      box_txn_savepoint_t *
:      box_txn_savepoint();
: 
: -    struct __attribute__((packed)) port_c_entry {
: +    struct port_c_entry {
:          struct port_c_entry *next;
:          union {
:              struct tuple *tuple;
: diff --git a/src/box/port.h b/src/box/port.h
: index 76bb5ed1b..43d0f9deb 100644
: --- a/src/box/port.h
: +++ b/src/box/port.h
: @@ -106,12 +106,7 @@ void
:  port_vdbemem_create(struct port *base, struct sql_value *mem,
:  		    uint32_t mem_count);
: 
: -/**
: - * The struct is PACKED to avoid unnecessary 4 byte padding
: - * after mp_size. These objects are never stored on stack, neither
: - * allocated as an array, so the padding is not needed.
: - */
: -struct PACKED port_c_entry {
: +struct port_c_entry {
:  	struct port_c_entry *next;
:  	union {
:  		/** Valid if mp_size is 0. */
: @@ -126,9 +121,6 @@ struct PACKED port_c_entry {
:  	uint32_t mp_size;
:  };
: 
: -static_assert(sizeof(struct port_c_entry) == 20,
: -	      "port_c_entry is expected to be perfectly aligned and
: packed");
: -
:  /**
:   * C port is used by C functions from the public API. They can
:   * return tuples and arbitrary MessagePack.
: diff --git a/src/lib/core/port.h b/src/lib/core/port.h
: index bb7787679..5c51f76e1 100644
: --- a/src/lib/core/port.h
: +++ b/src/lib/core/port.h
: @@ -122,7 +122,7 @@ struct port {
:  	 * Implementation dependent content. Needed to declare
:  	 * an abstract port instance on stack.
:  	 */
: -	char pad[52];
: +	char pad[60];
:  };
: 
:  /** Is not inlined just to be exported. */
: --
: 2.21.1 (Apple Git-122.3)