VRF implementation for dataplane

[stal76]

Previously, it was possible to add routing rules for different VRFs, but it was not possible to set VRFs for packets and only 'default' was used for them.
In the current change, the ability to set VRF for the entire logical port has been added. Later it will be possible to set VRF by acl rules.
To solve the LPM problem, an algorithm like to 'a path-compressed trie' is used.

[ol-imorozko]

constexpr auto YANET_RIB_VRF_DEFAULT = "default";

[ol-imorozko]

I think we should replace that in 97th line in librib/libyabird.cpp too:

		std::get<1>(request) = {peer_address,
		                        {"default", ///< @todo: vrf
		                         YANET_RIB_PRIORITY_DEFAULT}};

[ol-imorozko]

const method returning value should be marked [[nodiscard]]

[ol-imorozko]

here we checking that exist is true for at least one element of the sequence, so I suggest to implement it like this:

[[nodiscard]] bool is_ignored_table(const std::string& table_name) const
{
	return std::any_of(routes.begin(), routes.end(), [&table_name](const auto& pair) {
		return exist(pair.second.ignore_tables, table_name);
	});
}

[ol-imorozko]

I understand that this is not your code initially, but I would do

const auto& [physicalPortName, vlanId, vrf, macAddress, promiscuousMode] = logicalPort;

table.insert(logicalPortName,
             physicalPortName,
             vlanId,
             vrf,
             macAddress,
             promiscuousMode ? "true" : "false");

here to avoid confusion

[ol-imorozko]

If my above proposal is correct, add

void ReserveFirstN(uint8_t n)
{
	for (uint8_t i = 0; i < n; ++i)
	{
		available.reset(i);
	}
}

[ol-imorozko]

uint8_t first_available = available._Find_first();
available.reset(first_available);
return first_available;

[ol-imorozko]

available.set(bit);

[ol-imorozko]

Not needed

[ol-imorozko]

These are good. Though I would add a test that covers a case where number_first_reserved gets equal to blocks_total_, smth like this should work:

TEST(BlocksAllocator, SequentialAllocationFull)
{
	size_t blocks_number = 1024;
	uint32_t reserved = blocks_number;

	size_t size = common::allocator::BlocksAllocator<64>::GetBufferSize(blocks_number);
	std::unique_ptr<uint8_t[]> buffer = std::make_unique<uint8_t[]>(size);
	common::allocator::BlocksAllocator<64> allocator;
	allocator.Init(buffer.get(), size, reserved);

	// no more blocks shoudl be available
	ASSERT_EQ(allocator.Allocate(), common::allocator::BlocksAllocator<64>::null_block);
	ASSERT_EQ(allocator.Stat(), std::make_pair(blocks_number, blocks_number));

	for (size_t i = 0; i < reserved; i++)
	{
		ASSERT_EQ(allocator[i], buffer.get() + 64 * i);
	}
}

[ol-imorozko]

just using dataplane::lpmValueIdInvalid;

[ol-imorozko]

using const references with simple fixed-size integer types is redunant and obscure, I would suggest to remove that:

  eResult Insert(Stats& stats, tVrfId vrf_id, uint32_t ip_address, uint8_t mask, uint32_t value_id);
  eResult Remove(Stats& stats, tVrfId vrf_id, uint32_t ip_address, uint8_t mask);

[ol-imorozko]

annotate Swap with noexcept

[ol-imorozko]

const unsigned int& count -> int count

[ol-imorozko]

no need to add VrfLpm4Linear:: here, we're already in that namespace, just BlocksAllocator::Index next_block;

[ol-imorozko]

here unordered_map will work too, by just finding the block pointer in O(1)

[ol-imorozko]

use extended_chunks_size_min instead of YANET_RIB_VRF_MAX_NUMBER, we done
constexpr static uint64_t extended_chunks_size_min = YANET_RIB_VRF_MAX_NUMBER; for a reason

And in 13 other places in this commit where YANET_RIB_VRF_MAX_NUMBER gets used

[ol-imorozko]

Segmentation fault.
What if we create a VrfLpm4Linear with an allocator with it's void Init(void* pointer, size_t size, Index number_first_reserved) being called with size less than YANET_RIB_VRF_MAX_NUMBER * BlockSize?

[ol-imorozko]

Since this class is designed for VRF, we should add some guard against initializing that class with buffers less than vrf max number times BlockSize with a "throw" in a constuctor, for example

[ol-imorozko]

IMHO this is cleaner:

VrfLpm4Linear::OneBlock* VrfLpm4Linear::Block(BlocksAllocator::Index index) const
{
	if (index == BlocksAllocator::null_block)
	{
		return nullptr;
	}

	return reinterpret_cast<OneBlock*>(allocator_[index]);
}

[ol-imorozko]

Same review comment as for VrfLpm4Linear applies for VrfLpm6Linear too

