RE: [RFC 1/2] eal: add pointer compression functions

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, October 9, 2023 10:54 AM
> To: Paul Szczepanek <Paul.Szczepanek@arm.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> Kamalakshitha Aligeri <Kamalakshitha.Aligeri@arm.com>
> Subject: Re: [RFC 1/2] eal: add pointer compression functions
> 
> 27/09/2023 17:08, Paul Szczepanek:
> > Add a new utility header for compressing pointers. Pointers are
> > compressed by taking advantage of their locality. Instead of storing
> > the full address only an offset from a known base is stored.
> 
> You probably need to insert some explanations from the cover letter.
> 
> > The provided functions can store pointers in 32bit offsets.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Paul Szczepanek <paul.szczepanek@arm.com>
> > Signed-off-by: Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> [...]
> > --- a/lib/eal/include/meson.build
> > +++ b/lib/eal/include/meson.build
> > @@ -35,6 +35,7 @@ headers += files(
> >          'rte_pci_dev_feature_defs.h',
> >          'rte_pci_dev_features.h',
> >          'rte_per_lcore.h',
> > +	'rte_ptr_compress.h',
> >          'rte_pflock.h',
> >          'rte_random.h',
> >          'rte_reciprocal.h',
> 
> Did you try to sort alphabetically? failed :)
> 
> > +#ifndef _RTE_PTR_COMPRESS_H_
> > +#define _RTE_PTR_COMPRESS_H_
> 
> No need extra underscores.
> 
> > +
> > +/**
> > + * @file
> > + * RTE pointer compression and decompression.
> 
> RTE has no mean here I think.
> 
> > + */
> > +
> > +#include <stdint.h>
> > +#include <inttypes.h>
> > +
> > +#include <rte_branch_prediction.h>
> > +#include <rte_common.h>
> > +#include <rte_debug.h>
> > +#include <rte_vect.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * Compress pointers into 32 bit offsets from base pointer.
> 
> I think it should be "32-bit".
> 
> > + *
> > + * @note Offsets from the base pointer must fit within 32bits.
> > + Alignment allows
> > + * us to drop bits from the offsets - this means that for pointers
> > + aligned by
> > + * 8 bytes they must be within 32GB of the base pointer. Unaligned
> > + pointers
> > + * must be within 4GB.
> 
> Not clear what is "alignment".
> > + *
> > + * @param ptr_base
> > + *   A pointer used to calculate offsets of pointers in src_table.
> > + * @param src_table
> > + *   A pointer to an array of pointers.
> > + * @param dest_table
> > + *   A pointer to an array of compressed pointers returned by this function.
> > + * @param n
> > + *   The number of objects to compress, must be strictly positive.
> > + * @param bit_shift
> > + *   Byte alignment of memory pointed to by the pointers allows for
> > + *   bits to be dropped from the offset and hence widen the memory region
> that
> > + *   can be covered. This controls how many bits are right shifted.
> > + **/
> > +static __rte_always_inline void
> > +rte_ptr_compress_32(void *ptr_base, void **src_table,
> > +		uint32_t *dest_table, unsigned int n, unsigned int bit_shift) {
> > +	unsigned int i = 0;
> > +#if defined RTE_HAS_SVE_ACLE
> > +	svuint64_t v_src_table;
> > +	svuint64_t v_dest_table;
> > +	svbool_t pg = svwhilelt_b64(i, n);
> > +	do {
> > +		v_src_table = svld1_u64(pg, (uint64_t *)src_table + i);
> > +		v_dest_table = svsub_x(pg, v_src_table, (uint64_t)ptr_base);
> > +		v_dest_table = svlsr_x(pg, v_dest_table, bit_shift);
> > +		svst1w(pg, &dest_table[i], v_dest_table);
> > +		i += svcntd();
> > +		pg = svwhilelt_b64(i, n);
> > +	} while (svptest_any(svptrue_b64(), pg)); #elif defined __ARM_NEON
> > +	uint64_t ptr_diff;
> > +	uint64x2_t v_src_table;
> > +	uint64x2_t v_dest_table;
> > +	/* right shift is done by left shifting by negative int */
> > +	int64x2_t v_shift = vdupq_n_s64(-bit_shift);
> > +	uint64x2_t v_ptr_base = vdupq_n_u64((uint64_t)ptr_base);
> > +	for (; i < (n & ~0x1); i += 2) {
> > +		v_src_table = vld1q_u64((const uint64_t *)src_table + i);
> > +		v_dest_table = vsubq_u64(v_src_table, v_ptr_base);
> > +		v_dest_table = vshlq_u64(v_dest_table, v_shift);
> > +		vst1_u32(dest_table + i, vqmovn_u64(v_dest_table));
> > +	}
> > +	/* process leftover single item in case of odd number of n */
> > +	if (unlikely(n & 0x1)) {
> > +		ptr_diff = RTE_PTR_DIFF(src_table[i], ptr_base);
> > +		dest_table[i] = (uint32_t) (ptr_diff >> bit_shift);
> > +	}
> > +#else
> > +	uint64_t ptr_diff;
> > +	for (; i < n; i++) {
> > +		ptr_diff = RTE_PTR_DIFF(src_table[i], ptr_base);
> > +		/* save extra bits that are redundant due to alignment */
> > +		ptr_diff = ptr_diff >> bit_shift;
> > +		/* make sure no truncation will happen when casting */
> > +		RTE_ASSERT(ptr_diff <= UINT32_MAX);
> > +		dest_table[i] = (uint32_t) ptr_diff;
> > +	}
> > +#endif
> > +}
> 
> I see it is providing some per-CPU optimizations, so it is in favor of having it in
> DPDK.
> Other than that, it looks very generic, so it is questionable to have in DPDK.
We had it done for mbuf pointers. But then, we thought it could be generic.

Right now the API results in 32b indices. We could make it generic further by allowing for 16b indices. 8b indices does not make sense.
> 
>