[tarantool-patches] [PATCH 0/4] swim suspicion preparation

[tarantool-patches] [PATCH 0/4] swim suspicion preparation

[Vladislav Shpilevoy]

This is a set of preparatory patches for SWIM suspicion mechanism - the last
SWIM's native component.

First two patches are pure refactoring to make the code more reusable.

Next third patch fixes a bug about invalidation of a round message when it is
scheduled for sending but is not actually sent. It led to sending empty packets
when the kernel slowly generates output events.

The fourth commit fixes a bug made round packet cache working not in full power.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-suspicion-preparation
Issue: https://github.com/tarantool/tarantool/issues/3234

Vladislav Shpilevoy (4):
  swim: move sockaddr_in checkers to swim_proto.h
  swim: extract binary ip/port into a separate struct
  swim: fix a bug with invalidation of round msg in fly
  swim: do not rebuild packet meta multiple times

 src/lib/swim/swim.c       | 32 +++++++++-------
 src/lib/swim/swim_io.c    | 29 +++++++++++++--
 src/lib/swim/swim_proto.c | 77 +++++++++++++++++++++++++++------------
 src/lib/swim/swim_proto.h | 70 ++++++++++++++++++++++++-----------
 4 files changed, 146 insertions(+), 62 deletions(-)

-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Vladislav Shpilevoy]

There are several places where it is necessary to check if a
sockaddr_in is nullified, and to compare a couple of addresses.
Some of them are in swim_proto.c, and more are coming in indirect
SWIM messages patch. The patch moves the checkers into
swim_proto.h so as to be usable from anywhere in SWIM.

Also minor renames are made alongside. 'sockaddr_in' is too long
to use in each related function's name, and is replaced with
'inaddr' by analogue with the standard library.

Part of #3234
---
 src/lib/swim/swim.c       | 12 ++----------
 src/lib/swim/swim_proto.c | 26 ++++++++++++++++----------
 src/lib/swim/swim_proto.h | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 2dac6eedd..40fa3fb21 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -201,14 +201,6 @@ swim_uuid_str(const struct tt_uuid *uuid)
 	return buf;
 }
 
-/** Check if two AF_INET addresses are equal. */
-static bool
-swim_sockaddr_in_eq(const struct sockaddr_in *a1, const struct sockaddr_in *a2)
-{
-	return a1->sin_port == a2->sin_port &&
-	       a1->sin_addr.s_addr == a2->sin_addr.s_addr;
-}
-
 /**
  * A cluster member description. This structure describes the
  * last known state of an instance. This state is updated
@@ -950,7 +942,7 @@ swim_complete_step(struct swim_task *task,
 	struct swim_member *m =
 		rlist_first_entry(&swim->round_queue, struct swim_member,
 				  in_round_queue);
-	if (swim_sockaddr_in_eq(&m->addr, &task->dst)) {
+	if (swim_inaddr_eq(&m->addr, &task->dst)) {
 		rlist_shift(&swim->round_queue);
 		if (rc > 0) {
 			/*
@@ -1048,7 +1040,7 @@ static inline void
 swim_update_member_addr(struct swim *swim, struct swim_member *member,
 			const struct sockaddr_in *addr, int incarnation_inc)
 {
-	if (! swim_sockaddr_in_eq(addr, &member->addr)) {
+	if (! swim_inaddr_eq(addr, &member->addr)) {
 		member->incarnation += incarnation_inc;
 		member->addr = *addr;
 		swim_on_member_update(swim, member);
diff --git a/src/lib/swim/swim_proto.c b/src/lib/swim/swim_proto.c
index d84550663..cd9dd195b 100644
--- a/src/lib/swim/swim_proto.c
+++ b/src/lib/swim/swim_proto.c
@@ -154,6 +154,20 @@ swim_decode_uuid(struct tt_uuid *uuid, const char **pos, const char *end,
 	return 0;
 }
 
+/**
+ * Check if @a addr is not empty, i.e. not nullified. Set an error
+ * in the diagnostics area in case of emptiness.
+ */
+static inline int
+swim_check_inaddr_not_empty(const struct sockaddr_in *addr, const char *prefix,
+			    const char *addr_name)
+{
+	if (! swim_inaddr_is_empty(addr))
+		return 0;
+	diag_set(SwimError, "%s %s address is mandatory", prefix, addr_name);
+	return -1;
+}
+
 void
 swim_member_def_create(struct swim_member_def *def)
 {
@@ -236,15 +250,11 @@ swim_member_def_decode(struct swim_member_def *def, const char **pos,
 		if (swim_decode_member_key(key, pos, end, prefix, def) != 0)
 			return -1;
 	}
-	if (def->addr.sin_port == 0 || def->addr.sin_addr.s_addr == 0) {
-		diag_set(SwimError, "%s member address is mandatory", prefix);
-		return -1;
-	}
 	if (tt_uuid_is_nil(&def->uuid)) {
 		diag_set(SwimError, "%s member uuid is mandatory", prefix);
 		return -1;
 	}
-	return 0;
+	return swim_check_inaddr_not_empty(&def->addr, prefix, "member");
 }
 
 void