[ol-imorozko]

This should be done like in controlplane/unittest/meson.build, with sources defined in a separate variable:

dataplane_sources = files('../vrf.cpp',
                         )

sources = files('unittest.cpp',
                'ip_address.cpp',
                'hashtable.cpp',
                'sdp.cpp',
                'random_prefixes.cpp',
                'vrf_lpm_linear.cpp',
                'vrf_lpm_binary.cpp',
                )

arch = 'corei7'
cpp_args_append = ['-march=' + arch]

unittest = executable('yanet-dataplane-unittest',
                      [dataplane_sources, sources],

[ol-imorozko]

[[nodiscard]]

[ol-imorozko]

auto root = std::make_shared<Node>();

[ol-imorozko]

use emplace instead of push with a temporary object:

	work.emplace(root, 0);

[ol-imorozko]

prefixes.emplace_back(addr, static_cast<uint8_t>(node->depth));

[ol-imorozko]

Same comments as for v4 version

[ol-imorozko]

We could greatly reduce code duplication by utilizing GoogleTests fixtures and TYPED_TEST_SUITE to parametrize tests for both VrfLpmMap and VrfLpmLinear

[ol-imorozko]

And here we can use that VrfLpmTestBase to automatically define tests for both v4 and v6

[ol-imorozko]

use memcpy when dealing with simple u64 types instead of dereferencing a pointer with reintepret casts, much cleaner this way:

        uint64_t hi = 0, low = 0;

        std::memcpy(&hi, prefix.address.bytes, sizeof(hi));
        std::memcpy(&low, prefix.address.bytes + 8, sizeof(low));
        
        hi = rte_cpu_to_be_64(hi);
        low = rte_cpu_to_be_64(low);

[ol-imorozko]

        hi = rte_cpu_to_be_64(hi);
        low = rte_cpu_to_be_64(low);
        std::memcpy(result.bytes.data(), &hi, sizeof(hi));
        std::memcpy(result.bytes.data() + 8, &low, sizeof(low));

[ol-imorozko]

Those are functions that are used only for this .h file, so they should be static instead of inline.

[ol-imorozko]

Speaking about #242 (comment):

We need to create a struct, smth like AddressTraits, explicitly instantiate it with uint32_t and std::array<uint8_t, 16>, and use it with fixture to parametrize tests.

smth like this:

template<typename AddressType>
struct AddressTraits;

template<>
struct AddressTraits<uint32_t> {
    static uint32_t ToInternal(const ipv4_address_t& address) {
        return rte_cpu_to_be_32(address.address);
    }

    static std::string ToString(const uint32_t& address) {
        return common::ipv4_address_t(address).toString();
    }

    static std::vector<uint32_t> AddressesForTest(const ipv4_prefix_t& prefix) {
        return {rte_cpu_to_be_32(prefix.address.address), LastAddress(prefix)};
    }

    static uint32_t LastAddress(const ipv4_prefix_t& prefix) {
// impl
    }
};

template<>
struct AddressTraits<std::array<uint8_t, 16>> {
    static std::array<uint8_t, 16> ToInternal(const ipv6_address_t& address) {
        return common::ipv6_address_t(address.bytes);
    }

    static std::string ToString(const std::array<uint8_t, 16>& address) {
        return common::ipv6_address_t(address).toString();
    }

    static std::vector<ipv6_address_t> AddressesForTest(const ipv6_prefix_t& prefix) {
        return {prefix.address, LastAddress(prefix)};
    }

    static ipv6_address_t LastAddress(const ipv6_prefix_t& prefix) {
// impl
    }
};

Maybe we should also add to corresponding instantiations these things, consider it

static constexpr OneTestData tests_ipv4[] = {
        {1000, 20.0, 0.8, true, 42, 10}, // tree sizes: 1-4
        {1000, 15.0, 0.8, true, 42, 10}, // tree sizes: 2-7
        {1000, 10.0, 0.8, true, 42, 10}, // tree sizes: 6-15
        {1000, 5.0, 0.8, true, 42, 10}, // tree sizes: 86-271
        {10000, 3.0, 0.1, true, 42, 10}, // tree sizes: 271-992
        {10000, 3.0, 0.2, true, 42, 10} // tree sizes: 574-2053
};

static constexpr size_t number_tests_ipv4 = sizeof(tests_ipv4) / sizeof(OneTestData);

[ol-imorozko]

just list1 == list2? std::vector and std::tuple have == operator

[ol-imorozko]

And these methods should be within a typed test from which we will inherit later, parametrized by AddressType, like:

template<typename PrefixType, typename AddressType, typename LookupAddressType, typename VrfLpmType>
class VrfLpmTestBase : public ::testing::Test {
protected:
    using Traits = AddressTraits<AddressType>;

    void SetUp() override {
        // common init
    }

    bool FillPrefixTree(const std::vector<PrefixType>& prefixes, VrfLpmType& tree, tVrfId vrf_number) {
        // implementation
        // Use Traits::ToInternalinstead of IpAddressToInternal
    }

    bool CompareTrees(const VrfLpmType& tree1, const VrfLpmType& tree2) {
        auto list1 = tree1.GetFullList();
        auto list2 = tree2.GetFullList();
        std::sort(list1.begin(), list1.end());
        std::sort(list2.begin(), list2.end());
        return list1 == list2;
    }

    bool CheckRequests(tVrfId vrf_number, const std::vector<PrefixType>& prefixes,
                       const VrfLpmType& tree1, const VrfLpmType& tree2) {
        // implementation
        // Use Traits::AddressesForTest instead of AddressesForTest
    }
};

[ol-imorozko]

I think this should be added to VrfLpmTestBase as well, so we can use AddressType with things like if constexpr to not duplicate the code.
Those strings can be declared as constexpr static arrays of string_view in the AddressTraits:
constexpr std::array myStrings = { "::/0"sv, "7e01::/64"sv, ...};

[ol-imorozko]

And here we can use that VrfLpmTestBase to automatically define tests for both v4 and v6

[ol-imorozko]

add should_shrink and use it here

[ol-imorozko]

limits.emplace_back(std::string(name) + ".extended_chunks",

[ol-imorozko]

Current implementation with ObjectType storing reference to an allocator is really obscure. One has no
way of understanding why this code is correct if he don't know that ObjectType actually stores a reference to an allocator. Cause it looks like a regular value semantics, which should lead to an allocator object being in a wrong state.
But here we have:

• updater_vrf_lpm gets constructed and vrf_lpm_object(allocator) gets called from the member initializer list, storing the reference to that allocator object in an vrf_lpm_object member.
• user calls init which calls create
• create creates a tmp_allocator and initializes tmp_object with it. tmp_object now stores a reference to that tmp_allocator
• vrf_lpm_object.Swap(tmp_object) gets called which swaps allocators, which due to the fact that they are references, swaps the actual allocator object
• old pointer gets deleted through tmp_object
  Thi is obscure. Also manual memory managing though swapped tmp_object looks bad.

As I suggested eariler, we shouldn't use reference members:
#242 (comment)

Instead, we should:

1. Delete copy and move constructors from ObjectType. This is really important as apparently the memory that we have there gets managed by an external object, and if that pointer gets copied by a copy constructor, and the original gets deleted, we will have use-after-free
2. Just create vrf_lpm_object with default constructor in updater_vrf_lpm consturctor, it will have allocator with null pointer, which is fine
3. ObjectType already has CopyFrom, now we need to implement MoveFrom that accepts memory_manager
   and looks something like this:

void move_from(Obj obj, memory_manager)
{
    if (allocator_.GetPointer() != nullptr) allocator_.GetPointer());
    // move data
}

this way this code gets turned into:

eResult create(const uint64_t chunks_size, bool save_old)
{
	void* next_pointer = memory_manager->create(...)
	BlocksAllocator tmp_allocator;
	tmp_allocator.Init(...);
	ObjectType tmp_object(std::move(tmp_allocator));

	if (save_old)
	{
		tmp_object.CopyFrom(vrf_lpm_object);
	}

	vrf_lpm_object.MoveFrom(tmp_object, memory_manager);

	return eResult::success;
}

And if the vrf_lpm_object was nullptr, memory_manager from MoveFrom will just ignore it.
If it wasn't, it will correctly delete it and fill with the new data.

This way we will ensure that ObjectType has correct ownership of an allocator due to it being moved into a constructor
4. Delete BlocksAllocator allocator; as a field, why do we even need it here?
report(nlohmann::json& report) should get a pointer from an actual object anyway, like vrf_lpm_object.GetAllocator().GetPointer()

[ol-imorozko]

    auto hi_bytes = reinterpret_cast<const uint8_t*>(&hi);
    auto low_bytes = reinterpret_cast<const uint8_t*>(&low);

    std::copy(hi_bytes, hi_bytes + sizeof(hi), result.begin());
    std::copy(low_bytes, low_bytes + sizeof(low), result.begin() + sizeof(hi));

[ol-imorozko]

Fix the same issues as with Linear, please.
Like avoidance of reference field member, emplace instead of push with braces initialization: push({...}), should throw in constructor if the size allocated is not enough for YANET_RIB_VRF_MAX_NUMBER, otherwise loop like this

	for (tVrfId vrfId = 0; vrfId < YANET_RIB_VRF_MAX_NUMBER; vrfId++)
	{
		OneBlock* block = Block(vrfId);

will segfault on Block when we'll call allocator_[index]

And also an observation that we can cache the blocks by a pair ipAddress + mask to avoid looking them up each time