Skip to content

Commit aca61a6

Browse files
yosefeyosefe
authored
Merge pull request #5238 from hoopoepg/topic/fixed-coverity-issues-ucx-pml
UCX/PML: fixed few coverity issues
2 parents 4c23068 + 502d04b commit aca61a6

File tree

5 files changed

+69
-71
lines changed

5 files changed

+69
-71
lines changed

‎ompi/mca/osc/ucx/osc_ucx_comm.c

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ static inline int get_dynamic_win_info(uint64_t remote_addr, ompi_osc_ucx_module
338338

339339
if ((module->win_info_array[target]).rkey_init == true) {
340340
ucp_rkey_destroy((module->win_info_array[target]).rkey);
341-
(module->win_info_array[target]).rkey_init == false;
341+
(module->win_info_array[target]).rkey_init = false;
342342
}
343343

344344
status = ucp_get_nbi(ep, (void *)temp_buf, len, remote_state_addr, state_rkey);
@@ -523,6 +523,7 @@ int ompi_osc_ucx_accumulate(const void *origin_addr, int origin_count,
523523
return ret;
524524
}
525525
} else {
526+
void *temp_addr_holder = NULL;
526527
void *temp_addr = NULL;
527528
uint32_t temp_count;
528529
ompi_datatype_t *temp_dt;
@@ -540,7 +541,7 @@ int ompi_osc_ucx_accumulate(const void *origin_addr, int origin_count,
540541
}
541542
}
542543
ompi_datatype_get_true_extent(temp_dt, &temp_lb, &temp_extent);
543-
temp_addr = malloc(temp_extent * temp_count);
544+
temp_addr = temp_addr_holder = malloc(temp_extent * temp_count);
544545
if (temp_addr == NULL) {
545546
return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
546547
}
@@ -616,7 +617,7 @@ int ompi_osc_ucx_accumulate(const void *origin_addr, int origin_count,
616617
return OMPI_ERROR;
617618
}
618619

619-
free(temp_addr);
620+
free(temp_addr_holder);
620621
}
621622

622623
ret = end_atomicity(module, ep, target);
@@ -774,6 +775,7 @@ int ompi_osc_ucx_get_accumulate(const void *origin_addr, int origin_count,
774775
return ret;
775776
}
776777
} else {
778+
void *temp_addr_holder = NULL;
777779
void *temp_addr = NULL;
778780
uint32_t temp_count;
779781
ompi_datatype_t *temp_dt;
@@ -791,7 +793,7 @@ int ompi_osc_ucx_get_accumulate(const void *origin_addr, int origin_count,
791793
}
792794
}
793795
ompi_datatype_get_true_extent(temp_dt, &temp_lb, &temp_extent);
794-
temp_addr = malloc(temp_extent * temp_count);
796+
temp_addr = temp_addr_holder = malloc(temp_extent * temp_count);
795797
if (temp_addr == NULL) {
796798
return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
797799
}
@@ -866,7 +868,7 @@ int ompi_osc_ucx_get_accumulate(const void *origin_addr, int origin_count,
866868
return OMPI_ERROR;
867869
}
868870

869-
free(temp_addr);
871+
free(temp_addr_holder);
870872
}
871873
}
872874

