Hello Paul, On Thu, Mar 7, 2024 at 9:40 PM Paul Szczepanek <paul.szczepa...@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
