Re: [PATCH v7 0/4] add pointer compression API

[David Marchand <david.marchand@redhat.com>]

Hello Paul,

On Thu, Mar 7, 2024 at 9:40 PM Paul Szczepanek <paul.szczepanek@arm.com> wrote:
>
> This patchset is proposing adding a new EAL header with utility functions
> that allow compression of arrays of pointers.
>
> When passing caches full of pointers between threads, memory containing
> the pointers is copied multiple times which is especially costly between
> cores. A compression method will allow us to shrink the memory size
> copied.
>
> The compression takes advantage of the fact that pointers are usually
> located in a limited memory region (like a mempool). We can compress them
> by converting them to offsets from a base memory address.
>
> Offsets can be stored in fewer bytes (dictated by the memory region size
> and alignment of the pointer). For example: an 8 byte aligned pointer
> which is part of a 32GB memory pool can be stored in 4 bytes. The API is
> very generic and does not assume mempool pointers, any pointer can be
> passed in.
>
> Compression is based on few and fast operations and especially with vector
> instructions leveraged creates minimal overhead.
>
> The API accepts and returns arrays because the overhead means it only is
> worth it when done in bulk.
>
> Test is added that shows potential performance gain from compression. In
> this test an array of pointers is passed through a ring between two cores.
> It shows the gain which is dependent on the bulk operation size. In this
> synthetic test run on ampere altra a substantial (up to 25%) performance
> gain is seen if done in bulk size larger than 32. At 32 it breaks even and
> lower sizes create a small (less than 5%) slowdown due to overhead.
>
> In a more realistic mock application running the l3 forwarding dpdk
> example that works in pipeline mode on two cores this translated into a
> ~5% throughput increase on an ampere altra.
>
> v2:
> * addressed review comments (style, explanations and typos)
> * lowered bulk iterations closer to original numbers to keep runtime short
> * fixed pointer size warning on 32-bit arch
> v3:
> * added 16-bit versions of compression functions and tests
> * added documentation of these new utility functions in the EAL guide
> v4:
> * added unit test
> * fix bug in NEON implementation of 32-bit decompress
> v5:
> * disable NEON and SVE implementation on AARCH32 due to wrong pointer size
> v6:
> * added example usage to commit message of the initial commit
> v7:
> * rebase to remove clashing mailmap changes
> v8:
> * put ptr compress into its own library
> * add depends-on tag
> * remove copyright bumps
> * typos
>
> Paul Szczepanek (4):
>   ptr_compress: add pointer compression library
>   test: add pointer compress tests to ring perf test
>   docs: add pointer compression guide
>   test: add unit test for ptr compression
>
>  app/test/meson.build                       |  21 +-
>  app/test/test_ptr_compress.c               | 108 +++++++
>  app/test/test_ring.h                       |  92 ++++++
>  app/test/test_ring_perf.c                  | 352 ++++++++++++++-------
>  doc/guides/prog_guide/ptr_compress_lib.rst | 144 +++++++++
>  lib/meson.build                            |   1 +
>  lib/ptr_compress/meson.build               |   4 +
>  lib/ptr_compress/rte_ptr_compress.h        | 266 ++++++++++++++++
>  lib/ptr_compress/version.map               |   3 +
>  9 files changed, 859 insertions(+), 132 deletions(-)
>  create mode 100644 app/test/test_ptr_compress.c
>  create mode 100644 doc/guides/prog_guide/ptr_compress_lib.rst
>  create mode 100644 lib/ptr_compress/meson.build
>  create mode 100644 lib/ptr_compress/rte_ptr_compress.h
>  create mode 100644 lib/ptr_compress/version.map

We mentionned during the weekly release meeting, it seemed too late
for merging this work in the 24.03 release.

Looking at v8, I have comments on this series:
- rather than put a Depends-on: tag, take the lib: patch as part of
your series, there is no need for this patch without the ptr_compress
lib and it will avoid any CI issue (ovsrobot does not support
Depends-on: patch- for example),
- lib/ptr_compress/version.map is unneeded now,
- lib/ptr_compress/, app/test/test_ptr_compress.c and
doc/guides/prog_guide/ptr_compress_lib.rst need a MAINTAINERS entry,
- prefer lowercase characters for mail addresses in commitlogs,
- the documentation is not referenced in doc/guides/prog_guide/index.rst,
- doxygen does not know of this new library, you must update
doc/api/doxy-api-index.md and doc/api/doxy-api.conf.in,
- a RN entry is missing,

There were also comments on the lib: patch.

At this point, it is better to take our time to finish putting this
work in good form and merge it in 24.07.

Thanks.

-- 
David Marchand