@@ -429,11 +439,7 @@ swim_meta_def_decode(struct swim_meta_def *def, const char **pos,
 		diag_set(SwimError, "%s version is mandatory", prefix);
 		return -1;
 	}
-	if (def->src.sin_port == 0 || def->src.sin_addr.s_addr == 0) {
-		diag_set(SwimError, "%s source address is mandatory", prefix);
-		return -1;
-	}
-	return 0;
+	return swim_check_inaddr_not_empty(&def->src, prefix, "source");
 }
 
 void
diff --git a/src/lib/swim/swim_proto.h b/src/lib/swim/swim_proto.h
index ab4057185..c6ff4539d 100644
--- a/src/lib/swim/swim_proto.h
+++ b/src/lib/swim/swim_proto.h
@@ -472,4 +472,19 @@ int
 swim_decode_uuid(struct tt_uuid *uuid, const char **pos, const char *end,
 		 const char *prefix, const char *param_name);
 
+/** Check if @a addr is not empty, i.e. not nullified. */
+static inline bool
+swim_inaddr_is_empty(const struct sockaddr_in *addr)
+{
+	return addr->sin_port == 0 && addr->sin_addr.s_addr == 0;
+}
+
+/** Check if two AF_INET addresses are equal. */
+static inline bool
+swim_inaddr_eq(const struct sockaddr_in *a1, const struct sockaddr_in *a2)
+{
+	return a1->sin_port == a2->sin_port &&
+	       a1->sin_addr.s_addr == a2->sin_addr.s_addr;
+}
+
 #endif /* TARANTOOL_SWIM_PROTO_H_INCLUDED */
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH 2/4] swim: extract binary ip/port into a separate struct

[Vladislav Shpilevoy]

At this moment there are two binary structures in the SWIM
protocol carrying an address: swim_member_passport and
swim_meta_header_bin - one address in each. This code duplication
was not formidable enough to stimulate creation of a separate
address structure.

But forthcoming indirect messages protocol extensions will add 2
new cases of encoding a binary address. It triggered this patch
to reduce code duplication.

Part of #3234
---
 src/lib/swim/swim_proto.c | 51 ++++++++++++++++++++++++++----------
 src/lib/swim/swim_proto.h | 55 +++++++++++++++++++++++----------------
 2 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/src/lib/swim/swim_proto.c b/src/lib/swim/swim_proto.c
index cd9dd195b..ce52faf1b 100644
--- a/src/lib/swim/swim_proto.c
+++ b/src/lib/swim/swim_proto.c
@@ -168,6 +168,31 @@ swim_check_inaddr_not_empty(const struct sockaddr_in *addr, const char *prefix,
 	return -1;
 }
 
