Skip to content

Commit 6dc87ad

Browse files
bosilcabosilca
committed
Fixing the partial pack unpack issue.
When unpacking a partial predefined element check the boundaries of the description vector type, and adjust the memory pointer accordingly (to reflect not only when a single basic type was correctly unpacked, but also when an entire blocklen has been unpacked). Signed-off-by: George Bosilca <[email protected]>
1 parent ca65630 commit 6dc87ad

File tree

4 files changed

+289
-103
lines changed

4 files changed

+289
-103
lines changed

‎opal/datatype/opal_datatype_unpack.c

Lines changed: 64 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ int32_t opal_unpack_homogeneous_contig_function(opal_convertor_t *pConv, struct
153153
}
154154
}
155155
}
156-
*out_size = iov_idx; /* we only reach this line after the for loop succesfully complete */
156+
*out_size = iov_idx; /* we only reach this line after the for loop successfully complete */
157157
*max_data = pConv->bConverted - initial_bytes_converted;
158158
if (pConv->bConverted == pConv->local_size) {
159159
pConv->flags |= CONVERTOR_COMPLETED;
@@ -183,36 +183,38 @@ static inline void opal_unpack_partial_datatype(opal_convertor_t *pConvertor, dt
183183
unsigned char *user_data = *user_buffer + pElem->elem.disp;
184184
size_t count_desc = 1;
185185
size_t data_length = opal_datatype_basicDatatypes[pElem->elem.common.type]->size;
186-
187-
DO_DEBUG(opal_output(0,
188-
"unpack partial data start %lu end %lu data_length %lu user %p\n"
189-
"\tbConverted %lu total_length %lu count %ld\n",
190-
(unsigned long) start_position, (unsigned long) start_position + length,
191-
(unsigned long) data_length, (void *) *user_buffer,
192-
(unsigned long) pConvertor->bConverted,
193-
(unsigned long) pConvertor->local_size, pConvertor->count););
194-
195-
/* Find a byte that is not used in the partial buffer */
196-
find_unused_byte:
197-
for (size_t i = 0; i < length; i++) {
198-
if (unused_byte == partial_data[i]) {
186+
dt_elem_desc_t single_elem = { .elem = { .common = pElem->elem.common, .count = 1, .blocklen = 1,
187+
.extent = data_length, /* advance by a full data element */
188+
.disp = pElem->elem.disp } };
189+
DO_DEBUG( opal_output( 0, "unpack partial data start %lu end %lu data_length %lu user %p\n"
190+
"\tbConverted %lu total_length %lu count %ld\n",
191+
(unsigned long)start_position, (unsigned long)start_position + length,
192+
(unsigned long)data_length, (void*)*user_buffer,
193+
(unsigned long)pConvertor->bConverted,
194+
(unsigned long)pConvertor->local_size, pConvertor->count ); );
195+
196+
/* Find a byte value that is not used in the partial buffer. We use it as a marker
197+
* to identify what has not been modified by the unpack call. */
198+
find_unused_byte:
199+
for (size_t i = 0; i < length; i++ ) {
200+
if( unused_byte == partial_data[i] ) {
199201
unused_byte--;
200202
goto find_unused_byte;
201203
}
202204
}
203205

204-
/* Copy and fill the rest of the buffer with the unused byte */
205-
memset(temporary, unused_byte, data_length);
206-
MEMCPY(temporary + start_position, partial_data, length);
206+
/* Prepare an full element of the predefined type, by populating an entire type
207+
* with the unused byte and then put the partial data at the right position. */
208+
memset( temporary, unused_byte, data_length );
209+
MEMCPY( temporary + start_position, partial_data, length );
207210

211+
/* Save the original content of the user memory */
208212
#if OPAL_CUDA_SUPPORT
209213
/* In the case where the data is being unpacked from device memory, need to
210-
* use the special host to device memory copy. Note this code path was only
211-
* seen on large receives of noncontiguous data via buffered sends. */
212-
pConvertor->cbmemcpy(saved_data, user_data, data_length, pConvertor);
214+
* use the special host to device memory copy. */
215+
pConvertor->cbmemcpy(saved_data, user_data, data_length, pConvertor );
213216
#else
214-
/* Save the content of the user memory */
215-
MEMCPY(saved_data, user_data, data_length);
217+
MEMCPY( saved_data, user_data, data_length );
216218
#endif
217219

218220
/* Then unpack the data into the user memory */
@@ -222,14 +224,11 @@ static inline void opal_unpack_partial_datatype(opal_convertor_t *pConvertor, dt
222224
/* reload the length as it is reset by the macro */
223225
data_length = opal_datatype_basicDatatypes[pElem->elem.common.type]->size;
224226

225-
/* For every occurrence of the unused byte move data from the saved
226-
* buffer back into the user memory.
227-
*/
227+
/* Rebuild the data by pulling back the unmodified bytes from the original
228+
* content in the user memory. */
228229
#if OPAL_CUDA_SUPPORT
229230
/* Need to copy the modified user_data again so we can see which
230-
* bytes need to be converted back to their original values. Note
231-
* this code path was only seen on large receives of noncontiguous
232-
* data via buffered sends. */
231+
* bytes need to be converted back to their original values. */
233232
{
234233
char resaved_data[16];
235234
pConvertor->cbmemcpy(resaved_data, user_data, data_length, pConvertor);
@@ -271,9 +270,8 @@ int32_t opal_generic_simple_unpack_function(opal_convertor_t *pConvertor, struct
271270
size_t iov_len_local;
272271
uint32_t iov_count;
273272

274-
DO_DEBUG(opal_output(0, "opal_convertor_generic_simple_unpack( %p, {%p, %lu}, %u )\n",
275-
(void *) pConvertor, (void *) iov[0].iov_base,
276-
(unsigned long) iov[0].iov_len, *out_size););
273+
DO_DEBUG( opal_output( 0, "opal_convertor_generic_simple_unpack( %p, iov[%u] = {%p, %lu} )\n",
274+
(void*)pConvertor, *out_size, (void*)iov[0].iov_base, (unsigned long)iov[0].iov_len ); );
277275

278276
description = pConvertor->use_desc->desc;
279277

@@ -300,26 +298,37 @@ int32_t opal_generic_simple_unpack_function(opal_convertor_t *pConvertor, struct
300298
iov_ptr = (unsigned char *) iov[iov_count].iov_base;
301299
iov_len_local = iov[iov_count].iov_len;
302300

303-
if (0 != pConvertor->partial_length) {
304-
size_t element_length = opal_datatype_basicDatatypes[pElem->elem.common.type]->size;
305-
size_t missing_length = element_length - pConvertor->partial_length;
306-
307-
assert(pElem->elem.common.flags & OPAL_DATATYPE_FLAG_DATA);
308-
COMPUTE_CSUM(iov_ptr, missing_length, pConvertor);
309-
opal_unpack_partial_datatype(pConvertor, pElem, iov_ptr, pConvertor->partial_length,
310-
(size_t)(element_length - pConvertor->partial_length),
311-
&conv_ptr);
312-
--count_desc;
313-
if (0 == count_desc) {
314-
conv_ptr = pConvertor->pBaseBuf + pStack->disp;
315-
pos_desc++; /* advance to the next data */
316-
UPDATE_INTERNAL_COUNTERS(description, pos_desc, pElem, count_desc);
317-
}
318-
iov_ptr += missing_length;
319-
iov_len_local -= missing_length;
320-
pConvertor->partial_length = 0; /* nothing more inside */
321-
}
301+
/* Deal with all types of partial predefined datatype unpacking, including when
302+
* unpacking a partial predefined element and when unpacking a part smaller than
303+
* the blocklen.
304+
*/
322305
if (pElem->elem.common.flags & OPAL_DATATYPE_FLAG_DATA) {
306+
if (0 != pConvertor->partial_length) { /* partial predefined element */
307+
size_t element_length = opal_datatype_basicDatatypes[pElem->elem.common.type]->size;
308+
size_t missing_length = element_length - pConvertor->partial_length;
309+
310+
assert( pElem->elem.common.flags & OPAL_DATATYPE_FLAG_DATA );
311+
COMPUTE_CSUM( iov_ptr, missing_length, pConvertor );
312+
opal_unpack_partial_datatype( pConvertor, pElem,
313+
iov_ptr,
314+
pConvertor->partial_length, missing_length,
315+
&conv_ptr );
316+
--count_desc;
317+
if( 0 == (count_desc % pElem->elem.blocklen)) { /* did we reach the end of the blocklen ? */
318+
if (0 == count_desc) { /* the end of the vector ? */
319+
conv_ptr = pConvertor->pBaseBuf + pStack->disp;
320+
pos_desc++; /* advance to the next data */
321+
UPDATE_INTERNAL_COUNTERS(description, pos_desc, pElem, count_desc);
322+
} else {
323+
conv_ptr += pElem->elem.extent - (pElem->elem.blocklen * element_length);
324+
}
325+
}
326+
iov_ptr += missing_length;
327+
iov_len_local -= missing_length;
328+
pConvertor->partial_length = 0; /* nothing more inside */
329+
if( 0 == iov_len_local )
330+
goto complete_loop;
331+
}
323332
if (((size_t) pElem->elem.count * pElem->elem.blocklen) != count_desc) {
324333
/* we have a partial (less than blocklen) basic datatype */
325334
int rc = UNPACK_PARTIAL_BLOCKLEN(pConvertor, pElem, count_desc, iov_ptr, conv_ptr,
@@ -401,14 +410,14 @@ int32_t opal_generic_simple_unpack_function(opal_convertor_t *pConvertor, struct
401410
}
402411
}
403412
complete_loop:
404-
assert(pElem->elem.common.type < OPAL_DATATYPE_MAX_PREDEFINED);
405-
if ((pElem->elem.common.flags & OPAL_DATATYPE_FLAG_DATA) && (0 != iov_len_local)) {
406-
unsigned char *temp = conv_ptr;
413+
assert( pElem->elem.common.type < OPAL_DATATYPE_MAX_PREDEFINED );
414+
if( (pElem->elem.common.flags & OPAL_DATATYPE_FLAG_DATA) && (0 != iov_len_local) ) {
415+
unsigned char* temp = conv_ptr;
407416
/* We have some partial data here. Let's copy it into the convertor
408417
* and keep it hot until the next round.
409418
*/
410-
assert(iov_len_local < opal_datatype_basicDatatypes[pElem->elem.common.type]->size);
411-
COMPUTE_CSUM(iov_ptr, iov_len_local, pConvertor);
419+
assert( iov_len_local < opal_datatype_basicDatatypes[pElem->elem.common.type]->size );
420+
COMPUTE_CSUM( iov_ptr, iov_len_local, pConvertor );
412421

413422
opal_unpack_partial_datatype(pConvertor, pElem, iov_ptr, 0, iov_len_local, &temp);
414423

@@ -543,11 +552,6 @@ int32_t opal_unpack_general_function(opal_convertor_t *pConvertor, struct iovec
543552
unsigned char *conv_ptr, *iov_ptr;
544553
uint32_t iov_count;
545554
size_t iov_len_local;
546-
#if 0
547-
const opal_convertor_master_t *master = pConvertor->master;
548-
ptrdiff_t advance; /* number of bytes that we should advance the buffer */
549-
#endif
550-
size_t rc;
551555

552556
DO_DEBUG(opal_output(0, "opal_convertor_general_unpack( %p, {%p, %lu}, %d )\n",
553557
(void *) pConvertor, (void *) iov[0].iov_base,

‎opal/datatype/opal_datatype_unpack.h

Lines changed: 47 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,10 @@
2929
#endif
3030

3131
/**
32-
* This function deals only with partial elements. The COUNT points however to the whole leftover
33-
* count, but this function is only expected to operate on an amount less than blength, that would
34-
* allow the rest of the pack process to handle only entire blength blocks (plus the left over).
32+
* This function deals only with partial elements. The COUNT points however to
33+
* the whole leftover count, but this function is only expected to operate on
34+
* an amount less than blength, that would allow the rest of the pack process
35+
* to handle only entire blength blocks (plus the left over).
3536
*
3637
* Return 1 if we are now aligned on a block, 0 otherwise.
3738
*/
@@ -47,6 +48,8 @@ static inline int unpack_partial_blocklen(opal_convertor_t *CONVERTOR, const dt_
4748

4849
assert(*(COUNT) <= ((size_t)(_elem->count * _elem->blocklen)));
4950

51+
if( (*SPACE) < do_now_bytes ) /* Can we do anything ? */
52+
return 0;
5053
/**
5154
* First check if we already did something on this element ? The COUNT is the number
5255
* of remaining predefined types in the current elem, not how many predefined types
@@ -63,19 +66,18 @@ static inline int unpack_partial_blocklen(opal_convertor_t *CONVERTOR, const dt_
6366

6467
do_now_bytes *= do_now;
6568

66-
OPAL_DATATYPE_SAFEGUARD_POINTER(_memory, do_now_bytes, (CONVERTOR)->pBaseBuf,
67-
(CONVERTOR)->pDesc, (CONVERTOR)->count);
68-
DO_DEBUG(opal_output(0, "unpack memcpy( %p, %p, %lu ) => space %lu [prolog]\n",
69-
(void *) _memory, (void *) _packed, (unsigned long) do_now_bytes,
70-
(unsigned long) (*(SPACE))););
71-
MEMCPY_CSUM(_memory, _packed, do_now_bytes, (CONVERTOR));
72-
*(memory) += (ptrdiff_t) do_now_bytes;
73-
if (do_now == left_in_block) /* compensate if completed a blocklen */
74-
*(memory) += _elem->extent
75-
- (_elem->blocklen * opal_datatype_basicDatatypes[_elem->common.type]->size);
76-
77-
*(COUNT) -= do_now;
78-
*(SPACE) -= do_now_bytes;
69+
OPAL_DATATYPE_SAFEGUARD_POINTER( _memory, do_now_bytes, (CONVERTOR)->pBaseBuf,
70+
(CONVERTOR)->pDesc, (CONVERTOR)->count );
71+
DO_DEBUG( opal_output( 0, "unpack memcpy( %p [%ld], %p, %lu ) => space %lu [prolog]\n",
72+
(void*)_memory, _memory - CONVERTOR->pBaseBuf,
73+
(void*)_packed, (unsigned long)do_now_bytes, (unsigned long)(*(SPACE)) ); );
74+
MEMCPY_CSUM( _memory, _packed, do_now_bytes, (CONVERTOR) );
75+
*(memory) += (ptrdiff_t)do_now_bytes;
76+
if( do_now == left_in_block ) /* compensate if completed a blocklen */
77+
*(memory) += _elem->extent - (_elem->blocklen * opal_datatype_basicDatatypes[_elem->common.type]->size);
78+
79+
*(COUNT) -= do_now;
80+
*(SPACE) -= do_now_bytes;
7981
*(packed) += do_now_bytes;
8082
return (do_now == left_in_block);
8183
}
@@ -96,7 +98,7 @@ static inline void unpack_predefined_data(opal_convertor_t *CONVERTOR, const dt_
9698
if ((blocklen_bytes * cando_count) > *(SPACE))
9799
cando_count = (*SPACE) / blocklen_bytes;
98100

99-
/* premptively update the number of COUNT we will return. */
101+
/* preemptively update the number of COUNT we will return. */
100102
*(COUNT) -= cando_count;
101103

102104
if (_elem->blocklen < 9) {
@@ -109,16 +111,17 @@ static inline void unpack_predefined_data(opal_convertor_t *CONVERTOR, const dt_
109111
/* else unrecognized _elem->common.type, use the memcpy path */
110112
}
111113

112-
if (1 == _elem->blocklen) { /* Do as many full blocklen as possible */
114+
if (1 == _elem->blocklen) { /* Do as many full blocklen as possible */
113115
for (; cando_count > 0; cando_count--) {
114-
OPAL_DATATYPE_SAFEGUARD_POINTER(_memory, blocklen_bytes, (CONVERTOR)->pBaseBuf,
115-
(CONVERTOR)->pDesc, (CONVERTOR)->count);
116-
DO_DEBUG(opal_output(0, "unpack memcpy( %p, %p, %lu ) => space %lu [blen = 1]\n",
117-
(void *) _memory, (void *) _packed, (unsigned long) blocklen_bytes,
118-
(unsigned long) (*(SPACE) - (_packed - *(packed)))););
119-
MEMCPY_CSUM(_memory, _packed, blocklen_bytes, (CONVERTOR));
120-
_packed += blocklen_bytes;
121-
_memory += _elem->extent;
116+
OPAL_DATATYPE_SAFEGUARD_POINTER( _memory, blocklen_bytes, (CONVERTOR)->pBaseBuf,
117+
(CONVERTOR)->pDesc, (CONVERTOR)->count );
118+
DO_DEBUG( opal_output( 0, "unpack memcpy( %p [%ld], %p [%ld], %lu ) => space %lu [blen = 1]\n",
119+
(void*)_memory, _memory - CONVERTOR->pBaseBuf,
120+
(void*)_packed, _packed - *packed,
121+
(unsigned long)blocklen_bytes, (unsigned long)(*(SPACE) - (_packed - *(packed))) ); );
122+
MEMCPY_CSUM( _memory, _packed, blocklen_bytes, (CONVERTOR) );
123+
_packed += blocklen_bytes;
124+
_memory += _elem->extent;
122125
}
123126
goto update_and_return;
124127
}
@@ -127,14 +130,15 @@ static inline void unpack_predefined_data(opal_convertor_t *CONVERTOR, const dt_
127130
blocklen_bytes *= _elem->blocklen;
128131

129132
do { /* Do as many full blocklen as possible */
130-
OPAL_DATATYPE_SAFEGUARD_POINTER(_memory, blocklen_bytes, (CONVERTOR)->pBaseBuf,
131-
(CONVERTOR)->pDesc, (CONVERTOR)->count);
132-
DO_DEBUG(opal_output(0, "unpack 2. memcpy( %p, %p, %lu ) => space %lu\n",
133-
(void *) _memory, (void *) _packed, (unsigned long) blocklen_bytes,
134-
(unsigned long) (*(SPACE) - (_packed - *(packed)))););
135-
MEMCPY_CSUM(_memory, _packed, blocklen_bytes, (CONVERTOR));
136-
_packed += blocklen_bytes;
137-
_memory += _elem->extent;
133+
OPAL_DATATYPE_SAFEGUARD_POINTER( _memory, blocklen_bytes, (CONVERTOR)->pBaseBuf,
134+
(CONVERTOR)->pDesc, (CONVERTOR)->count );
135+
DO_DEBUG( opal_output( 0, "unpack 2. memcpy( %p [%ld], %p [%ld], %lu ) => space %lu\n",
136+
(void*)_memory, _memory - CONVERTOR->pBaseBuf,
137+
(void*)_packed, _packed - *packed,
138+
(unsigned long)blocklen_bytes, (unsigned long)(*(SPACE) - (_packed - *(packed))) ); );
139+
MEMCPY_CSUM( _memory, _packed, blocklen_bytes, (CONVERTOR) );
140+
_packed += blocklen_bytes;
141+
_memory += _elem->extent;
138142
cando_count -= _elem->blocklen;
139143
} while (_elem->blocklen <= cando_count);
140144
}
@@ -146,14 +150,15 @@ static inline void unpack_predefined_data(opal_convertor_t *CONVERTOR, const dt_
146150
assert((cando_count < _elem->blocklen)
147151
|| ((1 == _elem->count) && (cando_count <= _elem->blocklen)));
148152
do_now_bytes = cando_count * opal_datatype_basicDatatypes[_elem->common.type]->size;
149-
OPAL_DATATYPE_SAFEGUARD_POINTER(_memory, do_now_bytes, (CONVERTOR)->pBaseBuf,
150-
(CONVERTOR)->pDesc, (CONVERTOR)->count);
151-
DO_DEBUG(opal_output(0, "unpack 3. memcpy( %p, %p, %lu ) => space %lu [epilog]\n",
152-
(void *) _memory, (void *) _packed, (unsigned long) do_now_bytes,
153-
(unsigned long) (*(SPACE) - (_packed - *(packed)))););
154-
MEMCPY_CSUM(_memory, _packed, do_now_bytes, (CONVERTOR));
155-
_memory += do_now_bytes;
156-
_packed += do_now_bytes;
153+
OPAL_DATATYPE_SAFEGUARD_POINTER( _memory, do_now_bytes, (CONVERTOR)->pBaseBuf,
154+
(CONVERTOR)->pDesc, (CONVERTOR)->count );
155+
DO_DEBUG( opal_output( 0, "unpack 3. memcpy( %p [%ld], %p [%ld], %lu ) => space %lu [epilog]\n",
156+
(void*)_memory, _memory - CONVERTOR->pBaseBuf,
157+
(void*)_packed, _packed - *packed,
158+
(unsigned long)do_now_bytes, (unsigned long)(*(SPACE) - (_packed - *(packed))) ); );
159+
MEMCPY_CSUM( _memory, _packed, do_now_bytes, (CONVERTOR) );
160+
_memory += do_now_bytes;
161+
_packed += do_now_bytes;
157162
}
158163

159164
update_and_return:

‎test/datatype/Makefile.am

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
#
1616

1717
if PROJECT_OMPI
18-
MPI_TESTS = checksum position position_noncontig ddt_test ddt_raw ddt_raw2 unpack_ooo ddt_pack external32 large_data
18+
MPI_TESTS = checksum position position_noncontig ddt_test ddt_raw ddt_raw2 unpack_ooo ddt_pack external32 large_data partial
1919
MPI_CHECKS = to_self reduce_local
2020
endif
2121
TESTS = opal_datatype_test unpack_hetero $(MPI_TESTS)
@@ -102,5 +102,11 @@ reduce_local_LDADD = \
102102
$(top_builddir)/ompi/lib@[email protected] \
103103
$(top_builddir)/opal/lib@[email protected]
104104

105+
partial_local_SOURCES = partial.c
106+
partial_LDFLAGS = $(OMPI_PKG_CONFIG_LDFLAGS)
107+
partial_LDADD = \
108+
$(top_builddir)/ompi/lib@[email protected] \
109+
$(top_builddir)/opal/lib@[email protected]
110+
105111
distclean:
106112
rm -rf *.dSYM .deps .libs *.log *.o *.trs $(check_PROGRAMS) Makefile

0 commit comments

Comments
 (0)