From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Aditya Ambadipudi" <aditya.ambadipudi@arm.com>, <dev@dpdk.org>,
<jackmin@nvidia.com>, <stephen@networkplumber.org>,
<matan@nvidia.com>, <viacheslavo@nvidia.com>,
<roretzla@linux.microsoft.com>, <konstantin.ananyev@huawei.com>,
<hofors@lysator.liu.se>
Cc: <wathsala.vithanage@arm.com>, <dhruv.tripathi@arm.com>,
<honnappa.nagarahalli@arm.com>, <nd@arm.com>
Subject: RE: [PATCH v2 1/2] deque: add multi-thread unsafe double ended queue
Date: Wed, 24 Apr 2024 17:16:50 +0200 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F3E2@smartserver.smartshare.dk> (raw)
In-Reply-To: <20240424134233.1336370-2-aditya.ambadipudi@arm.com>
[...]
> +
> +/* mask of all valid flag values to deque_create() */
> +#define __RTE_DEQUE_F_MASK (RTE_DEQUE_F_EXACT_SZ)
> +ssize_t
> +rte_deque_get_memsize_elem(unsigned int esize, unsigned int count)
> +{
> + ssize_t sz;
> +
> + /* Check if element size is a multiple of 4B */
> + if (esize % 4 != 0) {
> + rte_log(RTE_LOG_ERR, rte_deque_log_type,
> + "%s(): element size is not a multiple of 4\n",
> + __func__);
Double indent when continuing on the next line:
+ rte_log(RTE_LOG_ERR, rte_deque_log_type,
+ "%s(): element size is not a multiple of 4\n",
+ __func__);
Not just here, but multiple locations in the code.
> +
> + return -EINVAL;
> + }
> +
> + /* count must be a power of 2 */
> + if ((!RTE_IS_POWER_OF_2(count)) || (count > RTE_DEQUE_SZ_MASK)) {
> + rte_log(RTE_LOG_ERR, rte_deque_log_type,
> + "%s(): Requested number of elements is invalid,"
> + "must be power of 2, and not exceed %u\n",
> + __func__, RTE_DEQUE_SZ_MASK);
Please use shorter error messages, so they can fit on one line in the source code.
Note: DPDK coding style allows 100 chars source code line length, not just 80.
[...]
> +/* create the deque for a given element size */
> +struct rte_deque *
> +rte_deque_create(const char *name, unsigned int esize, unsigned int count,
> + int socket_id, unsigned int flags)
> +{
> + char mz_name[RTE_MEMZONE_NAMESIZE];
> + struct rte_deque *d;
> + const struct rte_memzone *mz;
> + ssize_t deque_size;
> + int mz_flags = 0;
> + const unsigned int requested_count = count;
> + int ret;
> +
> + /* for an exact size deque, round up from count to a power of two */
> + if (flags & RTE_DEQUE_F_EXACT_SZ)
> + count = rte_align32pow2(count + 1);
> +
> + deque_size = rte_deque_get_memsize_elem(esize, count);
> + if (deque_size < 0) {
> + rte_errno = -deque_size;
> + return NULL;
> + }
> +
> + ret = snprintf(mz_name, sizeof(mz_name), "%s%s",
> + RTE_DEQUE_MZ_PREFIX, name);
> + if (ret < 0 || ret >= (int)sizeof(mz_name)) {
> + rte_errno = ENAMETOOLONG;
> + return NULL;
> + }
> +
> + /* reserve a memory zone for this deque. If we can't get rte_config or
> + * we are secondary process, the memzone_reserve function will set
> + * rte_errno for us appropriately - hence no check in this function
> + */
> + mz = rte_memzone_reserve_aligned(mz_name, deque_size, socket_id,
> + mz_flags, alignof(struct rte_deque));
> + if (mz != NULL) {
> + d = mz->addr;
> + /* no need to check return value here, we already checked the
> + * arguments above
> + */
> + rte_deque_init(d, name, requested_count, flags);
rte_deque_init() error handling is missing here.
> + d->memzone = mz;
> + } else {
> + d = NULL;
> + rte_log(RTE_LOG_ERR, rte_deque_log_type,
> + "%s(): Cannot reserve memory\n", __func__);
> + }
> + return d;
> +}
[...]
> +#define RTE_DEQUE_MZ_PREFIX "DEQUE_"
> +/** The maximum length of a deque name. */
> +#define RTE_DEQUE_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
> + sizeof(RTE_DEQUE_MZ_PREFIX) + 1)
> +
> +/**
> + * Double ended queue (deque) structure.
> + *
> + * The producer and the consumer have a head and a tail index. These indices
> + * are not between 0 and size(deque)-1. These indices are between 0 and
> + * 2^32 -1. Their value is masked while accessing the objects in deque.
> + * These indices are unsigned 32bits. Hence the result of the subtraction is
> + * always a modulo of 2^32 and it is between 0 and capacity.
> + */
> +struct rte_deque {
> + alignas(RTE_CACHE_LINE_SIZE) char name[RTE_DEQUE_NAMESIZE];
Suggest alternative:
+struct __rte_cache_aligned rte_deque {
+ char name[RTE_DEQUE_NAMESIZE];
> + /**< Name of the deque */
> + int flags;
> + /**< Flags supplied at creation. */
> + const struct rte_memzone *memzone;
> + /**< Memzone, if any, containing the rte_deque */
> +
> + alignas(RTE_CACHE_LINE_SIZE) char pad0; /**< empty cache line */
Why the cache alignment here?
If required, omit the pad0 field and cache align the size field instead.
Alternatively, use RTE_CACHE_GUARD, if that is what you are trying to achieve.
> +
> + uint32_t size; /**< Size of deque. */
> + uint32_t mask; /**< Mask (size-1) of deque. */
> + uint32_t capacity; /**< Usable size of deque */
> + /** Ring head and tail pointers. */
> + volatile uint32_t head;
> + volatile uint32_t tail;
> +};
[...]
> +static __rte_always_inline void
> +__rte_deque_enqueue_elems_head_128(struct rte_deque *d,
> + const void *obj_table,
> + unsigned int n)
> +{
> + unsigned int i;
> + const uint32_t size = d->size;
> + uint32_t idx = (d->head & d->mask);
> + rte_int128_t *deque = (rte_int128_t *)&d[1];
> + const rte_int128_t *obj = (const rte_int128_t *)obj_table;
> + if (likely(idx + n <= size)) {
> + for (i = 0; i < (n & ~0x1); i += 2, idx += 2)
> + memcpy((void *)(deque + idx),
> + (const void *)(obj + i), 32);
With 100 chars source code line length, this memcpy() fits on one line.
Not just here, but in all the functions.
> + switch (n & 0x1) {
> + case 1:
> + memcpy((void *)(deque + idx),
> + (const void *)(obj + i), 16);
> + }
> + } else {
> + for (i = 0; idx < size; i++, idx++)
> + memcpy((void *)(deque + idx),
> + (const void *)(obj + i), 16);
> + /* Start at the beginning */
> + for (idx = 0; i < n; i++, idx++)
> + memcpy((void *)(deque + idx),
> + (const void *)(obj + i), 16);
> + }
> +}
next prev parent reply other threads:[~2024-04-24 15:16 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-21 6:04 [RFC] lib/st_ring: add single thread ring Honnappa Nagarahalli
2023-08-21 7:37 ` Morten Brørup
2023-08-22 5:47 ` Honnappa Nagarahalli
2023-08-24 8:05 ` Morten Brørup
2023-08-24 10:52 ` Mattias Rönnblom
2023-08-24 11:22 ` Morten Brørup
2023-08-26 23:34 ` Honnappa Nagarahalli
2023-08-21 21:14 ` Mattias Rönnblom
2023-08-22 5:43 ` Honnappa Nagarahalli
2023-08-22 8:04 ` Mattias Rönnblom
2023-08-22 16:28 ` Honnappa Nagarahalli
2023-09-04 10:13 ` Konstantin Ananyev
2023-09-04 18:10 ` Honnappa Nagarahalli
2023-09-05 8:19 ` Konstantin Ananyev
2024-04-01 1:37 ` [PATCH v1 0/2] deque: add multithread unsafe deque library Aditya Ambadipudi
2024-04-01 1:37 ` [PATCH v1 1/2] deque: add multi-thread unsafe double ended queue Aditya Ambadipudi
2024-04-06 9:35 ` Morten Brørup
2024-04-24 13:42 ` [PATCH v2 0/2] deque: add multithread unsafe deque library Aditya Ambadipudi
2024-04-24 13:42 ` [PATCH v2 1/2] deque: add multi-thread unsafe double ended queue Aditya Ambadipudi
2024-04-24 15:16 ` Morten Brørup [this message]
2024-04-24 17:21 ` Patrick Robb
2024-04-25 7:43 ` Ali Alnubani
2024-04-24 23:28 ` Mattias Rönnblom
2024-05-02 20:19 ` [PATCH v3 0/2] deque: add multithread unsafe deque library Aditya Ambadipudi
2024-05-02 20:19 ` [PATCH v3 1/2] deque: add multi-thread unsafe double ended queue Aditya Ambadipudi
2024-05-02 20:19 ` [PATCH v3 2/2] deque: add unit tests for the deque library Aditya Ambadipudi
2024-05-02 20:29 ` [PATCH v3 0/2] deque: add multithread unsafe " Aditya Ambadipudi
2024-04-24 13:42 ` [PATCH v2 2/2] deque: add unit tests for the " Aditya Ambadipudi
2024-04-01 1:37 ` [PATCH v1 " Aditya Ambadipudi
2024-04-01 14:05 ` [PATCH v1 0/2] deque: add multithread unsafe " Stephen Hemminger
2024-04-01 22:28 ` Aditya Ambadipudi
2024-04-02 0:05 ` Tyler Retzlaff
2024-04-02 0:47 ` Stephen Hemminger
2024-04-02 1:35 ` Honnappa Nagarahalli
2024-04-02 2:00 ` Stephen Hemminger
2024-04-02 2:14 ` Honnappa Nagarahalli
2024-04-02 2:53 ` Stephen Hemminger
[not found] ` <PAVPR08MB9185DC373708CBD16A38EFA8EF3E2@PAVPR08MB9185.eurprd08.prod.outlook.com>
2024-04-02 4:20 ` Tyler Retzlaff
2024-04-02 23:44 ` Stephen Hemminger
2024-04-03 0:12 ` Honnappa Nagarahalli
2024-04-03 23:52 ` Variable name issues with codespell Stephen Hemminger
2024-04-02 4:20 ` [PATCH v1 0/2] deque: add multithread unsafe deque library Tyler Retzlaff
2024-04-03 16:50 ` Honnappa Nagarahalli
2024-04-03 17:46 ` Tyler Retzlaff
2024-04-02 6:05 ` Mattias Rönnblom
2024-04-02 15:25 ` Stephen Hemminger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35E9F3E2@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=aditya.ambadipudi@arm.com \
--cc=dev@dpdk.org \
--cc=dhruv.tripathi@arm.com \
--cc=hofors@lysator.liu.se \
--cc=honnappa.nagarahalli@arm.com \
--cc=jackmin@nvidia.com \
--cc=konstantin.ananyev@huawei.com \
--cc=matan@nvidia.com \
--cc=nd@arm.com \
--cc=roretzla@linux.microsoft.com \
--cc=stephen@networkplumber.org \
--cc=viacheslavo@nvidia.com \
--cc=wathsala.vithanage@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).