+/**
+ * Create a binary address structure. It requires explicit IP and
+ * port keys specification since the keys depend on the component
+ * employing the address.
+ */
+static inline void
+swim_inaddr_bin_create(struct swim_inaddr_bin *bin, uint8_t ip_key,
+		       uint8_t port_key)
+{
+	assert(mp_sizeof_uint(ip_key) == 1 && mp_sizeof_uint(port_key) == 1);
+	bin->k_addr = ip_key;
+	bin->m_addr = 0xce;
+	bin->k_port = port_key;
+	bin->m_port = 0xcd;
+}
+
+/** Fill already created @a bin with a real inet address. */
+static inline void
+swim_inaddr_bin_fill(struct swim_inaddr_bin *bin,
+		     const struct sockaddr_in *addr)
+{
+	bin->v_addr = mp_bswap_u32(ntohl(addr->sin_addr.s_addr));
+	bin->v_port = mp_bswap_u16(ntohs(addr->sin_port));
+}
+
 void
 swim_member_def_create(struct swim_member_def *def)
 {
@@ -343,12 +368,12 @@ swim_anti_entropy_header_bin_create(struct swim_anti_entropy_header_bin *header,
 void
 swim_passport_bin_create(struct swim_passport_bin *passport)
 {
-	passport->m_header = 0x85;
+	int map_size = 3 + SWIM_INADDR_BIN_SIZE;
+	assert(mp_sizeof_map(map_size) == 1);
+	passport->m_header = 0x80 | map_size;
 	passport->k_status = SWIM_MEMBER_STATUS;
-	passport->k_addr = SWIM_MEMBER_ADDRESS;
-	passport->m_addr = 0xce;
-	passport->k_port = SWIM_MEMBER_PORT;
-	passport->m_port = 0xcd;
+	swim_inaddr_bin_create(&passport->addr, SWIM_MEMBER_ADDRESS,
+			       SWIM_MEMBER_PORT);
 	passport->k_uuid = SWIM_MEMBER_UUID;
 	passport->m_uuid = 0xc4;
 	passport->m_uuid_len = UUID_LEN;
@@ -363,8 +388,7 @@ swim_passport_bin_fill(struct swim_passport_bin *passport,
 		       enum swim_member_status status, uint64_t incarnation)
 {
 	passport->v_status = status;
-	passport->v_addr = mp_bswap_u32(ntohl(addr->sin_addr.s_addr));
-	passport->v_port = mp_bswap_u16(ntohs(addr->sin_port));
+	swim_inaddr_bin_fill(&passport->addr, addr);
 	memcpy(passport->v_uuid, uuid, UUID_LEN);
 	passport->v_incarnation = mp_bswap_u64(incarnation);
 }
@@ -382,16 +406,15 @@ void
 swim_meta_header_bin_create(struct swim_meta_header_bin *header,
 			    const struct sockaddr_in *src)
 {
-	header->m_header = 0x83;
+	int map_size = 1 + SWIM_INADDR_BIN_SIZE;
+	assert(mp_sizeof_map(map_size) == 1);
+	header->m_header = 0x80 | map_size;
 	header->k_version = SWIM_META_TARANTOOL_VERSION;
 	header->m_version = 0xce;
 	header->v_version = mp_bswap_u32(tarantool_version_id());
-	header->k_addr = SWIM_META_SRC_ADDRESS;
-	header->m_addr = 0xce;
-	header->v_addr = mp_bswap_u32(ntohl(src->sin_addr.s_addr));
-	header->k_port = SWIM_META_SRC_PORT;
-	header->m_port = 0xcd;
-	header->v_port = mp_bswap_u16(ntohs(src->sin_port));
+	swim_inaddr_bin_create(&header->src_addr, SWIM_META_SRC_ADDRESS,
+			       SWIM_META_SRC_PORT);
+	swim_inaddr_bin_fill(&header->src_addr, src);
 }
 
 int
diff --git a/src/lib/swim/swim_proto.h b/src/lib/swim/swim_proto.h
index c6ff4539d..db366f729 100644
--- a/src/lib/swim/swim_proto.h
+++ b/src/lib/swim/swim_proto.h
@@ -232,6 +232,35 @@ swim_failure_detection_def_decode(struct swim_failure_detection_def *def,
 
 /** {{{                  Anti-entropy component                 */
 
+enum {
+	/**
+	 * Number of keys in the address binary structure.
+	 * Structures storing an address should use this size so
+	 * as to correctly encode MessagePack map header.
+	 */
+	SWIM_INADDR_BIN_SIZE = 2,
+};
+
+/**
+ * Binary inet address structure. It contains two fields at this
+ * moment - IP and port encoded as usual numbers. It means that
+ * after mp_decode_uint() it is necessary to use htonl/htons()
+ * functions to assign the values to struct sockaddr_in.
+ */
+struct PACKED swim_inaddr_bin {
+	/** mp_encode_uint(address key) */
+	uint8_t k_addr;
+	/** mp_encode_uint(ntohl(addr.sin_addr.s_addr)) */
+	uint8_t m_addr;
+	uint32_t v_addr;
+
+	/** mp_encode_uint(port key) */
+	uint8_t k_port;
+	/** mp_encode_uint(ntohs(addr.sin_port)) */
+	uint8_t m_port;
+	uint16_t v_port;
+};
+
 /**
  * Attributes of each record of a broadcasted member table. Just
  * the same as some of struct swim_member attributes.
@@ -279,17 +308,8 @@ struct PACKED swim_passport_bin {
 	/** mp_encode_uint(enum member_status) */
 	uint8_t v_status;
 
-	/** mp_encode_uint(SWIM_MEMBER_ADDRESS) */
-	uint8_t k_addr;
-	/** mp_encode_uint(addr.sin_addr.s_addr) */
-	uint8_t m_addr;
-	uint32_t v_addr;
-
-	/** mp_encode_uint(SWIM_MEMBER_PORT) */
-	uint8_t k_port;
-	/** mp_encode_uint(addr.sin_port) */
-	uint8_t m_port;
-	uint16_t v_port;
+	/** SWIM_MEMBER_ADDRESS and SWIM_MEMBER_PORT. */
+	struct swim_inaddr_bin addr;
 
 	/** mp_encode_uint(SWIM_MEMBER_UUID) */
 	uint8_t k_uuid;
@@ -386,17 +406,8 @@ struct PACKED swim_meta_header_bin {
 	uint8_t m_version;
 	uint32_t v_version;
 
-	/** mp_encode_uint(SWIM_META_SRC_ADDRESS) */
-	uint8_t k_addr;
-	/** mp_encode_uint(addr.sin_addr.s_addr) */
-	uint8_t m_addr;
-	uint32_t v_addr;
-
-	/** mp_encode_uint(SWIM_META_SRC_PORT) */
-	uint8_t k_port;
-	/** mp_encode_uint(addr.sin_port) */
-	uint8_t m_port;
-	uint16_t v_port;
+	/** SWIM_META_SRC_ADDRESS and SWIM_META_SRC_PORT. */
+	struct swim_inaddr_bin src_addr;
 };
 
 /** Initialize meta section. */
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH 3/4] swim: fix a bug with invalidation of round msg in fly

[Vladislav Shpilevoy]

SWIM works in rounds, each split in steps. On step SWIM sends a
round message. The message is not changed between steps of one
round usually, and is cached so as not to rebuild it without
necessity. But when something changes in one member's attributes,
or in the member table, the message is invalidated to be rebuilt
on a next step. Invalidation resets the cached packet.

But it leads to a bug, when a round message is already scheduled
to be sent, however is not actually sent and invalidated in fly.
Such a message on a next EV_WRITE event will be sent as an empty
packet, which obviously makes no sense.

On the other hand it would be harmful to cancel the invalidated
packet if it is in fly, because during frequent changes the
instance will not send anything.

There are no a test case, because empty packets does not break
anything, but still they are useless. And, as it is said above,
such invalidation would prevent sending any round messages when
there are lots of updates.

Follow up for cf0ddeb889ad798c38542a6f0883bc53ea4b3388
(swim: keep encoded round message cached)
---
 src/lib/swim/swim.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 40fa3fb21..902b7111f 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -366,6 +366,11 @@ struct swim {
 	 * be triggered by any update of any member.
 	 */
 	struct swim_task round_step_task;
+	/**
+	 * True if a packet in the round step task is still valid
+	 * and can be resent on a next round step.
+	 */
+	bool is_round_packet_valid;
 	/**
 	 * Preallocated buffer to store shuffled members here at
 	 * the beginning of each round.
@@ -416,11 +421,18 @@ struct swim {
 	struct rlist dissemination_queue;
 };
 
-/** Reset cached round message on any change of any member. */
+/**
+ * Mark cached round message invalid on any change of any member.
+ * It triggers postponed rebuilding of the message. The round
+ * packet can not be rebuilt right now because 1) invalidation can
+ * occur several times in row when multiple member attributes are
+ * updated, or more than one member are added, 2) the message can
+ * be in fly right now in the output queue inside the scheduler.
+ */
 static inline void
 swim_cached_round_msg_invalidate(struct swim *swim)
 {
-	swim_packet_create(&swim->round_step_task.packet);
+	swim->is_round_packet_valid = false;
 }
 
 /** Put the member into a list of ACK waiters. */
@@ -847,7 +859,7 @@ swim_encode_dissemination(struct swim *swim, struct swim_packet *packet)
 static void
 swim_encode_round_msg(struct swim *swim)
 {
-	if (swim_packet_body_size(&swim->round_step_task.packet) > 0)
+	if (swim->is_round_packet_valid)
 		return;
 	struct swim_packet *packet = &swim->round_step_task.packet;
 	swim_packet_create(packet);
@@ -861,6 +873,7 @@ swim_encode_round_msg(struct swim *swim)
 
 	assert(mp_sizeof_map(map_size) == 1 && map_size >= 2);
 	mp_encode_map(header, map_size);
+	swim->is_round_packet_valid = true;
 }
 
 /**
@@ -964,6 +977,7 @@ swim_send_fd_msg(struct swim *swim, struct swim_task *task,
 	/*
 	 * Reset packet allocator in case if task is being reused.
 	 */
+	assert(! swim_task_is_scheduled(task));
 	swim_packet_create(&task->packet);
 	char *header = swim_packet_alloc(&task->packet, 1);
 	int map_size = swim_encode_src_uuid(swim, &task->packet);
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH 4/4] swim: do not rebuild packet meta multiple times

[Vladislav Shpilevoy]

Before the patch there were 2 cases when an unchanged packet was
rebuilt partially on each send:

  - cached round message's meta section was rebuilt on each
    EV_WRITE event in swim_scheduler_on_output() function;

  - broadcast message's meta section was rebuilt too even though
    its content does not depend on a broadcast interface.

The third case appears with indirect pings patch which aggravates
meta building business by routing and packet forwarding. When a
packet needs to be forwarded farther, its meta is built in a
special manner preserving the route before EV_WRITE appears, and
on_output should not touch that meta.

This patch adds a check preventing unnecessary meta rebuilds.
Besides, the check and the meta building code are moved into a
dedicated function out of swim_scheduler_on_output() - it allows
to completely split logic of packing a message and sending it.
Separated logic helps a lot when indirect pings are introduced.

Part of #3234
---
 src/lib/swim/swim_io.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/lib/swim/swim_io.c b/src/lib/swim/swim_io.c
index 03d79ae3e..7d6addf02 100644
--- a/src/lib/swim/swim_io.c
+++ b/src/lib/swim/swim_io.c
@@ -57,6 +57,31 @@ swim_packet_create(struct swim_packet *packet)
 	swim_packet_alloc_meta(packet, sizeof(struct swim_meta_header_bin));
 }
 
+/** Fill metadata prefix of a packet. */
+static inline void
+swim_packet_build_meta(struct swim_packet *packet,
+		       const struct sockaddr_in *src)
+{
+	char *meta = packet->meta;
+	char *end = packet->body;
+	/*
+	 * Meta has already been built. It happens when the same
+	 * task is resent multiple times.
+	 */
+	if (meta == end)
+		return;
+	struct swim_meta_header_bin header;
+	swim_meta_header_bin_create(&header, src);
+	assert(meta + sizeof(header) == end);
+	memcpy(meta, &header, sizeof(header));
+	/*
+	 * Once meta is built, it is consumed by the body. Used
+	 * not to rebuild the meta again if the task will be
+	 * scheduled again without changes in data.
+	 */
+	packet->body = packet->meta;
+}
+
 void
 swim_task_create(struct swim_task *task, swim_task_f complete,
 		 swim_task_f cancel, const char *desc)
@@ -298,9 +323,7 @@ swim_scheduler_on_output(struct ev_loop *loop, struct ev_io *io, int events)
 	say_verbose("SWIM %d: send %s to %s", swim_scheduler_fd(scheduler),
 		    task->desc, sio_strfaddr((struct sockaddr *) &task->dst,
 					     sizeof(task->dst)));
-	struct swim_meta_header_bin header;
-	swim_meta_header_bin_create(&header, &scheduler->transport.addr);
-	memcpy(task->packet.meta, &header, sizeof(header));
+	swim_packet_build_meta(&task->packet, &scheduler->transport.addr);
 	int rc = swim_transport_send(&scheduler->transport, task->packet.buf,
 				     task->packet.pos - task->packet.buf,
 				     (const struct sockaddr *) &task->dst,
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] Re: [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Vladislav Shpilevoy]

Sorry, accidentally broke a test. The fix is
force pushed:

======================================================
swim_inaddr_is_empty(const struct sockaddr_in *addr)
 {
-	return addr->sin_port == 0 && addr->sin_addr.s_addr == 0;
+	return addr->sin_port == 0 || addr->sin_addr.s_addr == 0;
 }
======================================================

[tarantool-patches] Re: [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:
> There are several places where it is necessary to check if a
> sockaddr_in is nullified, and to compare a couple of addresses.
> Some of them are in swim_proto.c, and more are coming in indirect
> SWIM messages patch. The patch moves the checkers into
> swim_proto.h so as to be usable from anywhere in SWIM.
> 
> Also minor renames are made alongside. 'sockaddr_in' is too long
> to use in each related function's name, and is replaced with
> 'inaddr' by analogue with the standard library.
> 
> Part of #3234

OK to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 17:11]:
> Sorry, accidentally broke a test. The fix is
> force pushed:
> 
> ======================================================
> swim_inaddr_is_empty(const struct sockaddr_in *addr)
>  {
> -	return addr->sin_port == 0 && addr->sin_addr.s_addr == 0;
> +	return addr->sin_port == 0 || addr->sin_addr.s_addr == 0;
>  }

This means you haven't paused to explain or test or add a
pre-condition check to the setter function.
Why either of the components can be empty, not both? Shouldn't 
you check for such broken addresses when setting them, to not
allow setting them at all?

The patch is OK to push obviously, just food for thought.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 2/4] swim: extract binary ip/port into a separate struct

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:
> At this moment there are two binary structures in the SWIM
> protocol carrying an address: swim_member_passport and
> swim_meta_header_bin - one address in each. This code duplication
> was not formidable enough to stimulate creation of a separate
> address structure.
> 
> But forthcoming indirect messages protocol extensions will add 2
> new cases of encoding a binary address. It triggered this patch
> to reduce code duplication.
> 

OK to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 3/4] swim: fix a bug with invalidation of round msg in fly

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:

Why have a concept of invalidation at all? Why can't you simply
always keep the message up to date (built)? Don't you think member
updates are so rare that you're winning very little by building on
demand but the code is more complex because of it?

The patch is OK to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Vladislav Shpilevoy]

On 18/04/2019 18:16, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 17:11]:
>> Sorry, accidentally broke a test. The fix is
>> force pushed:
>>
>> ======================================================
>> swim_inaddr_is_empty(const struct sockaddr_in *addr)
>>  {
>> -	return addr->sin_port == 0 && addr->sin_addr.s_addr == 0;
>> +	return addr->sin_port == 0 || addr->sin_addr.s_addr == 0;
>>  }
> 
> This means you haven't paused to explain or test or add a
> pre-condition check to the setter function.
> Why either of the components can be empty, not both? Shouldn't 
> you check for such broken addresses when setting them, to not
> allow setting them at all?
> 
> The patch is OK to push obviously, just food for thought.

It's mainly against malicious and bad-formatter packets. Since
the protocol will be public, it can happen that custom drivers
will send bad packets.

> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

[tarantool-patches] Re: [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 18:27]:
> It's mainly against malicious and bad-formatter packets. Since
> the protocol will be public, it can happen that custom drivers
> will send bad packets.

This definitely needs a comment. Besides, it's a bad address then,
not empty address, and such messages should be simply rejected.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 4/4] swim: do not rebuild packet meta multiple times

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:
> Before the patch there were 2 cases when an unchanged packet was
> rebuilt partially on each send:
> 
>   - cached round message's meta section was rebuilt on each
>     EV_WRITE event in swim_scheduler_on_output() function;
> 
>   - broadcast message's meta section was rebuilt too even though
>     its content does not depend on a broadcast interface.
> 
> The third case appears with indirect pings patch which aggravates
> meta building business by routing and packet forwarding. When a
> packet needs to be forwarded farther, its meta is built in a
> special manner preserving the route before EV_WRITE appears, and
> on_output should not touch that meta.
> 
> This patch adds a check preventing unnecessary meta rebuilds.
> Besides, the check and the meta building code are moved into a
> dedicated function out of swim_scheduler_on_output() - it allows
> to completely split logic of packing a message and sending it.
> Separated logic helps a lot when indirect pings are introduced.
> 