@@ -904,9 +906,7 @@ int ompi_osc_ucx_rput(const void *origin_addr, int origin_count,
904906
rkey = (module->win_info_array[target]).rkey;
905907

906908
OMPI_OSC_UCX_REQUEST_ALLOC(win, ucx_req);
907-
if (NULL == ucx_req) {
908-
return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
909-
}
909+
assert(NULL != ucx_req);
910910

911911
ret = ompi_osc_ucx_put(origin_addr, origin_count, origin_dt, target, target_disp,
912912
target_count, target_dt, win);
@@ -967,9 +967,7 @@ int ompi_osc_ucx_rget(void *origin_addr, int origin_count,
967967
rkey = (module->win_info_array[target]).rkey;
968968

969969
OMPI_OSC_UCX_REQUEST_ALLOC(win, ucx_req);
970-
if (NULL == ucx_req) {
971-
return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
972-
}
970+
assert(NULL != ucx_req);
973971

974972
ret = ompi_osc_ucx_get(origin_addr, origin_count, origin_dt, target, target_disp,
975973
target_count, target_dt, win);
@@ -1016,9 +1014,7 @@ int ompi_osc_ucx_raccumulate(const void *origin_addr, int origin_count,
10161014
}
10171015

10181016
OMPI_OSC_UCX_REQUEST_ALLOC(win, ucx_req);
1019-
if (NULL == ucx_req) {
1020-
return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
1021-
}
1017+
assert(NULL != ucx_req);
10221018

10231019
ret = ompi_osc_ucx_accumulate(origin_addr, origin_count, origin_dt, target, target_disp,
10241020
target_count, target_dt, op, win);
@@ -1050,9 +1046,7 @@ int ompi_osc_ucx_rget_accumulate(const void *origin_addr, int origin_count,
10501046
}
10511047

10521048
OMPI_OSC_UCX_REQUEST_ALLOC(win, ucx_req);
1053-
if (NULL == ucx_req) {
1054-
return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
1055-
}
1049+
assert(NULL != ucx_req);
10561050

10571051
ret = ompi_osc_ucx_get_accumulate(origin_addr, origin_count, origin_datatype,
10581052
result_addr, result_count, result_datatype,

‎ompi/mca/osc/ucx/osc_ucx_component.c

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ static int component_init(bool enable_progress_threads, bool enable_mpi_threads)
149149

150150
/* initialize UCP context */
151151

152-
memset(&context_params, 0, sizeof(ucp_context_h));
152+
memset(&context_params, 0, sizeof(context_params));
153153
context_params.field_mask = UCP_PARAM_FIELD_FEATURES |
154154
UCP_PARAM_FIELD_MT_WORKERS_SHARED |
155155
UCP_PARAM_FIELD_ESTIMATED_NUM_EPS |
@@ -329,7 +329,7 @@ static int component_select(struct ompi_win_t *win, void **base, size_t size, in
329329
ucp_worker_params_t worker_params;
330330
ucp_worker_attr_t worker_attr;
331331

332-
memset(&worker_params, 0, sizeof(ucp_worker_h));
332+
memset(&worker_params, 0, sizeof(worker_params));
333333
worker_params.field_mask = UCP_WORKER_PARAM_FIELD_THREAD_MODE;
334334
worker_params.thread_mode = (mca_osc_ucx_component.enable_mpi_threads == true)
335335
? UCS_THREAD_MODE_MULTI : UCS_THREAD_MODE_SINGLE;
@@ -340,7 +340,7 @@ static int component_select(struct ompi_win_t *win, void **base, size_t size, in
340340
"%s:%d: ucp_worker_create failed: %d\n",
341341
__FILE__, __LINE__, status);
342342
ret = OMPI_ERROR;
343-
goto error;
343+
goto error_nomem;
344344
}
345345

346346
ret = opal_progress_register(progress_callback);
@@ -360,7 +360,7 @@ static int component_select(struct ompi_win_t *win, void **base, size_t size, in
360360
"%s:%d: ucp_worker_query failed: %d\n",
361361
__FILE__, __LINE__, status);
362362
ret = OMPI_ERROR;
363-
goto error;
363+
goto error_nomem;
364364
}
365365

366366
if (mca_osc_ucx_component.enable_mpi_threads == true &&
@@ -369,7 +369,7 @@ static int component_select(struct ompi_win_t *win, void **base, size_t size, in
369369
"%s:%d: ucx does not support multithreading\n",
370370
__FILE__, __LINE__);
371371
ret = OMPI_ERROR;
372-
goto error;
372+
goto error_nomem;
373373
}
374374

375375
worker_created = true;
@@ -379,7 +379,7 @@ static int component_select(struct ompi_win_t *win, void **base, size_t size, in
379379
module = (ompi_osc_ucx_module_t *)calloc(1, sizeof(ompi_osc_ucx_module_t));
380380
if (module == NULL) {
381381
ret = OMPI_ERR_TEMP_OUT_OF_RESOURCE;
382-
goto error;
382+
goto error_nomem;
383383
}
384384

385385
/* fill in the function pointer part */
@@ -676,8 +676,10 @@ static int component_select(struct ompi_win_t *win, void **base, size_t size, in
676676
}
677677
}
678678
if (progress_registered) opal_progress_unregister(progress_callback);
679-
if (worker_created) ucp_worker_destroy(mca_osc_ucx_component.ucp_worker);
680679
if (module) free(module);
680+
681+
error_nomem:
682+
if (worker_created) ucp_worker_destroy(mca_osc_ucx_component.ucp_worker);
681683
return ret;
682684
}
683685

@@ -777,6 +779,11 @@ int ompi_osc_ucx_win_detach(struct ompi_win_t *win, const void *base) {
777779
(uint64_t)base, 1, &insert);
778780
assert(contain >= 0 && contain < module->state.dynamic_win_count);
779781

782+
/* if we can't find region - just exit */
783+
if (contain < 0) {
784+
return OMPI_SUCCESS;
785+
}
786+
780787
module->local_dynamic_win_info[contain].refcnt--;
781788
if (module->local_dynamic_win_info[contain].refcnt == 0) {
782789
ucp_mem_unmap(mca_osc_ucx_component.ucp_context,
@@ -796,16 +803,7 @@ int ompi_osc_ucx_win_detach(struct ompi_win_t *win, const void *base) {
796803

797804
int ompi_osc_ucx_free(struct ompi_win_t *win) {
798805
ompi_osc_ucx_module_t *module = (ompi_osc_ucx_module_t*) win->w_osc_module;
799-
int i, ret = OMPI_SUCCESS;
800-
801-
if ((module->epoch_type.access != NONE_EPOCH && module->epoch_type.access != FENCE_EPOCH)
802-
|| module->epoch_type.exposure != NONE_EPOCH) {
803-
ret = OMPI_ERR_RMA_SYNC;
804-
}
805-
806-
if (module->start_group != NULL || module->post_group != NULL) {
807-
ret = OMPI_ERR_RMA_SYNC;
808-
}
806+
int i, ret;
809807

810808
assert(module->global_ops_num == 0);
811809
assert(module->lock_count == 0);
@@ -824,7 +822,7 @@ int ompi_osc_ucx_free(struct ompi_win_t *win) {
824822
for (i = 0; i < ompi_comm_size(module->comm); i++) {
825823
if ((module->win_info_array[i]).rkey_init == true) {
826824
ucp_rkey_destroy((module->win_info_array[i]).rkey);
827-
(module->win_info_array[i]).rkey_init == false;
825+
(module->win_info_array[i]).rkey_init = false;
828826
}
829827
ucp_rkey_destroy((module->state_info_array[i]).rkey);
830828
}

‎ompi/mca/osc/ucx/osc_ucx_passive_target.c

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ static inline int start_shared(ompi_osc_ucx_module_t *module, int target) {
3535
__FILE__, __LINE__, status);
3636
return OMPI_ERROR;
3737
}
38-
assert(result_value >= 0);
38+
assert((int64_t)result_value >= 0);
3939
if (result_value >= TARGET_LOCK_EXCLUSIVE) {
4040
status = ucp_atomic_post(ep, UCP_ATOMIC_POST_OP_ADD, (-1), sizeof(uint64_t),
4141
remote_addr, rkey);
@@ -245,12 +245,8 @@ int ompi_osc_ucx_lock_all(int assert, struct ompi_win_t *win) {
245245
} else {
246246
module->lock_all_is_nocheck = true;
247247
}
248-
249-
if (ret != OMPI_SUCCESS) {
250-
module->epoch_type.access = NONE_EPOCH;
251-
}
252-
253-
return ret;
248+
assert(OMPI_SUCCESS == ret);
249+
return OMPI_SUCCESS;
254250
}
255251

256252
int ompi_osc_ucx_unlock_all(struct ompi_win_t *win) {

‎ompi/mca/pml/ucx/pml_ucx.c

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -367,12 +367,12 @@ static inline ucp_ep_h mca_pml_ucx_get_ep(ompi_communicator_t *comm, int rank)
367367
return NULL;
368368
}
369369

370-
static void mca_pml_ucx_waitall(void **reqs, size_t *count_p)
370+
static void mca_pml_ucx_waitall(void **reqs, int *count_p)
371371
{
372372
ucs_status_t status;
373-
size_t i;
373+
int i;
374374

375-
PML_UCX_VERBOSE(2, "waiting for %d disconnect requests", (int)*count_p);
375+
PML_UCX_VERBOSE(2, "waiting for %d disconnect requests", *count_p);
376376
for (i = 0; i < *count_p; ++i) {
377377
do {
378378
opal_progress();
@@ -398,7 +398,8 @@ int mca_pml_ucx_del_procs(struct ompi_proc_t **procs, size_t nprocs)
398398
{
399399
volatile int fenced = 0;
400400
ompi_proc_t *proc;
401-
size_t num_reqs, max_reqs;
401+
int num_reqs;
402+
size_t max_reqs;
402403
void *dreq, **dreqs;
403404
ucp_ep_h ep;
404405
size_t i;
@@ -422,25 +423,27 @@ int mca_pml_ucx_del_procs(struct ompi_proc_t **procs, size_t nprocs)
422423
continue;
423424
}
424425

426+
proc->proc_endpoints[OMPI_PROC_ENDPOINT_TAG_PML] = NULL;
427+
425428
PML_UCX_VERBOSE(2, "disconnecting from rank %d", proc->super.proc_name.vpid);
426429
dreq = ucp_disconnect_nb(ep);
427430
if (dreq != NULL) {
428431
if (UCS_PTR_IS_ERR(dreq)) {
429432
PML_UCX_ERROR("ucp_disconnect_nb(%d) failed: %s",
430433
proc->super.proc_name.vpid,
431434
ucs_status_string(UCS_PTR_STATUS(dreq)));
435+
continue;
432436
} else {
433437
dreqs[num_reqs++] = dreq;
438+
if (num_reqs >= ompi_pml_ucx.num_disconnect) {
439+
mca_pml_ucx_waitall(dreqs, &num_reqs);
440+
}
434441
}
435442
}
436-
437-
proc->proc_endpoints[OMPI_PROC_ENDPOINT_TAG_PML] = NULL;
438-
439-
if ((int)num_reqs >= ompi_pml_ucx.num_disconnect) {
440-
mca_pml_ucx_waitall(dreqs, &num_reqs);
441-
}
442443
}
443-
444+
/* num_reqs == 0 is processed by mca_pml_ucx_waitall routine,
445+
* so suppress coverity warning */
446+
/* coverity[uninit_use_in_call] */
444447
mca_pml_ucx_waitall(dreqs, &num_reqs);
445448
free(dreqs);
446449

@@ -541,6 +544,7 @@ int mca_pml_ucx_recv(void *buf, size_t count, ompi_datatype_t *datatype, int src
541544

542545
PML_UCX_TRACE_RECV("%s", buf, count, datatype, src, tag, comm, "recv");
543546

547+
/* coverity[bad_alloc_arithmetic] */
544548
PML_UCX_MAKE_RECV_TAG(ucp_tag, ucp_tag_mask, tag, src, comm);
545549
req = PML_UCX_REQ_ALLOCA();
546550
status = ucp_tag_recv_nbr(ompi_pml_ucx.ucp_worker, buf, count,
@@ -765,6 +769,7 @@ mca_pml_ucx_send_nbr(ucp_ep_h ep, const void *buf, size_t count,
765769
void *req;
766770
ucs_status_t status;
767771

772+
/* coverity[bad_alloc_arithmetic] */
768773
req = PML_UCX_REQ_ALLOCA();
769774
status = ucp_tag_send_nbr(ep, buf, count, ucx_datatype, tag, req);
770775
if (OPAL_LIKELY(status == UCS_OK)) {

0 commit comments

Comments
 (0)