From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 55EA4A0C49; Wed, 16 Jun 2021 13:11:21 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C9FB34067A; Wed, 16 Jun 2021 13:11:20 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id D380640140 for ; Wed, 16 Jun 2021 13:11:19 +0200 (CEST) IronPort-SDR: XM0BM3c73ALFmn57pdvQxhH8a/hMIhktbCoKZTIhZi4IbRmLnQXjtImrUYCSGMHtltrlMhlZkK yhHQn5AiLCuw== X-IronPort-AV: E=McAfee;i="6200,9189,10016"; a="185848404" X-IronPort-AV: E=Sophos;i="5.83,277,1616482800"; d="scan'208";a="185848404" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Jun 2021 04:11:18 -0700 IronPort-SDR: 4GaV5QefXit9I8SpqemFSFDdBbv4j3DRSGfCL2IkZETg6VAROGPVih5+4xQj5JGB6gVZr8UrsS E+RfKQpOLy1w== X-IronPort-AV: E=Sophos;i="5.83,277,1616482800"; d="scan'208";a="442849192" Received: from gepeat-mobl1.ger.corp.intel.com (HELO [10.213.218.158]) ([10.213.218.158]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Jun 2021 04:11:17 -0700 To: dev@dpdk.org References: <20210614105839.3379790-1-thomas@monjalon.net> From: "Burakov, Anatoly" Message-ID: Date: Wed, 16 Jun 2021 12:11:13 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 MIME-Version: 1.0 In-Reply-To: <20210614105839.3379790-1-thomas@monjalon.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] parray: introduce internal API for dynamic arrays X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 14-Jun-21 11:58 AM, Thomas Monjalon wrote: > Performance of access in a fixed-size array is very good > because of cache locality > and because there is a single pointer to dereference. > The only drawback is the lack of flexibility: > the size of such an array cannot be increase at runtime. > > An approach to this problem is to allocate the array at runtime, > being as efficient as static arrays, but still limited to a maximum. > > That's why the API rte_parray is introduced, > allowing to declare an array of pointer which can be resized dynamically > and automatically at runtime while keeping a good read performance. > > After resize, the previous array is kept until the next resize > to avoid crashs during a read without any lock. > > Each element is a pointer to a memory chunk dynamically allocated. > This is not good for cache locality but it allows to keep the same > memory per element, no matter how the array is resized. > Cache locality could be improved with mempools. > The other drawback is having to dereference one more pointer > to read an element. > > There is not much locks, so the API is for internal use only. > This API may be used to completely remove some compilation-time maximums. > > Signed-off-by: Thomas Monjalon > --- > +int32_t > +rte_parray_find_next(struct rte_parray *obj, int32_t index) > +{ > + if (obj == NULL || index < 0) { > + rte_errno = EINVAL; > + return -1; > + } > + > + pthread_mutex_lock(&obj->mutex); > + > + while (index < obj->size && obj->array[index] == NULL) > + index++; > + if (index >= obj->size) > + index = -1; > + > + pthread_mutex_unlock(&obj->mutex); > + > + rte_errno = 0; > + return index; > +} > + Just a general comment about this: I'm not really sure i like this "kinda-sorta-threadsafe-but-not-really" approach. IMO something either should be thread-safe, or it should be explicitly not thread-safe. There's no point in locking here because any user of find_next() will *necessarily* race with other users, because by the time we exit the function, the result becomes stale - so why are we locking in the first place? Would be perhaps be better to leave it as non-thread-safe at its core, but introduce wrappers for atomic-like access to the array? E.g. something like `rte_parray_find_next_free_and_set()` that will perform the lock-find-next-set-unlock sequence? Or, alternatively, have the mutex there, but provide API's for explicit locking, and put the burden on the user to actually do the locking correctly. -- Thanks, Anatoly