Change the use of rte_ring for dumping in/out/drop packets to a newer…

[yevzman]

… approach

[ol-imorozko]

As far as I know, we don't have any coding style document apart from .clang-format which should specify such things. However, if we strive to use "modern C++", we shouldn't use macros.

From C++ Core Guidelines, part ES.31:
Don’t use macros for constants or “functions”
Reason
Macros are a major source of bugs. Macros don’t obey the usual scope and type rules. Macros don’t obey the usual rules for argument passing. Macros ensure that the human reader sees something different from what the compiler sees. Macros complicate tool building.

Here we could use constexpr unsigned long max_pack_size = 16384; or maybe constexpr auto MY_CONSTANT{42}

[GeorgyKirichenko]

At least we should move the definition to common/config.release.h where it will be look more consistent.

[ol-imorozko]

Again, I don't know if we allow or prohibit this, but C++ Core Guidelines tells us "ES.46: Avoid lossy (narrowing, truncating) arithmetic conversions".
This is a narrowing conversion from 'unsigned long' to signed type 'int' for both unit_size and units_member.

[GeorgyKirichenko]

Good catch

[ol-imorozko]

This almost looks like a previous block of code (body of for (const auto& [tag, ring_cfg] : config.shared_memory) loop).
I think we could extract common functionality to a function.
something like:

  void foo(arguments)
  {
      auto name = base_name + std::to_string(core_id) + (is_low_priority ? "" : "_" + std::to_string(ring_id));
  
      auto unit_size = sizeof(sharedmemory::item_header_t) + dump_size;
      if (unit_size % RTE_CACHE_LINE_SIZE != 0)
      {
          unit_size += RTE_CACHE_LINE_SIZE - unit_size % RTE_CACHE_LINE_SIZE;
      }
  
      auto size = sizeof(sharedmemory::ring_header_t) + unit_size * units_number;
      auto offset = offsets[shm];
      auto memaddr = reinterpret_cast<void*>(reinterpret_cast<std::uintptr_t>(shm) + offset);
  
      sharedmemory::cSharedMemory ring;
      ring.init(memaddr, unit_size, units_number);
      offsets[shm] += size;
  
      auto meta = common::idp::get_shm_info::dump_meta(name, tag, unit_size, units_number, core_id, socket_id, key, offset);
      dumps_meta.emplace_back(meta);
  }

as we cannot interact with private worker fields in the regular function.

Then the "low priority ring" turns into:

foo(...);
worker->lowPriorityRing = ring;

and loop body turns into

foo();
worker->dumpRings[ring_id] = ring;                                                                                                                                  
tag_to_id[tag] = ring_id;                                          
ring_id++;  

[ol-imorozko]

Avoid c-style casts.
It's generally better to avoid casts whatsoever, but I understand that some things cannot be done without them (though I'm pretty sure we can avoid some of them by using templates and/or std::variant, but that's a topic for discussion).

Here we should use named casts: auto memaddr = reinterpret_cast<void*>(reinterpret_cast<std::uintptr_t>(shm) + offset);

[GeorgyKirichenko]

We use C-style typecast in sake of performance and the code is on our most important hot-path.

Also this memory region will contain many data structures (and arrays of them) mapped dynamically with data offsets. Another requirement that the region layout should be parseable by third-party application written in C or Go for example.

However, I think we could improve the approach but let us do this in the another piece of work.

[ol-imorozko]

Okay, but I cannot resolve the conversation by myself, I guess I don't have the related permissions :)

[TheRandomCharacter]

Unlike static_cast, but like const_cast, the reinterpret_cast expression does not compile to any CPU instructions (except when converting between integers and pointers, or between pointers on obscure architectures where pointer representation depends on its type). It is primarily a compile-time directive which instructs the compiler to treat expression as if it had the type target-type.

So there is no penalty in using reinterpret_cast. It also has a benefit of being easily greped