> > > > > > Isn't the RTE_MEMPOOL_NAMESIZE too short?
> > > > > >
> > > > > > Looking at the names sizes:
> > > > > >
> > > > > > RTE_MEMZONE_NAMESIZE = 32,
> > > > > > RTE_RING_NAMESIZE = RTE_MEMZONE_NAMESIZE - (sizeof("RG_")=4)
> + 1
> > > =
> > > > > 29,
> > > > > > RTE_MEMPOOL_NAMESIZE = RTE_RING_NAMESIZE - (sizeof("MP_")=4)
> + 1
> > > =
> > > > 26
> > > > > >
> > > > > > Referring to [1], I think it should be fixed as:
> > > > > > - #define RTE_MEMPOOL_NAMESIZE (RTE_RING_NAMESIZE - \
> > > > > > sizeof(RTE_MEMPOOL_MZ_PREFIX) + 1)
> > > > > > + #define RTE_MEMPOOL_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
> > > > > > sizeof(RTE_MEMPOOL_MZ_PREFIX) + 1)
> > > > > >
> > > > > > There is no ring involved, so I guess it is some kind of
> copy-
> > > paste-
> > > > > search-replace error.
> > > > > >
> > > > >
> > > > > I guess ring is involved in fact since the default mempool
> driver
> > > is
> > > > > ring.
> > > > >
> > > > > See drivers/mempool/ring/rte_mempool_ring.c ring_alloc().
> > > > >
> > > > > Yes, it is not ideal, but at least it explains why
> > > RTE_RING_NAMESIZE
> > > > > is used.
> > > >
> > > > Thanks, that explains it. Bad layer violation...
> > > > Let's hope no future mempool driver adds anything longer than
> "RG_"
> > > to the
> > > > name of any memzone it creates.
> > > >
> > > > Looking into the associated string length checks, using a too
> long
> > > name will fail
> > > > with ENAMETOOLONG.
> > > > So, using a long mempool name might succeed with some mempool
> drivers
> > > and
> > > > fail with others. :-(
> > > >
> > > > I guess there's no simple fix for that.
> > > > And I was wrong to think that the RTE_MEMPOOL_NAMESIZE should be
> > > > increased from 26 to 29.
> > >
> > > As a generic thought: might be it is time to make the length across
> > > these
> > > structs (mempool, ring, etc.) arbitrary?
> > > At our next big API breakage or so.
> >
> > I assume the names are allocated as part of the structures to make
> them part of
> > the shared memory, to be searchable by secondary processes.
>
> Yes sure, but we can put the actual name-buffer somewhere else:
> let say for the ring at the very end (after the elements array) and
> store only a pointer to it in the rte_ring header.
> I.E. at ring_create we check name-length - allocate extra 'name-length'
> bytes for it, initialize pointer to name with address of this extra
> bytes.
> Probably the same can be done with the mempool, etc.
Could be done, yes.
Another reason for having the names first in the structures might be for
debugging, e.g. when looking at raw memory.
Supporting arbitrary name lengths is probably not worth the effort.
But 31 characters seems somewhat short to me.
E.g. I have one named "RG_HT_ethertype_0000:01:00.0", 28 characters, which
seems to be created by a driver.
So we could extend them to 63 characters. "Should be enough for anyone" -
famous last words. ;-)
>
> >
> > I guess someone once thought 32 characters would be enough.
> >
> > Furthermore, these startup-only parts of the structures really should
> be clearly
> > marked as such, to ensure that they remain in dedicated cache lines,
> not mixed
> > with hot fields in the same structures.
> > As I mentioned below, when building for 32 bit architectures, the
> cache_size field
> > - which is accessed by the mempool fastpath functions - is in a cache
> line shared
> > with the name, instead of the cache line with the local_cache field
> pointing to the
> > local cache; i.e. causing two cache misses instead of one, when
> getting or putting
> > objects from/to a mempool cache.
> >
> > I agree that this should be cleaned up at a future big API breakage.
> >
> > >
> > > > >
> > > > > >
> > > > > > Looking at the rte_mempool structure [2]:
> > > > > > struct __rte_cache_aligned rte_mempool {
> > > > > > char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
> > > > > > union {
> > > > > > void *pool_data; /**< Ring or pool to store
> > > > > objects. */
> > > > > > uint64_t pool_id; /**< External mempool
> > > identifier.
> > > > > */
> > > > > > };
> > > > > >
> > > > > >
> > > > > > Due to the 8-byte alignment of the pool_id field following
> the
> > > name
> > > > > field, fixing the length as suggested doesn't change the memory
> > > layout
> > > > > for 64 bit CPU architectures.
> > > > > > But it does for 32 bit CPU architectures, which will only 4-
> byte
> > > > > align the pool_id field.
> > > > > >
> > > > > > [1]: https://elixir.bootlin.com/dpdk/v26.03-
> > > > > rc1/source/lib/mempool/rte_mempool.h#L128
> > > > > > [2]: https://elixir.bootlin.com/dpdk/v26.03-
> > > > > rc1/source/lib/mempool/rte_mempool.h#L230
> > > > > >
> > > > > >
> > > > > > Another thing:
> > > > > > On 32 bit CPU architectures, the cache_size and local_cache
> > > fields in
> > > > > the rte_mempool structure are not in the same cache line.
> > > > > > But I guess we don't really care about 32 bit CPU
> architectures.
> > > > > >
> > > > > >
> > > > > > Venlig hilsen / Kind regards,
> > > > > > -Morten Brørup
> > > > > >
