Skip to content

Commit

Permalink
Fix core dump when performing misaligned atomics.
Browse files Browse the repository at this point in the history 
c_reqops MTT test has a weird alignment when creating the window,
which is perfectly valid. This will cause a segv
when performing atomic operations. While this was found on
Power, it probably is not limited to it.

Detect a misalignment, and fallback to a lock + copy method.

If it's detected that the window alignment doesn't match
the type, abandon the atomics and use a lock implementation
for all atomic operations in that window.

Signed-off-by: Austen Lauria <[email protected]>
  • Loading branch information
@awlauria
awlauria committed Mar 11, 2021
1 parent 57026cf commit 587f4a9
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 20 deletions.
3 changes: 3 additions & 0 deletions ompi/mca/osc/rdma/osc_rdma.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* Copyright (c) 2019 Triad National Security, LLC. All rights
* reserved.
* Copyright (c) 2020-2021 Google, LLC. All rights reserved.
* Copyright (c) 2021 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -158,6 +159,8 @@ struct ompi_osc_rdma_module_t {

bool acc_use_amo;

bool acc_use_lock_fallback;

/** whether the group is located on a single node */
bool single_node;

Expand Down
36 changes: 27 additions & 9 deletions ompi/mca/osc/rdma/osc_rdma_accumulate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* Copyright (c) 2019 Triad National Security, LLC. All rights
* reserved.
* Copyright (c) 2019-2021 Google, LLC. All rights reserved.
* Copyright (c) 2021 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -133,6 +134,28 @@ static int ompi_osc_rdma_op_mapping[OMPI_OP_NUM_OF_TYPES + 1] = {
[OMPI_OP_REPLACE] = MCA_BTL_ATOMIC_SWAP,
};

/* set the appropriate flags for this atomic */
static inline int ompi_osc_rdma_set_btl_flags(ompi_osc_rdma_module_t *module, ompi_datatype_t *dt, ptrdiff_t extent) {

int flags = 0;

if(4 == extent) {
flags = MCA_BTL_ATOMIC_FLAG_32BIT;

}

if(OPAL_UNLIKELY((true == module->acc_use_lock_fallback) || (module->disp_unit % extent))) {
flags |= MCA_BTL_ATOMIC_FLAG_USE_LOCK_FALLBACK;
module->acc_use_lock_fallback = true;
}

if (OMPI_DATATYPE_FLAG_DATA_FLOAT & dt->super.flags) {
flags |= MCA_BTL_ATOMIC_FLAG_FLOAT;
}

return flags;
}

static int ompi_osc_rdma_fetch_and_op_atomic (ompi_osc_rdma_sync_t *sync, const void *origin_addr, void *result_addr, ompi_datatype_t *dt,
ptrdiff_t extent, ompi_osc_rdma_peer_t *peer, uint64_t target_address,
mca_btl_base_registration_handle_t *target_handle, ompi_op_t *op, ompi_osc_rdma_request_t *req)
Expand All @@ -151,10 +174,7 @@ static int ompi_osc_rdma_fetch_and_op_atomic (ompi_osc_rdma_sync_t *sync, const

btl_op = ompi_osc_rdma_op_mapping[op->op_type];

flags = (4 == extent) ? MCA_BTL_ATOMIC_FLAG_32BIT : 0;
if (OMPI_DATATYPE_FLAG_DATA_FLOAT & dt->super.flags) {
flags |= MCA_BTL_ATOMIC_FLAG_FLOAT;
}
flags = ompi_osc_rdma_set_btl_flags(module, dt, extent);

OSC_RDMA_VERBOSE(MCA_BASE_VERBOSE_TRACE, "initiating fetch-and-op using %d-bit btl atomics. origin: 0x%" PRIx64,
(4 == extent) ? 32 : 64, *((int64_t *) origin_addr));
Expand Down Expand Up @@ -239,10 +259,7 @@ static int ompi_osc_rdma_acc_single_atomic (ompi_osc_rdma_sync_t *sync, const vo
origin = (8 == extent) ? ((uint64_t *) origin_addr)[0] : ((uint32_t *) origin_addr)[0];

/* set the appropriate flags for this atomic */
flags = (4 == extent) ? MCA_BTL_ATOMIC_FLAG_32BIT : 0;
if (OMPI_DATATYPE_FLAG_DATA_FLOAT & dt->super.flags) {
flags |= MCA_BTL_ATOMIC_FLAG_FLOAT;
}
flags = ompi_osc_rdma_set_btl_flags(module, dt, extent);

btl_op = ompi_osc_rdma_op_mapping[op->op_type];

Expand Down Expand Up @@ -659,7 +676,8 @@ static inline int ompi_osc_rdma_cas_atomic (ompi_osc_rdma_sync_t *sync, const vo

compare = (8 == size) ? ((int64_t *) compare_addr)[0] : ((int32_t *) compare_addr)[0];
source = (8 == size) ? ((int64_t *) source_addr)[0] : ((int32_t *) source_addr)[0];
flags = (4 == size) ? MCA_BTL_ATOMIC_FLAG_32BIT : 0;

flags = ompi_osc_rdma_set_btl_flags(module, datatype, size);

OSC_RDMA_VERBOSE(MCA_BASE_VERBOSE_TRACE, "initiating compare-and-swap using %d-bit btl atomics. compare: 0x%"
PRIx64 ", origin: 0x%" PRIx64, (int) size * 8, *((int64_t *) compare_addr), *((int64_t *) source_addr));
Expand Down
3 changes: 2 additions & 1 deletion ompi/mca/osc/rdma/osc_rdma_component.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* Copyright (c) 2012-2015 Sandia National Laboratories. All rights reserved.
* Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
* Copyright (c) 2015-2017 Intel, Inc. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* Copyright (c) 2016-2021 IBM Corporation. All rights reserved.
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2018 Amazon.com, Inc. or its affiliates. All Rights reserved.
* Copyright (c) 2019 Research Organization for Information Science
Expand Down Expand Up @@ -1333,6 +1333,7 @@ static int ompi_osc_rdma_component_select (struct ompi_win_t *win, void **base,
module->locking_mode = mca_osc_rdma_component.locking_mode;
module->acc_single_intrinsic = check_config_value_bool ("acc_single_intrinsic", info);
module->acc_use_amo = mca_osc_rdma_component.acc_use_amo;
module->acc_use_lock_fallback = false;
module->network_amo_max_count = mca_osc_rdma_component.network_amo_max_count;

module->all_sync.module = module;
Expand Down
3 changes: 3 additions & 0 deletions opal/mca/btl/base/base.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Copyright (c) 2006 Sun Microsystems, Inc. All rights reserved.
* Copyright (c) 2008 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2020 Google, LLC. All rights reserved.
* Copyright (c) 2021 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -71,6 +72,8 @@ OPAL_DECLSPEC int mca_btl_base_param_verify(mca_btl_base_module_t *module);
/*
* Globals
*/

extern opal_mutex_t mca_btl_atomic_fallback_lock;
extern char* mca_btl_base_include;
extern char* mca_btl_base_exclude;
extern int mca_btl_base_warn_component_unused;
Expand Down
126 changes: 118 additions & 8 deletions opal/mca/btl/base/btl_base_am_rdma.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright (c) 2011-2018 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2020-2021 Google, LLC. All rights reserved.
* Copyright (c) 2021 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -150,7 +151,10 @@ struct mca_btl_base_rdma_hdr_t {
/** operation size (bytes) */
uint8_t size;

uint8_t padding[2];
/** mis-aligned atomics case, fallback to a lock */
uint8_t use_lock;

uint8_t padding[1];

/** atomic operands */
int64_t operand[2];
Expand Down Expand Up @@ -506,6 +510,7 @@ mca_btl_base_rdma_start (mca_btl_base_module_t *btl, struct mca_btl_base_endpoin
hdr->data.rdma.use_rdma = use_rdma;
} else {
hdr->data.atomic.op = op;
hdr->data.atomic.use_lock = (flags & MCA_BTL_ATOMIC_FLAG_USE_LOCK_FALLBACK);
hdr->data.atomic.operand[0] = operand1;
hdr->data.atomic.operand[1] = operand2;
}
Expand Down Expand Up @@ -794,6 +799,53 @@ static int mca_btl_base_am_rdma_progress (void)
}));
}

static int mca_btl_base_am_atomic_lock_64 (int64_t *operand, opal_atomic_int64_t *addr,
mca_btl_base_atomic_op_t op) {

int64_t result = 0;

OPAL_THREAD_LOCK(&mca_btl_atomic_fallback_lock);
result = *addr;

switch (op) {
case MCA_BTL_ATOMIC_ADD:
*addr += *operand;
break;
case MCA_BTL_ATOMIC_AND:
*addr &= *operand;
break;
case MCA_BTL_ATOMIC_OR:
*addr |= *operand;
break;
case MCA_BTL_ATOMIC_XOR:
*addr ^= *operand;
break;
case MCA_BTL_ATOMIC_SWAP:
*addr = *operand;
break;
case MCA_BTL_ATOMIC_MIN:
if(result > *operand) {
*addr = *operand;
}
break;
case MCA_BTL_ATOMIC_MAX:
if(result < *operand) {
*addr = *operand;
}
break;
default: {
OPAL_THREAD_UNLOCK(&mca_btl_atomic_fallback_lock);
return OPAL_ERR_BAD_PARAM;
} // End default
} // End switch

*operand = result;
OPAL_THREAD_UNLOCK(&mca_btl_atomic_fallback_lock);

return OPAL_SUCCESS;

}

static int mca_btl_base_am_atomic_64 (int64_t *operand, opal_atomic_int64_t *addr,
mca_btl_base_atomic_op_t op)
{
Expand Down Expand Up @@ -829,8 +881,53 @@ static int mca_btl_base_am_atomic_64 (int64_t *operand, opal_atomic_int64_t *add
return OPAL_SUCCESS;
}

static int mca_btl_base_am_atomic_32 (int32_t *operand, opal_atomic_int32_t *addr,
mca_btl_base_atomic_op_t op)
static int mca_btl_base_am_atomic_lock_32 (int32_t *operand, opal_atomic_int32_t *addr,
mca_btl_base_atomic_op_t op) {

int32_t result = 0;

OPAL_THREAD_LOCK(&mca_btl_atomic_fallback_lock);
result = *addr;

switch (op) {
case MCA_BTL_ATOMIC_ADD:
*addr += *operand;
break;
case MCA_BTL_ATOMIC_AND:
*addr &= *operand;
break;
case MCA_BTL_ATOMIC_OR:
*addr |= *operand;
break;
case MCA_BTL_ATOMIC_XOR:
*addr ^= *operand;
break;
case MCA_BTL_ATOMIC_SWAP:
*addr = *operand;
break;
case MCA_BTL_ATOMIC_MIN:
if(result > *operand) {
*addr = *operand;
}
break;
case MCA_BTL_ATOMIC_MAX:
if(result < *operand) {
*addr = *operand;
}
break;
default: {
OPAL_THREAD_UNLOCK(&mca_btl_atomic_fallback_lock);
return OPAL_ERR_BAD_PARAM;
} // End default
} // End switch

*operand = result;
OPAL_THREAD_UNLOCK(&mca_btl_atomic_fallback_lock);
return OPAL_SUCCESS;

}

static int mca_btl_base_am_atomic_32 (int32_t *operand, opal_atomic_int32_t *addr, mca_btl_base_atomic_op_t op)
{
int32_t result = 0;

Expand Down Expand Up @@ -969,14 +1066,27 @@ static void mca_btl_base_am_process_atomic (mca_btl_base_module_t *btl,
case MCA_BTL_BASE_AM_ATOMIC:
if (4 == hdr->data.atomic.size) {
uint32_t tmp = (uint32_t)atomic_response;
mca_btl_base_am_atomic_32 (&tmp, (opal_atomic_int32_t *)(uintptr_t)hdr->target_address,
hdr->data.atomic.op);
if(OPAL_LIKELY(0 == hdr->data.atomic.use_lock)) {
mca_btl_base_am_atomic_32 (&tmp, (opal_atomic_int32_t *)(uintptr_t)hdr->target_address,
hdr->data.atomic.op);
}
else {
mca_btl_base_am_atomic_lock_32 (&tmp, (opal_atomic_int32_t *)(uintptr_t)hdr->target_address,
hdr->data.atomic.op);
}
atomic_response = tmp;
}
if (8 == hdr->data.atomic.size) {
mca_btl_base_am_atomic_64 ((int64_t *)hdr->target_address,
(opal_atomic_int64_t *)(uintptr_t)hdr->target_address,
hdr->data.atomic.op);
if(OPAL_LIKELY(0 == hdr->data.atomic.use_lock)) {
mca_btl_base_am_atomic_64 ((int64_t *)hdr->target_address,
(opal_atomic_int64_t *)(uintptr_t)hdr->target_address,
hdr->data.atomic.op);
}
else {
mca_btl_base_am_atomic_lock_64 ((int64_t *)hdr->target_address,
(opal_atomic_int64_t *)(uintptr_t)hdr->target_address,
hdr->data.atomic.op);
}
}
break;
case MCA_BTL_BASE_AM_CAS:
Expand Down
5 changes: 5 additions & 0 deletions opal/mca/btl/base/btl_base_frame.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* Copyright (c) 2016-2018 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2020 Google, LLC. All rights reserved.
* Copyright (c) 2021 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -121,6 +122,7 @@ char* mca_btl_base_exclude = NULL;
int mca_btl_base_warn_component_unused = 1;
int mca_btl_base_warn_peer_error = true;
opal_list_t mca_btl_base_modules_initialized = {{0}};
opal_mutex_t mca_btl_atomic_fallback_lock;
bool mca_btl_base_thread_multiple_override = false;

static int mca_btl_base_register(mca_base_register_flag_t flags)
Expand Down Expand Up @@ -216,6 +218,8 @@ static int mca_btl_base_open(mca_base_open_flag_t flags)
/* get the verbosity so that BTL_VERBOSE will work */
mca_btl_base_verbose = opal_output_get_verbosity(opal_btl_base_framework.framework_output);

OBJ_CONSTRUCT(&mca_btl_atomic_fallback_lock, opal_list_t);

/* All done */
return OPAL_SUCCESS;
}
Expand Down Expand Up @@ -243,6 +247,7 @@ static int mca_btl_base_close(void)
(void) mca_base_framework_components_close(&opal_btl_base_framework, NULL);

OBJ_DESTRUCT(&mca_btl_base_modules_initialized);
OBJ_DESTRUCT(&mca_btl_atomic_fallback_lock);

#if 0
/* restore event processing */
Expand Down
6 changes: 4 additions & 2 deletions opal/mca/btl/btl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* and Technology (RIST). All rights reserved.
* Copyright (c) 2020 Intel, Inc. All rights reserved.
* Copyright (c) 2020-2021 Google, LLC. All rights reserved.
* Copyright (c) 2021 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -345,9 +346,10 @@ enum {

enum {
/** Use 32-bit atomics */
MCA_BTL_ATOMIC_FLAG_32BIT = 0x00000001,
MCA_BTL_ATOMIC_FLAG_32BIT = 0x00000001,
/** Use floating-point atomics */
MCA_BTL_ATOMIC_FLAG_FLOAT = 0x00000002,
MCA_BTL_ATOMIC_FLAG_FLOAT = 0x00000002,
MCA_BTL_ATOMIC_FLAG_USE_LOCK_FALLBACK = 0x00000004,
};

enum mca_btl_base_atomic_op_t {
Expand Down

0 comments on commit 587f4a9

Please sign in to comment.