From: Dongju Chae Date: Fri, 12 Jul 2019 02:03:57 +0000 (+0900) Subject: [N71] Change a chunk state management to safely resize I/O buffers. X-Git-Tag: accepted/tizen/unified/20220103.130045~789 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a4ee40dd308b02e0c1efb75cdc4cd0192a66e4d0;p=platform%2Fadaptation%2Fnpu%2Ftrix-engine.git [N71] Change a chunk state management to safely resize I/O buffers. This commit changes the logic of chunk state management to safely resize I/O buffers. Now, each chunk has one of three states (i.e., free, activated, modified). This 'modified' state contains anything which can change the offset and size of a used chunk. Signed-off-by: Dongju Chae --- diff --git a/core/npu-engine/src/ne-mem.c b/core/npu-engine/src/ne-mem.c index 75579c0..b098690 100644 --- a/core/npu-engine/src/ne-mem.c +++ b/core/npu-engine/src/ne-mem.c @@ -37,11 +37,13 @@ /** * @brief memory chunk state + * @note here, 'modified' state means all kinds of behaviors (e.g., compaction, resize) + * which can change the offset and size of used chunks */ typedef enum { - CHUNK_STATE_FREE, /**< it can be compacted or activated later */ - CHUNK_STATE_ACTIVATED, /**< it's activated by a user; it cannot be compacted */ - CHUNK_STATE_COMPACT, /**< it's being compacted */ + CHUNK_STATE_FREE, /**< free state: this chunk can be compacted or activated later */ + CHUNK_STATE_ACTIVATED, /**< activated state: this chunk is being used by a client */ + CHUNK_STATE_MODIFIED, /**< modified state: this chunk is being modified */ } chunk_state; /** @@ -51,8 +53,10 @@ typedef struct { uint64_t offset; /**< chunk offset */ uint64_t size; /**< chunk size */ chunk_state state; /**< chunk state */ + list_node list; /**< list element */ pthread_mutex_t mutex; /**< mutex for locking */ + pthread_cond_t cond; /**< cond for broadcast */ } chunk; /** @@ -65,19 +69,19 @@ typedef enum { } hwmem_type; /** - * @brief private data structure for buffer + * @brief private data structure for hwmem */ typedef struct { - chunk *chunk; /**< associated chunk */ - hwmem_type type; /**< hwmem type */ + chunk *chunk; /**< associated chunk */ + hwmem_type type; /**< hwmem type */ } hwmem_priv; /** * @brief private data structure for buffer */ typedef struct { - hwmem *hwmem; /**< inherit hwmem */ - buffer_state state; /**< buffer state */ + hwmem *hwmem; /**< inherit hwmem */ + buffer_state state; /**< buffer state */ pthread_mutex_t mutex; /**< mutex for locking */ pthread_cond_t cond; /**< cond for broadcasting */ } buffer_priv; @@ -150,23 +154,39 @@ chunk_get_prev (list *chunk_list, chunk *target) * @brief set chunk state * @param[in] target the target chunk * @param[in] state the state to change - * @return ture if successful + * @param[in] sync the flag to indicate whether it needs to wait if failed + * @return 0 if no error, otherwise a negative errno * - * @note it should be handled exclusively to prevent - * race conditions for hwmem activation and memory compaction + * @note it should be handled exclusively to prevent race conditions */ -static bool -chunk_set_state (chunk *target, chunk_state state) +static int +chunk_set_state (chunk *target, chunk_state state, bool sync) { + int err = EINVAL; + if (!target) - return false; + goto out; pthread_mutex_lock (&target->mutex); - if (target->state == CHUNK_STATE_FREE) - target->state = state; - pthread_mutex_unlock (&target->mutex); - return target->state == state; + if (target->state == CHUNK_STATE_ACTIVATED || + target->state == CHUNK_STATE_MODIFIED) { + if (state == CHUNK_STATE_FREE) + pthread_cond_broadcast (&target->cond); + else if (sync) + while (target->state != CHUNK_STATE_FREE) + pthread_cond_wait (&target->cond, &target->mutex); + else + goto out_unlock; + } + + target->state = state; + err = 0; + +out_unlock: + pthread_mutex_unlock (&target->mutex); +out: + return -err; } /** @@ -189,6 +209,7 @@ chunk_new (uint64_t offset, uint64_t size) /* control its state */ pthread_mutex_init (&new_chunk->mutex, NULL); + pthread_cond_init (&new_chunk->cond, NULL); return new_chunk; } @@ -346,28 +367,36 @@ chunk_return (chunk *target) } /** - * @brief create a chunk with new size + * @brief create a chunk with new size * @param[in] target the chunk instance * @param[in] size size to be changed * @return new chunk instance + * @note we should be very careful to resize a used chunk + * because chunk's state (e.g., offset) can be changed. + * So, we need to ensure that nobody is using this hwmem. */ static chunk* chunk_resize (chunk *target, uint64_t size) { - chunk_state state; + chunk_state state; assert (target != NULL); assert (size > 0); - /** - * just return old chunk and alloc new chunk (to minimize fragmentations) - * but, we need to restore its chunk state after allocation - */ + /* save its state for new chunk */ state = target->state; + /* wait until it has a free state */ + chunk_set_state (target, CHUNK_STATE_MODIFIED, true); + + /* just return old chunk and alloc new chunk (to minimize fragmentations) */ chunk_return (target); target = chunk_alloc (size); + /* make it free */ + chunk_set_state (target, CHUNK_STATE_FREE, true); + + /* restore the state */ target->state = state; return target; @@ -424,7 +453,7 @@ chunk_compact_try (uint64_t *offset_dst) /* find a target chunk starting at top */ list_for_each_entry_reverse (target, mpriv.used_chunks, list) { /* should be not busy (i.e., no processing here) and other conditions (TBD)? */ - if (chunk_set_state (target, CHUNK_STATE_COMPACT)) { + if (chunk_set_state (target, CHUNK_STATE_MODIFIED, false) == 0) { chunk *next_chunk, *free_chunk = NULL; /* find a free chunk starting at bottom */ @@ -473,7 +502,7 @@ chunk_compact_try (uint64_t *offset_dst) } /* failure */ - assert (target->state == CHUNK_STATE_COMPACT); + assert (target->state == CHUNK_STATE_MODIFIED); target->state = CHUNK_STATE_FREE; } } @@ -510,7 +539,7 @@ chunk_compact (uint64_t size) target->offset = offset; chunk_add (&mpriv.used_chunks, target); - assert (target->state == CHUNK_STATE_COMPACT); + assert (target->state == CHUNK_STATE_MODIFIED); target->state = CHUNK_STATE_FREE; } @@ -642,7 +671,7 @@ buffer_destroy (buffer *buffer) } /** - * @brief resize the buffer's hwmem + * @brief resize the buffer's hwmem (or create buffer instance) * @param[in] buffer_p the pointer of buffer instance * @param[in] size size to be changed * @return true if successful @@ -652,8 +681,9 @@ buffer_resize (buffer** buffer_p, uint64_t size) { assert (buffer_p); + /* if size is zero, destroy buffer */ if (size == 0) { - if (*buffer_p) + if (*buffer_p) buffer_destroy (*buffer_p); *buffer_p = NULL; return true; @@ -664,7 +694,7 @@ buffer_resize (buffer** buffer_p, uint64_t size) assert (priv); return hwmem_resize (priv->hwmem, size); - } else + } else /* empty buffer */ return ((*buffer_p = buffer_create (size)) != NULL); } } @@ -892,7 +922,7 @@ mem_alloc (uint64_t size, hwmem **hwmem_p) } /** - * @brief resize the allocation of hwm + * @brief resize the allocation of hwmem * @param[in] hwmem the created hwmem instance * @param[in] size the new memory size * @return 0 if no error, otherwise a negative error value @@ -939,26 +969,19 @@ mem_dealloc (hwmem * hwmem) * @param[in] size new buffer size * @return 0 if no error, otherwise a negative error value * - * @note this would be used only when a model is unregistered, - * because the buffer size is the maximum size for all models. + * @note its latency can be increased due to the memory compaction. */ -static int + static int mem_resize_buffers (uint64_t size) { int err = 0; MEM_LOCK (); - /* resize buffers if larger ones are required */ - if (size > mpriv.buffer_size) { - bool first = false; - int idx; + if (size != mpriv.buffer_size) { + int idx = 0; - /* it's the first resize; we need to initialize buffers */ - if (mpriv.buffer_size == 0) - first = true; - - for (idx = 0; idx < MEM_NUM_BUFFERS; idx++) { + for (; idx < MEM_NUM_BUFFERS; idx++) { if (!buffer_resize (&mpriv.buffer[idx], size)) { /* failed; rollback existing buffers */ for (idx = idx -1; idx >= 0; idx--) @@ -968,17 +991,10 @@ mem_resize_buffers (uint64_t size) goto out; } mpriv.buffer[idx]->size = size; - if (first) { - buffer_priv *priv = mpriv.buffer[idx]->priv; - - mpriv.buffer_head = 0; - buffer_change_state (priv, BUFFER_STATE_EMPTY); - - /** - * Theses buffers are always activated. - * after buffer configuration, it can not be affected by compaction - */ - hwmem_activate (priv->hwmem); + if (mpriv.buffer_size == 0) { + /* it's the first resize; we need to initialize buffers */ + mpriv.buffer_idx[idx] = BUFFER_STATE_INVAL; + buffer_change_state (mpriv.buffer[idx]->priv, BUFFER_STATE_EMPTY); } } @@ -1072,27 +1088,31 @@ out: /** * @brief get the next I/O buffer dedicated to the requsted role. * @param[in] role the role of buffer + * @param[out] err set with error-number if there is an error * @return the next buffer if no error, otherwise NULL * * @note it needs to wait to get a valid buffer if another component is still using it. */ static buffer * -mem_get_next_buffer (buffer_role role) +mem_get_next_buffer (buffer_role role, int *err) { - buffer *buf = NULL; + buffer *buffer = NULL; + buffer_priv *priv; - if (!(role >= BUFFER_ROLE_INPUT && role <= BUFFER_ROLE_OUTPUT)) + if (!(role >= BUFFER_ROLE_INPUT && role <= BUFFER_ROLE_OUTPUT)) { + *err = -EINVAL; goto out; + } - buf = wait_until_available (role); - if (buf) { - buffer_priv *priv = buf->priv; + buffer = wait_until_available (role); - hwmem_activate (priv->hwmem); - } + assert (buffer); + + priv = buffer->priv; + *err = hwmem_activate (priv->hwmem); out: - return buf; + return buffer; } /** @@ -1237,18 +1257,16 @@ int hwmem_activate (hwmem *hwmem) { hwmem_priv *priv; - - if (!hwmem || !hwmem->priv) - RETURN_ERROR_MSG (EINVAL, "invalid hwmem; internal structure does not exist"); + + if (!hwmem || !hwmem->priv) { + logerr (MEM_TAG, "invalid hwmem; internal structure does not exist\n"); + return -EINVAL; + } priv = hwmem->priv; assert (priv->chunk); - if (priv->chunk->state != CHUNK_STATE_FREE || - !chunk_set_state (priv->chunk, CHUNK_STATE_ACTIVATED)) - RETURN_ERROR (EBUSY); - - return 0; + return chunk_set_state (priv->chunk, CHUNK_STATE_ACTIVATED, true); } /** @@ -1260,19 +1278,16 @@ int hwmem_deactivate (hwmem *hwmem) { hwmem_priv *priv; - - if (!hwmem || !hwmem->priv) - RETURN_ERROR_MSG (EINVAL, "invalid hwmem; internal structure does not exist"); + + if (!hwmem || !hwmem->priv) { + logerr (MEM_TAG, "invalid hwmem; internal structure does not exist\n"); + return -EINVAL; + } priv = hwmem->priv; assert (priv->chunk); - if (priv->chunk->state != CHUNK_STATE_ACTIVATED) - RETURN_ERROR_MSG (EINVAL, "invalid deactivation request"); - - priv->chunk->state = CHUNK_STATE_FREE; - - return 0; + return chunk_set_state (priv->chunk, CHUNK_STATE_FREE, true); } /**