From 7e83421cf0d3e8a74dcd67a66d53c43ae128a635 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Fri, 4 Oct 2019 09:59:59 -0700 Subject: [PATCH 1/7] btl/usnic: s/get_nsec/get_nticks/g Rename "get_nsec()" to "get_ticks()" to more accurately reflect that this function has no correlation to wall clock time at all. Signed-off-by: Jeff Squyres (cherry picked from commit ce2910a28aea61043b81324c67999f3a47cfe7ac) --- opal/mca/btl/usnic/btl_usnic.h | 2 +- opal/mca/btl/usnic/btl_usnic_module.c | 2 +- opal/mca/btl/usnic/btl_usnic_recv.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/opal/mca/btl/usnic/btl_usnic.h b/opal/mca/btl/usnic/btl_usnic.h index e8f6dafa2de..d25d6309bbd 100644 --- a/opal/mca/btl/usnic/btl_usnic.h +++ b/opal/mca/btl/usnic/btl_usnic.h @@ -68,7 +68,7 @@ extern uint64_t opal_btl_usnic_ticks; extern opal_recursive_mutex_t btl_usnic_lock; static inline uint64_t -get_nsec(void) +get_ticks(void) { return opal_btl_usnic_ticks; } diff --git a/opal/mca/btl/usnic/btl_usnic_module.c b/opal/mca/btl/usnic/btl_usnic_module.c index ba0442c43c4..2c3d8e04635 100644 --- a/opal/mca/btl/usnic/btl_usnic_module.c +++ b/opal/mca/btl/usnic/btl_usnic_module.c @@ -1236,7 +1236,7 @@ opal_btl_usnic_module_progress_sends( /* Is it time to send ACK? */ if (endpoint->endpoint_acktime == 0 || - endpoint->endpoint_acktime <= get_nsec()) { + endpoint->endpoint_acktime <= get_ticks()) { if (OPAL_LIKELY(opal_btl_usnic_ack_send(module, endpoint) == OPAL_SUCCESS)) { opal_btl_usnic_remove_from_endpoints_needing_ack(endpoint); } else { diff --git a/opal/mca/btl/usnic/btl_usnic_recv.h b/opal/mca/btl/usnic/btl_usnic_recv.h index 7e056e488db..256132f4b59 100644 --- a/opal/mca/btl/usnic/btl_usnic_recv.h +++ b/opal/mca/btl/usnic/btl_usnic_recv.h @@ -114,7 +114,7 @@ opal_btl_usnic_update_window( /* give this process a chance to send something before ACKing */ if (0 == endpoint->endpoint_acktime) { - endpoint->endpoint_acktime = get_nsec() + 50000; /* 50 usec */ + endpoint->endpoint_acktime = get_ticks() + 50000; } /* Save this incoming segment in the received segmentss array on the From 2150922e2cb938b0b77fc98122ea705cc2d5944f Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Fri, 4 Oct 2019 11:52:48 -0700 Subject: [PATCH 2/7] btl/usnic: clarifications and fixes regarding ACKs New MCA parameter: btl_usnic_ack_iteration_delay. Set this to the number of times through the usNIC component progress function before sending a standalone ACK (vs. piggy-backing the ACK on any other send going to the target peer). Use "ticks" language to clarify that we're really counting the number of times through the usNIC component DATA_CHANNEL completion check (to check for incoming messages) -- it has no relation to wall clock time whatsoever. Also slightly change the channel-checking scheme in usNIC component progress: only check the PRIORITY channel once (vs. checking it once, not finding anything, and then falling through the progress_2() where we check PRIORITY again and then check the DATA channel). As before, if our "progress" libevent fires, increment the tick counter enough to guarantee that all endpoints that need an ACK will get triggered to send standalone ACKs the next time through progress, if necessary. Signed-off-by: Jeff Squyres (cherry picked from commit 968b1a51b59898877a8c7268d463d3d7d78d86a3) --- opal/mca/btl/usnic/btl_usnic.h | 4 ++++ opal/mca/btl/usnic/btl_usnic_component.c | 25 +++++++++++++----------- opal/mca/btl/usnic/btl_usnic_mca.c | 4 ++++ opal/mca/btl/usnic/btl_usnic_recv.h | 7 +++++-- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/opal/mca/btl/usnic/btl_usnic.h b/opal/mca/btl/usnic/btl_usnic.h index d25d6309bbd..3251b673698 100644 --- a/opal/mca/btl/usnic/btl_usnic.h +++ b/opal/mca/btl/usnic/btl_usnic.h @@ -206,6 +206,10 @@ typedef struct opal_btl_usnic_component_t { /** retrans characteristics */ int retrans_timeout; + /** minimum number of times through component progress before + checking to see if standalone ACKs need to be sent */ + int ack_iteration_delay; + /** transport header length for all usNIC devices on this server (it is guaranteed that all usNIC devices on a single server will have the same underlying transport, and therefore the diff --git a/opal/mca/btl/usnic/btl_usnic_component.c b/opal/mca/btl/usnic/btl_usnic_component.c index 25a64a25d26..629d292c305 100644 --- a/opal/mca/btl/usnic/btl_usnic_component.c +++ b/opal/mca/btl/usnic/btl_usnic_component.c @@ -384,8 +384,9 @@ static int check_usnic_config(opal_btl_usnic_module_t *module, static void usnic_clock_callback(int fd, short flags, void *timeout) { - /* 1ms == 1,000,000 ns */ - opal_btl_usnic_ticks += 1000000; + /* Increase by so many ticks that we will definitely force sending + any ACKs that are pending */ + opal_btl_usnic_ticks += 1000; /* run progress to make sure time change gets noticed */ usnic_component_progress(); @@ -1132,7 +1133,7 @@ static mca_btl_base_module_t** usnic_component_init(int* num_btl_modules, */ static int usnic_handle_completion(opal_btl_usnic_module_t* module, opal_btl_usnic_channel_t *channel, struct fi_cq_entry *completion); -static int usnic_component_progress_2(void); +static int usnic_component_progress_2(bool check_priority); static void usnic_handle_cq_error(opal_btl_usnic_module_t* module, opal_btl_usnic_channel_t *channel, int cq_ret); @@ -1145,9 +1146,7 @@ static int usnic_component_progress(void) struct fi_cq_entry completion; opal_btl_usnic_channel_t *channel; static bool fastpath_ok = true; - - /* update our simulated clock */ - opal_btl_usnic_ticks += 5000; + bool check_priority = true; count = 0; if (fastpath_ok) { @@ -1180,10 +1179,11 @@ static int usnic_component_progress(void) usnic_handle_cq_error(module, channel, ret); } } + check_priority = false; } fastpath_ok = true; - return count + usnic_component_progress_2(); + return count + usnic_component_progress_2(check_priority); } static int usnic_handle_completion( @@ -1304,7 +1304,7 @@ usnic_handle_cq_error(opal_btl_usnic_module_t* module, } } -static int usnic_component_progress_2(void) +static int usnic_component_progress_2(bool check_priority) { int i, j, count = 0, num_events, ret; opal_btl_usnic_module_t* module; @@ -1313,15 +1313,18 @@ static int usnic_component_progress_2(void) int rc; int c; - /* update our simulated clock */ - opal_btl_usnic_ticks += 5000; + opal_btl_usnic_ticks += 1; + + /* If we need to check priority, start with the priority channel. + Otherwise, just check the data channel. */ + int c_start = check_priority ? USNIC_PRIORITY_CHANNEL : USNIC_DATA_CHANNEL; /* Poll for completions */ for (i = 0; i < mca_btl_usnic_component.num_modules; i++) { module = mca_btl_usnic_component.usnic_active_modules[i]; /* poll each channel */ - for (c=0; cmod_channels[c]; if (channel->chan_deferred_recv != NULL) { diff --git a/opal/mca/btl/usnic/btl_usnic_mca.c b/opal/mca/btl/usnic/btl_usnic_mca.c index 84f987cf22c..515f32abaaa 100644 --- a/opal/mca/btl/usnic/btl_usnic_mca.c +++ b/opal/mca/btl/usnic/btl_usnic_mca.c @@ -260,6 +260,10 @@ int opal_btl_usnic_component_register(void) 5000, &mca_btl_usnic_component.retrans_timeout, REGINT_GE_ONE, OPAL_INFO_LVL_5)); + CHECK(reg_int("ack_iteration_delay", "Minimum number of times through usNIC \"progress\" function before checking to see if standalone ACKs need to be sent", + 0, &mca_btl_usnic_component.ack_iteration_delay, + REGINT_GE_ZERO, OPAL_INFO_LVL_5)); + CHECK(reg_int("priority_limit", "Max size of \"priority\" messages (0 = use pre-set defaults; depends on number and type of devices available)", 0, &max_tiny_msg_size, REGINT_GE_ZERO, OPAL_INFO_LVL_5)); diff --git a/opal/mca/btl/usnic/btl_usnic_recv.h b/opal/mca/btl/usnic/btl_usnic_recv.h index 256132f4b59..7a178c1630f 100644 --- a/opal/mca/btl/usnic/btl_usnic_recv.h +++ b/opal/mca/btl/usnic/btl_usnic_recv.h @@ -112,9 +112,12 @@ opal_btl_usnic_update_window( opal_btl_usnic_add_to_endpoints_needing_ack(endpoint); } - /* give this process a chance to send something before ACKing */ + /* A hueristic: set to send this ACK after we have checked our + incoming DATA_CHANNEL component.act_iteration_delay times + (i.e., so we can piggyback an ACK on an outgoing send) */ if (0 == endpoint->endpoint_acktime) { - endpoint->endpoint_acktime = get_ticks() + 50000; + endpoint->endpoint_acktime = + get_ticks() + mca_btl_usnic_component.ack_iteration_delay; } /* Save this incoming segment in the received segmentss array on the From 74741b9265d63347f5e7fc18c1966f086631fe4a Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Fri, 4 Oct 2019 11:59:01 -0700 Subject: [PATCH 3/7] btl/usnic: increase default retrans_timeout Significantly increase the default retrans timeout. If the retrans timeout is too soon, we can end up in a retransmission storm where the logic will continually re-transmit the same frames during a single run through the usNIC progress function (because the timer for a single frame expires before we have run through re-transmitting all the frames pending re-transmission). Signed-off-by: Jeff Squyres (cherry picked from commit 3cc95d86b2123f38f392e56adca7ac8a1fef6454) --- opal/mca/btl/usnic/btl_usnic_mca.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opal/mca/btl/usnic/btl_usnic_mca.c b/opal/mca/btl/usnic/btl_usnic_mca.c index 515f32abaaa..54050dde300 100644 --- a/opal/mca/btl/usnic/btl_usnic_mca.c +++ b/opal/mca/btl/usnic/btl_usnic_mca.c @@ -257,7 +257,7 @@ int opal_btl_usnic_component_register(void) mca_btl_usnic_component.udp_port_base = (int) udp_port_base; CHECK(reg_int("retrans_timeout", "Number of microseconds before retransmitting a frame", - 5000, &mca_btl_usnic_component.retrans_timeout, + 100000, &mca_btl_usnic_component.retrans_timeout, REGINT_GE_ONE, OPAL_INFO_LVL_5)); CHECK(reg_int("ack_iteration_delay", "Minimum number of times through usNIC \"progress\" function before checking to see if standalone ACKs need to be sent", From ffdeaa714328852feea4bdf405e46ede538044e0 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Fri, 4 Oct 2019 12:05:13 -0700 Subject: [PATCH 4/7] btl/usnic: cap the number of resends per progress iteration New MCA param: btl_usnic_max_resends_per_iteration. This is the max number of resends we'll do in a single pass through usNIC component progress. This prevents progress from getting stuck in an endless loop of retransmissions (i.e., if more retransmissions are triggered during the sending of retransmissions). Specifically: we need to leave the resend loop to allow receives to happen (which may ACK messages we have sent previously, and therefore cause pending resends to be moot). Signed-off-by: Jeff Squyres (cherry picked from commit 27e3040dfeba00a9a2615a217c164899f0009e59) --- opal/mca/btl/usnic/btl_usnic.h | 4 ++++ opal/mca/btl/usnic/btl_usnic_mca.c | 4 ++++ opal/mca/btl/usnic/btl_usnic_module.c | 7 +++++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/opal/mca/btl/usnic/btl_usnic.h b/opal/mca/btl/usnic/btl_usnic.h index 3251b673698..b4831d82d9a 100644 --- a/opal/mca/btl/usnic/btl_usnic.h +++ b/opal/mca/btl/usnic/btl_usnic.h @@ -206,6 +206,10 @@ typedef struct opal_btl_usnic_component_t { /** retrans characteristics */ int retrans_timeout; + /** max number of messages re-sent during a single progress + iteration */ + int max_resends_per_iteration; + /** minimum number of times through component progress before checking to see if standalone ACKs need to be sent */ int ack_iteration_delay; diff --git a/opal/mca/btl/usnic/btl_usnic_mca.c b/opal/mca/btl/usnic/btl_usnic_mca.c index 54050dde300..f0399cbeb8c 100644 --- a/opal/mca/btl/usnic/btl_usnic_mca.c +++ b/opal/mca/btl/usnic/btl_usnic_mca.c @@ -260,6 +260,10 @@ int opal_btl_usnic_component_register(void) 100000, &mca_btl_usnic_component.retrans_timeout, REGINT_GE_ONE, OPAL_INFO_LVL_5)); + CHECK(reg_int("max_resends_per_iteration", "Maximum number of frames to resend in a single iteration through usNIC component progress", + 16, &mca_btl_usnic_component.max_resends_per_iteration, + REGINT_GE_ONE, OPAL_INFO_LVL_5)); + CHECK(reg_int("ack_iteration_delay", "Minimum number of times through usNIC \"progress\" function before checking to see if standalone ACKs need to be sent", 0, &mca_btl_usnic_component.ack_iteration_delay, REGINT_GE_ZERO, OPAL_INFO_LVL_5)); diff --git a/opal/mca/btl/usnic/btl_usnic_module.c b/opal/mca/btl/usnic/btl_usnic_module.c index 2c3d8e04635..086f99f5128 100644 --- a/opal/mca/btl/usnic/btl_usnic_module.c +++ b/opal/mca/btl/usnic/btl_usnic_module.c @@ -963,11 +963,12 @@ usnic_do_resends( opal_btl_usnic_send_segment_t *sseg; opal_btl_usnic_endpoint_t *endpoint; struct opal_btl_usnic_channel_t *data_channel; - int ret; + int ret, count; data_channel = &module->mod_channels[USNIC_DATA_CHANNEL]; - while ((get_send_credits(data_channel) > 1) && + count = mca_btl_usnic_component.max_resends_per_iteration; + while (count > 0 && (get_send_credits(data_channel) > 1) && !opal_list_is_empty(&module->pending_resend_segs)) { /* @@ -1009,6 +1010,8 @@ usnic_do_resends( BTL_ERROR(("hotel checkin failed\n")); abort(); /* should not be possible */ } + + --count; } } From c736d1a1728a503d4c78ae5889ed261029dd9257 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Fri, 4 Oct 2019 14:40:56 -0700 Subject: [PATCH 5/7] btl/usnic: properly size freelist items Move the prefix area from the head to the body in relevant size computations. This fixes a problem in high traffic situations where usNIC may have sent from unregistered memory. Signed-off-by: Jeff Squyres (cherry picked from commit fe7f772f21627b01838c007db7cedbbb0ce8b536) --- opal/mca/btl/usnic/btl_usnic_module.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/opal/mca/btl/usnic/btl_usnic_module.c b/opal/mca/btl/usnic/btl_usnic_module.c index 086f99f5128..94aefc9b715 100644 --- a/opal/mca/btl/usnic/btl_usnic_module.c +++ b/opal/mca/btl/usnic/btl_usnic_module.c @@ -2369,14 +2369,14 @@ static void init_freelists(opal_btl_usnic_module_t *module) uint32_t segsize; segsize = (module->local_modex.max_msg_size + - opal_cache_line_size - 1) & + mca_btl_usnic_component.prefix_send_offset + + opal_cache_line_size - 1) & ~(opal_cache_line_size - 1); /* Send frags freelists */ OBJ_CONSTRUCT(&module->small_send_frags, opal_free_list_t); rc = usnic_compat_free_list_init(&module->small_send_frags, - sizeof(opal_btl_usnic_small_send_frag_t) + - mca_btl_usnic_component.prefix_send_offset, + sizeof(opal_btl_usnic_small_send_frag_t), opal_cache_line_size, OBJ_CLASS(opal_btl_usnic_small_send_frag_t), segsize, @@ -2393,8 +2393,7 @@ static void init_freelists(opal_btl_usnic_module_t *module) OBJ_CONSTRUCT(&module->large_send_frags, opal_free_list_t); rc = usnic_compat_free_list_init(&module->large_send_frags, - sizeof(opal_btl_usnic_large_send_frag_t) + - mca_btl_usnic_component.prefix_send_offset, + sizeof(opal_btl_usnic_large_send_frag_t), opal_cache_line_size, OBJ_CLASS(opal_btl_usnic_large_send_frag_t), 0, /* payload size */ @@ -2411,8 +2410,7 @@ static void init_freelists(opal_btl_usnic_module_t *module) OBJ_CONSTRUCT(&module->put_dest_frags, opal_free_list_t); rc = usnic_compat_free_list_init(&module->put_dest_frags, - sizeof(opal_btl_usnic_put_dest_frag_t) + - mca_btl_usnic_component.prefix_send_offset, + sizeof(opal_btl_usnic_put_dest_frag_t), opal_cache_line_size, OBJ_CLASS(opal_btl_usnic_put_dest_frag_t), 0, /* payload size */ @@ -2430,8 +2428,7 @@ static void init_freelists(opal_btl_usnic_module_t *module) /* list of segments to use for sending */ OBJ_CONSTRUCT(&module->chunk_segs, opal_free_list_t); rc = usnic_compat_free_list_init(&module->chunk_segs, - sizeof(opal_btl_usnic_chunk_segment_t) + - mca_btl_usnic_component.prefix_send_offset, + sizeof(opal_btl_usnic_chunk_segment_t), opal_cache_line_size, OBJ_CLASS(opal_btl_usnic_chunk_segment_t), segsize, @@ -2449,11 +2446,11 @@ static void init_freelists(opal_btl_usnic_module_t *module) /* ACK segments freelist */ uint32_t ack_segment_len; ack_segment_len = (sizeof(opal_btl_usnic_btl_header_t) + + mca_btl_usnic_component.prefix_send_offset + opal_cache_line_size - 1) & ~(opal_cache_line_size - 1); OBJ_CONSTRUCT(&module->ack_segs, opal_free_list_t); rc = usnic_compat_free_list_init(&module->ack_segs, - sizeof(opal_btl_usnic_ack_segment_t) + - mca_btl_usnic_component.prefix_send_offset, + sizeof(opal_btl_usnic_ack_segment_t), opal_cache_line_size, OBJ_CLASS(opal_btl_usnic_ack_segment_t), ack_segment_len, From 8a4ccc3d5de2054fd4d37a11b30350ac150b24b0 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Tue, 8 Oct 2019 11:17:30 -0700 Subject: [PATCH 6/7] btl/usnic: set ack_iteration_delay default to 4 It was previously accidentally set to 0. Signed-off-by: Jeff Squyres (cherry picked from commit 132e4cab3bc71df0da87368a332d6af0090a6977) --- opal/mca/btl/usnic/btl_usnic_mca.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opal/mca/btl/usnic/btl_usnic_mca.c b/opal/mca/btl/usnic/btl_usnic_mca.c index f0399cbeb8c..e9f8fd294ad 100644 --- a/opal/mca/btl/usnic/btl_usnic_mca.c +++ b/opal/mca/btl/usnic/btl_usnic_mca.c @@ -265,7 +265,7 @@ int opal_btl_usnic_component_register(void) REGINT_GE_ONE, OPAL_INFO_LVL_5)); CHECK(reg_int("ack_iteration_delay", "Minimum number of times through usNIC \"progress\" function before checking to see if standalone ACKs need to be sent", - 0, &mca_btl_usnic_component.ack_iteration_delay, + 4, &mca_btl_usnic_component.ack_iteration_delay, REGINT_GE_ZERO, OPAL_INFO_LVL_5)); CHECK(reg_int("priority_limit", "Max size of \"priority\" messages (0 = use pre-set defaults; depends on number and type of devices available)", From 3d5c52728a12401386f303bc4bbd4f6edb44dcbc Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Tue, 8 Oct 2019 11:17:54 -0700 Subject: [PATCH 7/7] btl/usnic: set retrans_timeout back down to 5ms Signed-off-by: Jeff Squyres (cherry picked from commit 3080033a8c4db64199b03b6058e18488f619088c) --- opal/mca/btl/usnic/btl_usnic_mca.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opal/mca/btl/usnic/btl_usnic_mca.c b/opal/mca/btl/usnic/btl_usnic_mca.c index e9f8fd294ad..b3e130850c2 100644 --- a/opal/mca/btl/usnic/btl_usnic_mca.c +++ b/opal/mca/btl/usnic/btl_usnic_mca.c @@ -257,7 +257,7 @@ int opal_btl_usnic_component_register(void) mca_btl_usnic_component.udp_port_base = (int) udp_port_base; CHECK(reg_int("retrans_timeout", "Number of microseconds before retransmitting a frame", - 100000, &mca_btl_usnic_component.retrans_timeout, + 5000, &mca_btl_usnic_component.retrans_timeout, REGINT_GE_ONE, OPAL_INFO_LVL_5)); CHECK(reg_int("max_resends_per_iteration", "Maximum number of frames to resend in a single iteration through usNIC component progress",