Hi Vu, Thanks, I will update and send out new version. See my replies inline.
Best Regards, ThuanTr From: Nguyen Minh Vu <vu.m.ngu...@dektech.com.au> Sent: Monday, February 17, 2020 12:29 PM To: Thuan Tran <thuan.t...@dektech.com.au>; Minh Hon Chau <minh.c...@dektech.com.au>; Thang Duc Nguyen <thang.d.ngu...@dektech.com.au>; Gary Lee <gary....@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [PATCH 1/1] base: add serial number arithmetic (RFC1982) [#3074] Hi Thuan, Thanks. See my responses inline. Regards, Vu On 2/14/20 11:48 AM, Thuan Tran wrote: Hi Vu, Thanks for comments. Please check my replies inline. Best Regards, ThuanTr From: Nguyen Minh Vu <vu.m.ngu...@dektech.com.au><mailto:vu.m.ngu...@dektech.com.au> Sent: Thursday, February 13, 2020 5:50 PM To: Thuan Tran <thuan.t...@dektech.com.au><mailto:thuan.t...@dektech.com.au>; Minh Hon Chau <minh.c...@dektech.com.au><mailto:minh.c...@dektech.com.au>; Thang Duc Nguyen <thang.d.ngu...@dektech.com.au><mailto:thang.d.ngu...@dektech.com.au>; Gary Lee <gary....@dektech.com.au><mailto:gary....@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: Re: [PATCH 1/1] base: add serial number arithmetic (RFC1982) [#3074] Hi Thuan, Ack with comments inline. Regards, Vu On 2/12/20 5:36 PM, thuan.tran wrote: - Adapt MDS with this SNA implementation. --- src/base/Makefile.am | 6 +- src/base/sna.h | 136 +++++++++++++++++++++++++++++++ src/base/tests/sna_test.cc | 117 ++++++++++++++++++++++++++ src/mds/mds_tipc_fctrl_intf.cc | 2 +- src/mds/mds_tipc_fctrl_portid.cc | 17 ++-- src/mds/mds_tipc_fctrl_portid.h | 64 +-------------- 6 files changed, 267 insertions(+), 75 deletions(-) create mode 100644 src/base/sna.h create mode 100644 src/base/tests/sna_test.cc diff --git a/src/base/Makefile.am b/src/base/Makefile.am index 025fb86a2..5082175cf 100644 --- a/src/base/Makefile.am +++ b/src/base/Makefile.am @@ -173,7 +173,8 @@ noinst_HEADERS += \ src/base/unix_client_socket.h \ src/base/unix_server_socket.h \ src/base/unix_socket.h \ - src/base/usrbuf.h + src/base/usrbuf.h \ + src/base/sna.h TESTS += bin/testleap bin/libbase_test bin/core_common_test @@ -237,7 +238,8 @@ bin_libbase_test_SOURCES = \ src/base/tests/time_compare_test.cc \ src/base/tests/time_convert_test.cc \ src/base/tests/time_subtract_test.cc \ - src/base/tests/unix_socket_test.cc + src/base/tests/unix_socket_test.cc \ + src/base/tests/sna_test.cc bin_libbase_test_LDADD = \ $(GTEST_DIR)/lib/libgtest.la \ diff --git a/src/base/sna.h b/src/base/sna.h new file mode 100644 index 000000000..fee6627bb --- /dev/null +++ b/src/base/sna.h @@ -0,0 +1,136 @@ +/* -*- OpenSAF -*- + * + * Copyright Ericsson AB 2020 - All Rights Reserved. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed + * under the GNU Lesser General Public License Version 2.1, February 1999. + * The complete license can be accessed from the following location: + * http://opensource.org/licenses/lgpl-license.php + * See the Copying file included with the OpenSAF distribution for full + * licensing terms. + * + * Reference: Serial Number Arithmetic from RFC1982 + * + */ + +#ifndef BASE_SNA_H_ +#define BASE_SNA_H_ + +#include <typeinfo> +#include <stdexcept> + +#define SNA16_MAX 65536 // 2^16 +#define SNA16_SPACE 32768 // (2^16)/2 +#define SNA32_MAX 4294967296 // 2^32 +#define SNA32_SPACE 2147483648 // (2^32)/2 [Vu] can use: #define SNA16_MAX (1 << 16) #define SNA32_MAX (1 << 32) SPACE ones probably are not necessary. See my comment for space() method below. [Thuan] Is there any benefit with 1 << 16 or 32? I think define a clear value is better. [Vu] the benefit is you won't need to add the comment e.g. // 2^16 [Thuan] OK, change as your suggestion. Btw, I will change define macro SPACE (MAX/2). About space(), it is intended because I don’t want CPU calculate every time refer to it. + +template <class T> +class _sna { [Vu] How about `class SerialNumber {}` [Thuan] OK, I will change the class name. + private: [Vu] Declaration order should start with a public: section, followed by protected:, then private: [Thuan] OK, will change order. + T i; [Vu] Should use a descriptive name. e.g: T value_ {0}; [Thuan] OK, will update the name. + uint64_t max() { + if (typeid(T) == typeid(uint64_t)) { + return SNA32_MAX; + } + if (typeid(T) == typeid(uint32_t)) { + return SNA16_MAX; + } + throw std::out_of_range("Invalid type"); [Vu] OpenSAF code does not handle exception. Should use assertion instead. e.g: assert(0 && "Invalid data type"); [Thuan] I throw to do basic test, if assert() then cannot test the case. Btw, I think throw exception without try catch will also end with terminate? [Vu] I think so; and if you you do tests on Seq16, Seq32, you probably won't reach exceptions. [Thuan] OK, change to assert() inside max()/space(), but keep other “throw” is still kept for test. + } + uint64_t space() { + if (typeid(T) == typeid(uint64_t)) { + return SNA32_SPACE; + } + if (typeid(T) == typeid(uint32_t)) { + return SNA16_SPACE; + } + throw std::out_of_range("Invalid type"); [Vu] uint64_t space() { return max()/2;} [Thuan] As explain above, I don’t want calculate every time refer it. + } + + public: + _sna(): i(0) {} + _sna(const _sna &t) { + i = t.i; + } + explicit _sna(const uint64_t &n) { + if ((n < 0) || (n > (max()-1))) + throw std::out_of_range("Invalid initial value"); [Vu] Use assert() instead of throwing exception. [Thuan] Same above. + i = n; + } + _sna& operator=(const _sna &t) { + // check for self-assignment + if (&t == this) + return *this; + i = t.i; + return *this; + } + T v() const { + return i; + } + _sna& operator+=(const uint64_t& n) { + if ((n < 0) || (n > (space() - 1))) + throw std::out_of_range("Invalid addition value"); + i = (i + n) % max(); + return *this; + } + friend _sna operator+(_sna m, const uint64_t& n) { [Vu] my suggestion: friend _sna operator+(const _sna& m, ...) { _sna s{m}; s += n; return s; } [Thuan] passing lhs by value helps optimize chained a+b+c I prefer to this link https://en.cppreference.com/w/cpp/language/operators + m += n; + return m; + } + // prefix ++ + _sna& operator++() { + *this += 1; + return *this; + } + // postfix ++ + _sna operator++(int) { + _sna tmp(*this); + operator++(); + return tmp; + } + bool operator==(const _sna& rhs) { + return i == rhs.i; + } + bool operator==(const uint32_t val) { + return i == val; + } + bool operator!=(const _sna& rhs) { + return i != rhs.i; + } + bool operator<(const _sna& rhs) { + return (i < rhs.i && rhs.i - i < space()) || \ + (i > rhs.i && i - rhs.i > space()); + } + bool operator<=(const _sna& rhs) { + return *this == rhs || *this < rhs; + } + bool operator>(const _sna& rhs) { + return (i < rhs.i && rhs.i - i > space()) || \ + (i > rhs.i && i - rhs.i < space()); [Vu] shorter version: return !(*this <= rhs); [Thuan] This statement cause the issue in Erase message which run test sometimes failure. + } + bool operator>=(const _sna& rhs) { + return *this == rhs || *this > rhs; + } [Vu] Shorter version: return !(*this < rhs); [Thuan] This statement cause the issue in Erase message which run test sometimes failure. + int64_t operator-(const _sna& rhs) { + if (*this >= rhs) { + if (i >= rhs.v()) { + return i - rhs.v(); + } else { + return (i + max()) - rhs.v(); + } + } else { + if (i < rhs.v()) { + return i - rhs.v(); + } else { + return i - (rhs.v() + max()); + } + } + } +}; + +typedef _sna<uint32_t> Seq16; +typedef _sna<uint64_t> Seq32; [Vu] another option: using Seq16 = _sna<uint32_t>; [Thuan] Is there any benefit with this instead of typedef? [Vu] 'using' is a recommendation in c++ google coding rule. https://google.github.io/styleguide/cppguide.html#Aliases [Thuan] OK, change to this style. + +#endif // BASE_SNA_H_ diff --git a/src/base/tests/sna_test.cc b/src/base/tests/sna_test.cc new file mode 100644 index 000000000..b3d2d014c --- /dev/null +++ b/src/base/tests/sna_test.cc @@ -0,0 +1,117 @@ +/* -*- OpenSAF -*- + * + * Copyright Ericsson AB 2019 - All Rights Reserved. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed + * under the GNU Lesser General Public License Version 2.1, February 1999. + * The complete license can be accessed from the following location: + * http://opensource.org/licenses/lgpl-license.php + * See the Copying file included with the OpenSAF distribution for full + * licensing terms. + * + * Reference: Serial Number Arithmetic from RFC1982 + * + */ + +#include "base/sna.h" +#include "gtest/gtest.h" + +template <class T> +int test_sna(T x) { + int rc = 1; + printf("\n============= START with x=%lu =============\n", (uint64_t)x.v()); + T y = x; + printf("x=%lu, y=%lu: check x == y++ is TRUE\n", + (uint64_t)x.v(), (uint64_t)y.v()); + if (x == y++) { + printf("now y=%lu, reset y = x\n", (uint64_t)y.v()); + y = x; + printf("x=%lu, y=%lu: check x != ++y is TRUE\n", + (uint64_t)x.v(), (uint64_t)y.v()); + if (x != ++y) { + printf("now y=%lu, reset y = x\n", (uint64_t)y.v()); + y = x; + printf("x=%lu, y=%lu: check x < ++y is TRUE\n", + (uint64_t)x.v(), (uint64_t)y.v()); + if (x < ++y) { + printf("x=%lu: check x + 1 > x and x + 1 >= x is TRUE\n", + (uint64_t)x.v()); + if ((x + 1 > x) && (x + 1 >= x)) { + printf("x=%lu: check x < x + 1 and x <= x + 1 is TRUE\n", + (uint64_t)x.v()); + y = x + 1; + printf("y = x+1 => y=%lu\n", (uint64_t)y.v()); + y = y + 1; + printf("y = y+1 => y=%lu\n", (uint64_t)y.v()); + if ((x < x + 1) && (x <= x + 1)) { + try { + printf("x=%lu: add invalid (-1)\n", (uint64_t)x.v()); + x = x + (-1); + } catch (const std::out_of_range& oor) { + printf("Expected error: %s\n", oor.what()); + try { + uint64_t max_value = 0; + if (typeid(T) == typeid(Seq16)) + max_value = SNA16_MAX; + else if (typeid(T) == typeid(Seq32)) + max_value = SNA32_MAX; + printf("x=%lu: add invalid (%lu)\n", + (uint64_t)x.v(), max_value); + x = x + max_value; + } catch (const std::out_of_range& oor) { + printf("Expected error: %s\n", oor.what()); + rc = 0; + } + } + } + } + } + } + } + printf("================ END with x=%lu ==============\n", (uint64_t)x.v()); [Vu] Should remove all printf(); Developer can add it when following macros got failures. [Thuan] Is there any problem if I keep it? [Vu] I think printf should only be using in 'debug' version. [Thuan] OK, will hide these printf(). [Vu] However, I think we should test on `Seq16` and `Seq32` data type only. [Thuan] Yes, I test Seq16 and Seq32. + return rc; +} + + +class SnaTest : public ::testing::Test { + protected: + SnaTest() {} + virtual ~SnaTest() { + // Cleanup work that doesn't throw exceptions here. + } + virtual void SetUp() { + // Code here will be called immediately after the constructor (right + // before each test) + } + virtual void TearDown() {} +}; + [Vu] Probably don't need this class. [Thuan] I just keep them as other test files in base. +TEST_F(SnaTest, unit16_sna) { + Seq16 x; + EXPECT_EQ(0, test_sna(x)); + Seq16 x1 = Seq16(1); + Seq16 x2 = Seq16(SNA16_MAX - 1); + EXPECT_EQ(2, x1 - x2); + EXPECT_EQ(-2, x2 - x1); + EXPECT_EQ(0, test_sna(x1)); + EXPECT_EQ(0, test_sna(x2)); [Vu] The production code only use Seq16 & Seq32; so I think we only do test on these data types. [Thuan] Yes, I only test Seq16 & Seq32. +} + +TEST_F(SnaTest, unit32_sna) { + Seq32 x; + EXPECT_EQ(0, test_sna(x)); + Seq32 x1 = Seq32(1); + Seq32 x2 = Seq32(SNA32_MAX - 1); + EXPECT_EQ(2, x1 - x2); + EXPECT_EQ(-2, x2 - x1); + EXPECT_EQ(0, test_sna(x1)); + EXPECT_EQ(0, test_sna(x2)); +} + + +int main(int argc, char **argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/src/mds/mds_tipc_fctrl_intf.cc b/src/mds/mds_tipc_fctrl_intf.cc index f3883ba36..9c7d4e35c 100644 --- a/src/mds/mds_tipc_fctrl_intf.cc +++ b/src/mds/mds_tipc_fctrl_intf.cc @@ -106,7 +106,7 @@ void process_timer_event(const Event& evt) { static_cast<int>(evt.type_)); for (auto i : portid_map) { TipcPortId* portid = i.second; - + if (!portid) continue; if (evt.type_ == Event::Type::kEvtTmrTxProb) { if (portid->ReceiveTmrTxProb(kTxProbMaxRetries) == true) { txprob_restart = true; diff --git a/src/mds/mds_tipc_fctrl_portid.cc b/src/mds/mds_tipc_fctrl_portid.cc index 3562c4a00..b0f765d69 100644 --- a/src/mds/mds_tipc_fctrl_portid.cc +++ b/src/mds/mds_tipc_fctrl_portid.cc @@ -339,7 +339,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t mfrag, } // update receiver sequence window - if (rcvwnd_.acked_ < Seq16(fseq) && rcvwnd_.rcv_ + Seq16(1) == Seq16(fseq)) { + if (rcvwnd_.acked_ < Seq16(fseq) && rcvwnd_.rcv_ + 1 == Seq16(fseq)) { m_MDS_LOG_DBG("FCTRL: [me] <-- [node:%x, ref:%u], " "RcvData[mseq:%u, mfrag:%u, fseq:%u], " "rcvwnd[acked:%u, rcv:%u, nacked:%" PRIu64 "]", @@ -370,7 +370,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t mfrag, // It is not used for now, so ignore it. // check for transmission error - if (rcvwnd_.rcv_ + Seq16(1) < Seq16(fseq)) { + if (rcvwnd_.rcv_ + 1 < Seq16(fseq)) { if (rcvwnd_.rcv_ == 0 && rcvwnd_.acked_ == 0) { // peer does not realize that this portid reset m_MDS_LOG_ERR("FCTRL: [me] <-- [node:%x, ref:%u], " @@ -381,7 +381,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t mfrag, mseq, mfrag, fseq, rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_); - rcvwnd_.rcv_ = fseq; + rcvwnd_.rcv_ = Seq16(fseq); } else { rc = NCSCC_RC_FAILURE; // msg loss @@ -393,7 +393,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t mfrag, mseq, mfrag, fseq, rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_); // send nack - SendNack((rcvwnd_.rcv_ + Seq16(1)).v(), svc_id); + SendNack((rcvwnd_.rcv_ + 1).v(), svc_id); } } else if (Seq16(fseq) <= rcvwnd_.rcv_) { rc = NCSCC_RC_FAILURE; @@ -456,13 +456,12 @@ void TipcPortId::ReceiveChunkAck(uint16_t fseq, uint16_t chksize) { sndwnd_.acked_.v(), sndwnd_.send_.v(), sndwnd_.nacked_space_, sndqueue_.Size()); - // fast forward the sndwnd_.acked_ sequence to fseq - sndwnd_.acked_ = fseq; - // remove a number @chksize messages out of sndqueue_ and decrease // the nacked_space_ of sender - uint64_t acked_bytes = sndqueue_.Erase(Seq16(fseq) - (chksize-1), - Seq16(fseq)); + uint64_t acked_bytes = sndqueue_.Erase(sndwnd_.acked_ + 1, Seq16(fseq)); + // fast forward the sndwnd_.acked_ sequence to fseq + sndwnd_.acked_ = Seq16(fseq); + assert(sndwnd_.nacked_space_ >= acked_bytes); sndwnd_.nacked_space_ -= acked_bytes; diff --git a/src/mds/mds_tipc_fctrl_portid.h b/src/mds/mds_tipc_fctrl_portid.h index ef74921e7..83564459b 100644 --- a/src/mds/mds_tipc_fctrl_portid.h +++ b/src/mds/mds_tipc_fctrl_portid.h @@ -24,73 +24,11 @@ #include <stdio.h> #include <unistd.h> #include <deque> +#include "base/sna.h" #include "mds/mds_tipc_fctrl_msg.h" namespace mds { -class Seq16 { - public: -#define SEQ16_MAX 65536 -#define SEQ16_SPACE 32768 - uint16_t value_; - explicit Seq16(uint16_t v) { - value_ = uint16_t((uint32_t)v % SEQ16_MAX); - } - uint16_t v() { - return value_; - } - Seq16 operator + (const Seq16 add) const { - return Seq16(((uint32_t)value_ + (uint32_t)add.value_) % SEQ16_MAX); - } - - int16_t operator - (const Seq16 sub) const { - if (value_ < sub.value_ && (sub.value_ - value_ < SEQ16_SPACE)) { - return value_ - sub.value_; - } - if (value_ > sub.value_ && (value_ - sub.value_ > SEQ16_SPACE)) { - return (int32_t)value_ + SEQ16_MAX - (int32_t)sub.value_; - } - if (value_ < sub.value_ && (sub.value_ - value_ > SEQ16_SPACE)) { - return (int32_t)value_ + SEQ16_MAX - (int32_t)sub.value_; - } - if (value_ > sub.value_ && (value_ - sub.value_ < SEQ16_SPACE)) { - return value_ - sub.value_; - } - return 0; - } - Seq16 operator - (const uint16_t sub) const { - return Seq16(((uint32_t)value_ + 65536 - sub) % SEQ16_MAX); - } - void operator ++() { - value_ = (value_ + 1) % SEQ16_MAX; - } - void operator = (const uint16_t v) { - value_ = v % SEQ16_MAX; - } - bool operator == (const Seq16& seq) const { - return value_ == seq.value_; - } - bool operator == (uint16_t val) const { - return value_ == val; - } - bool operator <= (const Seq16& seq) { - return *this == seq || *this < seq; - } - bool operator < (const Seq16& seq) { - if (value_ < seq.value_ && (seq.value_ - value_ < SEQ16_SPACE)) return true; - if (value_ > seq.value_ && (value_ - seq.value_ > SEQ16_SPACE)) return true; - return false; - } - bool operator > (const Seq16& seq) { - if (value_ < seq.value_ && (seq.value_ - value_ > SEQ16_SPACE)) return true; - if (value_ > seq.value_ && (value_ - seq.value_ < SEQ16_SPACE)) return true; - return false; - } - bool operator >= (const Seq16& seq) { - return *this == seq || *this > seq; - } -}; - class MessageQueue { public: void Queue(DataMessage* msg); _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