ok to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 4/4] swim: do not rebuild packet meta multiple times

[Vladislav Shpilevoy]

Pushed into the master.

On 18/04/2019 20:23, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:
>> Before the patch there were 2 cases when an unchanged packet was
>> rebuilt partially on each send:
>>
>>   - cached round message's meta section was rebuilt on each
>>     EV_WRITE event in swim_scheduler_on_output() function;
>>
>>   - broadcast message's meta section was rebuilt too even though
>>     its content does not depend on a broadcast interface.
>>
>> The third case appears with indirect pings patch which aggravates
>> meta building business by routing and packet forwarding. When a
>> packet needs to be forwarded farther, its meta is built in a
>> special manner preserving the route before EV_WRITE appears, and
>> on_output should not touch that meta.
>>
>> This patch adds a check preventing unnecessary meta rebuilds.
>> Besides, the check and the meta building code are moved into a
>> dedicated function out of swim_scheduler_on_output() - it allows
>> to completely split logic of packing a message and sending it.
>> Separated logic helps a lot when indirect pings are introduced.
>>
> 
> ok to push.
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

[tarantool-patches] Re: [PATCH 3/4] swim: fix a bug with invalidation of round msg in fly

[Vladislav Shpilevoy]

Pushed into the master.

On 18/04/2019 18:19, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:
> 
> Why have a concept of invalidation at all? Why can't you simply
> always keep the message up to date (built)? Don't you think member
> updates are so rare that you're winning very little by building on
> demand but the code is more complex because of it?
> 
> The patch is OK to push.
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

[tarantool-patches] Re: [PATCH 2/4] swim: extract binary ip/port into a separate struct

[Vladislav Shpilevoy]

Pushed into the master.

On 18/04/2019 18:17, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 09:03]:
>> At this moment there are two binary structures in the SWIM
>> protocol carrying an address: swim_member_passport and
>> swim_meta_header_bin - one address in each. This code duplication
>> was not formidable enough to stimulate creation of a separate
>> address structure.
>>
>> But forthcoming indirect messages protocol extensions will add 2
>> new cases of encoding a binary address. It triggered this patch
>> to reduce code duplication.
>>
> 
> OK to push.
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

[tarantool-patches] Re: [PATCH 1/4] swim: move sockaddr_in checkers to swim_proto.h

[Vladislav Shpilevoy]

Added a comment and pushed into the master.

On 18/04/2019 19:02, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/18 18:27]:
>> It's mainly against malicious and bad-formatter packets. Since
>> the protocol will be public, it can happen that custom drivers
>> will send bad packets.
> 
> This definitely needs a comment. Besides, it's a bad address then,
> not empty address, and such messages should be simply rejected.
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
>