Hi Vu,
I have the comments [M].
Thanks
Minh
On 13/9/19 6:40 pm, Nguyen Minh Vu wrote:
Hi Minh,
I have minor comments below.
Regards, Vu
On 8/14/19 1:38 PM, Minh Chau wrote:
This patch makes the solution of TIPC buffer overflow configurable,
as well as the ack timeout/ack size.
For example:
The service config file can export the following environment variables
export MDS_TIPC_FCTRL_ENABLED=1
export MDS_TIPC_FCTRL_ACKTIMEOUT=1000
export MDS_TIPC_FCTRL_ACKSIZE=1
If MDS_TIPC_FCTRL_ACKTIMEOUT, MDS_TIPC_FCTRL_ACKSIZE are not specified,
the default values are used.
---
src/mds/mds_dt_tipc.c | 19 ++++++++++++++++---
src/mds/mds_tipc_fctrl_intf.cc | 25 +++++++++++++++++++++++--
src/mds/mds_tipc_fctrl_intf.h | 3 ++-
3 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/src/mds/mds_dt_tipc.c b/src/mds/mds_dt_tipc.c
index fef1c50..1b6c3f8 100644
--- a/src/mds/mds_dt_tipc.c
+++ b/src/mds/mds_dt_tipc.c
@@ -342,9 +342,22 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t
*mds_tipc_ref)
}
/* Create flow control tasks if enabled*/
- gl_mds_pro_ver = MDS_PROT_FCTRL;
- mds_tipc_fctrl_initialize(tipc_cb.BSRsock, port_id,
- (uint64_t)optval, tipc_mcast_enabled);
+ char* ptr;
+ if ((ptr = getenv("MDS_TIPC_FCTRL_ENABLED")) != NULL) {
+ if (atoi(ptr) == 1) {
+ gl_mds_pro_ver = MDS_PROT_FCTRL;
+ int ackto = -1;
+ int acksize = -1;
+ if ((ptr = getenv("MDS_TIPC_FCTRL_ACKTIMEOUT")) != NULL) {
+ ackto = atoi(ptr);
+ }
+ if ((ptr = getenv("MDS_TIPC_FCTRL_ACKSIZE")) != NULL) {
+ acksize = atoi(ptr);
+ }
[Vu] Do we have valid range of these environment variables? What if
they mistakenly set them to empty values?
e.g:
export MDS_TIPC_FCTRL_ACKTIMEOUT=""
[M] We have base::GetEnv and will try to use it here, if not possible
due to this source file is C code, then will add more handling for
out-of-range values or a warning if it's set a value e.g too big.
+ mds_tipc_fctrl_initialize(tipc_cb.BSRsock, port_id, (uint64_t)optval,
+ ackto, acksize, tipc_mcast_enabled);
+ }
+ }
/* Create a task to receive the events and data */
if (mdtm_create_rcv_task(tipc_cb.hdle_mdtm) != NCSCC_RC_SUCCESS) {
diff --git a/src/mds/mds_tipc_fctrl_intf.cc
b/src/mds/mds_tipc_fctrl_intf.cc
index 397114e..8949937 100644
--- a/src/mds/mds_tipc_fctrl_intf.cc
+++ b/src/mds/mds_tipc_fctrl_intf.cc
@@ -40,6 +40,9 @@ using mds::ChunkAck;
using mds::HeaderMessage;
namespace {
+// flow control enabled/disabled
+bool is_fctrl_enabled = false;
+
// multicast/broadcast enabled
// todo: to be removed if flow control support it
bool is_mcast_enabled = true;
@@ -225,7 +228,8 @@ uint32_t create_ncs_task(void *task_hdl) {
} // end local namespace
uint32_t mds_tipc_fctrl_initialize(int dgramsock, struct
tipc_portid id,
- uint64_t rcv_buf_size, bool mcast_enabled) {
+ uint64_t rcv_buf_size, int32_t ackto, int32_t acksize,
+ bool mcast_enabled) {
if (create_ncs_task(&p_task_hdl) !=
NCSCC_RC_SUCCESS) {
m_MDS_LOG_ERR("FCTRL: Start of the Created Task-failed:\n");
@@ -234,8 +238,10 @@ uint32_t mds_tipc_fctrl_initialize(int
dgramsock, struct tipc_portid id,
data_sock_fd = dgramsock;
snd_rcv_portid = id;
sock_buf_size = rcv_buf_size;
+ is_fctrl_enabled = true;
is_mcast_enabled = mcast_enabled;
-
+ if (ackto != -1) kChunkAckTimeout = ackto;
+ if (acksize != -1) kChunkAckSize = acksize;
m_MDS_LOG_NOTIFY("FCTRL: Initialize [node:%x, ref:%u]",
id.node, id.ref);
@@ -243,6 +249,7 @@ uint32_t mds_tipc_fctrl_initialize(int
dgramsock, struct tipc_portid id,
}
uint32_t mds_tipc_fctrl_shutdown(void) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
if (ncs_task_release(p_task_hdl) != NCSCC_RC_SUCCESS) {
m_MDS_LOG_ERR("FCTRL: Stop of the Created Task-failed:\n");
}
@@ -251,6 +258,8 @@ uint32_t mds_tipc_fctrl_shutdown(void) {
uint32_t mds_tipc_fctrl_sndqueue_capable(struct tipc_portid id,
uint16_t len,
uint16_t* next_seq) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
uint32_t rc = NCSCC_RC_SUCCESS;
portid_map_mutex.lock();
[Vu] We has a common class base::Lock that can help to unlock
automatically when it goes out
the scope. Should we make portid_map_mutex to be an Lock object?
[M]: Yes I should use base::Lock, will change it.
@@ -274,6 +283,8 @@ uint32_t mds_tipc_fctrl_sndqueue_capable(struct
tipc_portid id, uint16_t len,
uint32_t mds_tipc_fctrl_trysend(const uint8_t *buffer, uint16_t len,
struct tipc_portid id) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
uint32_t rc = NCSCC_RC_SUCCESS;
portid_map_mutex.lock();
@@ -304,6 +315,8 @@ uint32_t mds_tipc_fctrl_trysend(const uint8_t
*buffer, uint16_t len,
}
uint32_t mds_tipc_fctrl_portid_up(struct tipc_portid id, uint32_t
type) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
MDS_SVC_ID svc_id = (uint16_t)(type & MDS_EVENT_MASK_FOR_SVCID);
portid_map_mutex.lock();
@@ -328,6 +341,8 @@ uint32_t mds_tipc_fctrl_portid_up(struct
tipc_portid id, uint32_t type) {
}
uint32_t mds_tipc_fctrl_portid_down(struct tipc_portid id,
uint32_t type) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
MDS_SVC_ID svc_id = (uint16_t)(type & MDS_EVENT_MASK_FOR_SVCID);
portid_map_mutex.lock();
@@ -345,6 +360,8 @@ uint32_t mds_tipc_fctrl_portid_down(struct
tipc_portid id, uint32_t type) {
}
uint32_t mds_tipc_fctrl_portid_terminate(struct tipc_portid id) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
portid_map_mutex.lock();
// Delete this tipc portid out of the map
@@ -362,6 +379,8 @@ uint32_t mds_tipc_fctrl_portid_terminate(struct
tipc_portid id) {
uint32_t mds_tipc_fctrl_drop_data(uint8_t *buffer, uint16_t len,
struct tipc_portid id) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
HeaderMessage header;
header.Decode(buffer);
// if mds support flow control
@@ -402,6 +421,8 @@ uint32_t mds_tipc_fctrl_drop_data(uint8_t
*buffer, uint16_t len,
uint32_t mds_tipc_fctrl_rcv_data(uint8_t *buffer, uint16_t len,
struct tipc_portid id) {
+ if (is_fctrl_enabled == false) return NCSCC_RC_SUCCESS;
+
HeaderMessage header;
header.Decode(buffer);
// if mds support flow control
diff --git a/src/mds/mds_tipc_fctrl_intf.h
b/src/mds/mds_tipc_fctrl_intf.h
index 85a058f..c798b93 100644
--- a/src/mds/mds_tipc_fctrl_intf.h
+++ b/src/mds/mds_tipc_fctrl_intf.h
@@ -27,7 +27,8 @@ extern "C" {
#endif
uint32_t mds_tipc_fctrl_initialize(int dgramsock, struct
tipc_portid id,
- uint64_t rcv_buf_size, bool mbrcast_enabled);
+ uint64_t rcv_buf_size, int32_t ackto,
+ int32_t acksize, bool mbrcast_enabled);
uint32_t mds_tipc_fctrl_shutdown(void);
uint32_t mds_tipc_fctrl_rcv_data(uint8_t *buffer, uint16_t len,
struct tipc_portid id);
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
