RE: mempool name size incorrect?

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> > > > > > > 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.

I can hardly imagine how doing extra dereference will become a real problem
for some debugging scripts.

> 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'd rather make it arbitrary, or simply not touch at all ;)
 
> >
> > >
> > > 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
> > > > > > >