MapIterator support for Queues/Stacks + further testing #1349

s41m0n · 2024-02-19T11:39:43Z

Dear community, with this PR I would like to introduce the following contributions.

I'll be available for further changes/updates.

MapIterator support for Queue/Stack maps

Ther MapIterator.Next would normally fail with maps of type Queue or Stack for two reasons:

Lookup for Queue/Stack acts as a peek method, returning always the head value without removing it. It additionally fails when provided with a non-nil key
LookupAndDelete is the method for retrieving values in a Queue/Stack map.

With that in mind, instead of adding further workarounds and control to each individual call to the previous MapIterator.Next function or to each Map.Lookup (potentially a non-negligible overhead), I'd propose splitting it into two internal methods. In this case, the program logic of calling all the other functions (e.g., Lookup, BatchLookup that is not supported) to a Queue/Stack is preserved, throwing the expected error.

The proposed solution preserves a maximum amount of retrieved items, to avoid an infinite loop as the Queue/Stack may be continuously populated while values are retrieved.

Extended tests for Queue/Stack maps

Add further tests to the Lookup method (peek) with Queue maps, previously missing
Add full tests for Stack maps, previously missing

Example of using a Queue map

I'd introduce an example that demonstrates the usage of a Queue map within an XDP program. The program parses IPv4 packets, retrieving the source address and pushing it, alongside a computed ktime_ns into the Queue. In the userspace program, the Queue is periodically emptied, formatting its content into a human-readable string containing the IPv4 address and the timestamp into a go Time struct.

Further Proposal

Would it be useful to introduce a function to parse a ktime_ns value into Time struct as a utility function of this library? In case, please don't hesitate to point me to the right file where this change could be performed and I can do it. I'd use a similar method that retrieves the boot uptime time once (to prevent continuous syscalls) and simply return it incremented by the ktime_ns argument value.

…ue-Stack tests + add queue example 1. MapIterator.Next would previously fail with maps of type Queue or Stack, as their Lookup method does not accept a key and acts just like a peek function 2. Add further tests to the Lookup method (peek) with Queue maps, and introduced tests for Stack maps 3. Add example of using Queue within an XDP program Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>

dylandreimerink

Thank you very much for putting in the work. Though I would like to request some changes.

dylandreimerink · 2024-02-26T13:23:48Z

examples/queue/main.go

+// This program demonstrates attaching an eBPF program to a network interface
+// with XDP (eXpress Data Path). The program parses the IPv4 source address
+// from packets and pushes the address alongside the computed packet arrival timestamp
+// into a Queue. This is just an example and probably does not represent the most
+// efficient way to perform such a task. Another potential solution would be to use
+// an HashMap with a small __u64 arrays associated to each IPv4 address (key).
+// In both the two ways it is possible to lose some packet if (a) queue is not large
+// enough or the packet processing time is slow or (b) if the associated array is
+// smaller than the actual received packet from an address.
+// The userspace program (Go code in this file) prints the contents
+// of the map to stdout every second, parsing the raw structure into a human-readable
+// IPv4 address and Unix timestamp.
+// It is possible to modify the XDP program to drop or redirect packets
+// as well -- give it a try!
+// This example depends on bpf_link, available in Linux kernel version 5.7 or newer.


Like you mention yourself, this isn't a prime example of queues or stacks. I understand you want an example to showcase those map types. A good example is small and doesn't add to much extra info. I honestly don't think it adds much to your PR, I suggest dropping it.

I see your point and I agree.
Let me know if the ktime -> Unix time conversion utility function could be useful somehow. I'd leave it up to the user and wouldn't insert it as part of the library.

dylandreimerink · 2024-02-26T13:26:11Z

map.go

+	// For Queue/Stack map block the iteration after maxEntries
+	// to avoid potential infinite loops
+	// (values can be pushed to map while doing pop)
+	if mi.count == mi.maxEntries {
+		mi.err = fmt.Errorf("%w", ErrIterationAborted)
+		return false
+	}
+
+	mi.count++


I don't think this check works as intended. If a user indeed uses a stack or queue as kernel to userspace mechanism, and is constantly adding new objects, then you would expect to iterate for more than the maxEntries of the map.

I think we can just leave this out.

I see. In that way, is the user that decides whether to stop after certain iterations or not.

dylandreimerink · 2024-02-26T13:30:54Z

map.go

+	if mi.target.typ.isQueueStack() {
+		return mi.nextQueueMap(valueOut)
+	}
+	return mi.next(keyOut, valueOut)


I think that we should implement this slightly differently. Now that we have multiple iterator implementations (and I expect more to come), it makes sense to start using an interface with multiple implementations instead.

I suggest renaming MapIterator, and making it unexported. Then add a MapIterator interface with the Err() error and Next(keyOut, valueOut interface{}) bool methods. And add your logic as an new stack/queue iterator struct.

Then make Map.Iterate pick the correct iterator.

That is perfectly fine by me. I'm going to create two MapIterator implementations, one for key-value maps keyValueMapIterator and one for value-only maps keylessMapIterator . Correct me if the naming is horrible.

Concerning the PerCpu maps, I'd leave as it is, so that the old MapIterator (hence the newly keyValueMapIterator) handles this case.

dylandreimerink · 2024-02-26T13:34:21Z

map_test.go

+func TestMapStack(t *testing.T) {
+	testutils.SkipOnOldKernel(t, "4.20", "map type stack")
+
+	m, err := NewMap(&MapSpec{
+		Type:       Stack,
+		ValueSize:  4,
+		MaxEntries: 2,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer m.Close()
+
+	for _, v := range []uint32{42, 4242} {
+		if err := m.Put(nil, v); err != nil {
+			t.Fatalf("Can't put %d: %s", v, err)
+		}
+	}
+
+	var (
+		v  uint32
+		v2 uint32
+	)
+	if err := m.Lookup(nil, &v); err != nil {
+		t.Fatal("Lookup (Peek) on Stack:", err)
+	}
+
+	if err := m.Lookup(nil, &v2); err != nil {
+		t.Fatal("Lookup (Peek) consecutive on Stack:", err)
+	}
+
+	if v != v2 {
+		t.Fatal("Lookup (Peek) value removal from Stack:")
+	}
+
+	if v != 4242 {
+		t.Error("Want value 4242, got", v)
+	}
+	v = 0
+
+	if err := m.LookupAndDelete(nil, &v); err != nil {
+		t.Fatal("Can't lookup and delete element:", err)
+	}
+	if v != 4242 {
+		t.Error("Want value 4242, got", v)
+	}
+
+	v = 0
+	if err := m.LookupAndDelete(nil, unsafe.Pointer(&v)); err != nil {
+		t.Fatal("Can't lookup and delete element using unsafe.Pointer:", err)
+	}
+	if v != 42 {
+		t.Error("Want value 42, got", v)
+	}
+
+	if err := m.LookupAndDelete(nil, &v); !errors.Is(err, ErrKeyNotExist) {
+		t.Fatal("Lookup and delete on empty Stack:", err)
+	}
+
+	if err := m.Lookup(nil, &v); !errors.Is(err, ErrKeyNotExist) {
+		t.Fatal("Lookup (Peek) on empty Stack:", err)
+	}


I feel like this test is more about testing how the map type is implemented than it does the library. Something that is more at home in the kernel selftests. Same goes for the additions in the queue test. I would simply not bother with these.

On the other hand, I see no tests for the new iteration code. That is what I am interested in seeing, to check that the iterator actually works and stays working.

Agree, proceeding this way 👍

I would just leave one additional lookup (peek) test on the Queue for completeness, but please let me know if that is not needed.

s41m0n · 2024-02-29T13:59:47Z

Thank you very much for putting in the work. Though I would like to request some changes.

Many thanks again Dylan for the feedback!

… + tests Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>

ti-mo

Thanks for the patches! I'm not completely convinced we want to make MapIterator an interface (yet).

ti-mo · 2024-03-18T09:18:19Z

map.go

+
+	// Next decodes the next key and value.
+	//
+	// In case of a value-only map (Queue and Stack), the key


This is an implementation detail, not something the caller needs to take into account explicitly to correctly use the interface. Semantically, keyOut doesn't seem different from regular kv maps, so I'd drop this paragraph.

Yes, removing it in the next commit, thanks!

ti-mo · 2024-03-18T09:20:42Z

map.go

+	//
+	// The method must be called after Next returns nil.
+	//
+	// For key-value maps, returns ErrIterationAborted if


I'd remove the 'for key-value maps' part. The interface shouldn't document the implementation. Generally, ErrIterationAborted means a full iteration wasn't possible.

I agree, probably too many comments aren't needed in this case.

ti-mo · 2024-03-18T09:29:43Z

map.go

+// (Queue and Stack)
+//
+// See Map.Iterate.
+type keylessMapIterator struct {


I'm not 100% convinced that making MapIterator an interface is currently worth it. Only next() is fundamentally different, and the kv vs. keyless split feels somewhat arbitrary. If we were planning to add many more different implementations, it would make more sense, but currently this is not being discussed.

See my comment on keylessMapIterator.next() below.

I think that was how the code was structured originally, until Dylan suggested using an interface (after discussing it with me). I guess it doesn't make much difference either way.

I agree that it does not make much difference right now. Planning to support future MapIterators, probably the interface solution would be a clean way to go. But I'm not sure whether there will be new Iterators to support. Let me know!

ti-mo · 2024-03-18T09:31:37Z

map.go

 //
 // Returns false if there are no more entries. You must check
 // the result of Err afterwards.
+func (mi *keylessMapIterator) next(_, valueOut interface{}) bool {


I'd name this MapIterator.pop() and make MapIterator.Next() call next() or pop() depending on the outcome of isQueueStack().

ti-mo · 2024-03-18T09:33:44Z

types.go

@@ -102,6 +102,12 @@ func (mt MapType) hasPerCPUValue() bool {
 	return mt == PerCPUHash || mt == PerCPUArray || mt == LRUCPUHash || mt == PerCPUCGroupStorage
 }

+// isQueueStack returns true if the Map is a Queue (BPF_MAP_TYPE_QUEUE)
+// or Stack (BPF_MAP_TYPE_STACK)
+func (mt MapType) isQueueStack() bool {


This is a shortcut for the map's type instead of its semantics. Maybe needsPop() would be more descriptive?

Agree, going to change it in the next commit!

lmb

I have a question regarding semantics:

What happens when you do a plain Lookup on a queue or stack? Does that function like a peek? Or does it return an error?
Are there other map types which support LookupAndDelete? Are there ones which support passing a Key argument?

lmb · 2024-03-21T11:26:00Z

map.go

+// (Queue and Stack)
+//
+// See Map.Iterate.
+type keylessMapIterator struct {


I think that was how the code was structured originally, until Dylan suggested using an interface (after discussing it with me). I guess it doesn't make much difference either way.

s41m0n · 2024-05-06T13:26:38Z

Thanks to all the reviewers for the feedback.
I guess the only thing left to decide is whether to proceed with one of the two proposed solutions:

Creating a simple method pop that would be called instead of next, as for the solution proposed in the previous commit.
Creating an interface to support multiple MapIterators, as for the current commit under review.

lmb · 2024-05-14T15:35:35Z

Have you had the time to look into my questions in #1349 (review) ? I think they will help us decide what the implementation looks like.

s41m0n · 2024-05-15T09:51:56Z

Have you had the time to look into my questions in #1349 (review) ? I think they will help us decide what the implementation looks like.

@lmb I apologize, I missed the message.

According to https://docs.kernel.org/bpf/map_queue_stack.html and some quick tests of the code, Queues/Stack maps support the following operations.
- lookup (peek): returns the value at the head without removing it. If no value exists, ErrKeyNotExist is returned.
- update (push)
- delete (pop)
  In addition, batch operations are not supported. Also, NextKey is not supported: it either returns can't marshal key: <type> doesn't marshal to 0 bytes or next key: invalid argument when called with nil (these maps have a key of size 0).
On the other hand, other maps like Array and Hash support LookupAndDelete, and the method requires a non-null key as a parameter.

dylandreimerink · 2024-05-16T10:17:07Z

Queues/Stack maps support the following operations.

Clarification, for stacks/queues:
BPF_MAP_LOOKUP_ELEM -> peek
BPF_MAP_LOOKUP_AND_DELETE_ELEM -> pop
BPF_MAP_UPDATE_ELEM -> push

On the other hand, other maps like Array and Hash support LookupAndDelete, and the method requires a non-null key as a parameter.

Only hash maps(and variants: LRU, PER_CPU, LRU_PER_CPU) have support for BPF_MAP_LOOKUP_AND_DELETE_ELEM since v5.14. Array maps don't seem to support it since you can't delete from an array map.

Also interesting is that BPF_MAP_LOOKUP_AND_DELETE_BATCH which is the batch version is only supported by hash maps and not by stacks or queues and was added before the non-batch version was supported in v5.6.

Are there ones which support passing a Key argument?

Yes, the hashmap supports passing a key value. Actually, even before v5.14, a valid pointer must be provided to the key field. The kernel always checks it and copies it into kernel memory, just to discard it. After v5.14 it of course starts to actually get passed to the map implementation.

lmb · 2024-05-17T10:27:05Z

Okay! That was very helpful, thanks to both of you. The peek operations only return the top of the stack / first item in the queue? And currently doing .Iterate() on a queue or stack doesn't work at all since we pass a key?

I'm currently leaning towards adding a new method Map.Drain() *MapIterator. Initially this could only support queues / stacks but later on we may be able to extend it to hash maps. The benefit of a new method is that it makes it clearer that we're actually deleting items here. We can put the implementation into the same MapIterator struct for now as Timo suggested. I think that we should take MapIterator.Next(nil, &value) when draining (and not ignore the key argument as currently done.)

dylandreimerink · 2024-05-17T11:09:57Z

The peek operations only return the top of the stack / first item in the queue? And currently doing .Iterate() on a queue or stack doesn't work at all since we pass a key?

Yes, peeking only returns the top of the stack. Any key provided will be ignored as far as I understand. But as @s41m0n mentioned the BPF_MAP_GET_NEXT_KEY op would return an error if using the normal iterator.

s41m0n force-pushed the feature/queue_stack_support branch from d65df8e to 9a2089a Compare February 19, 2024 18:06

dylandreimerink requested changes Feb 26, 2024

View reviewed changes

s41m0n force-pushed the feature/queue_stack_support branch 2 times, most recently from 11c813b to 2dfaef8 Compare February 29, 2024 13:52

refactored MapIterator using an interface to support also Queue/Stack…

23c82af

… + tests Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>

s41m0n force-pushed the feature/queue_stack_support branch from 2dfaef8 to 23c82af Compare March 13, 2024 19:42

s41m0n changed the title ~~MapIterator support for Queues/Stacks + further testing + example~~ MapIterator support for Queues/Stacks + further testing Mar 13, 2024

s41m0n marked this pull request as ready for review March 13, 2024 19:45

s41m0n requested a review from a team as a code owner March 13, 2024 19:45

ti-mo requested changes Mar 18, 2024

View reviewed changes

lmb reviewed Mar 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MapIterator support for Queues/Stacks + further testing #1349

MapIterator support for Queues/Stacks + further testing #1349

s41m0n commented Feb 19, 2024

dylandreimerink left a comment

dylandreimerink Feb 26, 2024

s41m0n Feb 28, 2024

dylandreimerink Feb 26, 2024

s41m0n Feb 28, 2024

dylandreimerink Feb 26, 2024

s41m0n Feb 28, 2024 •

edited

dylandreimerink Feb 26, 2024

s41m0n Feb 28, 2024 •

edited

s41m0n commented Feb 29, 2024

ti-mo left a comment

ti-mo Mar 18, 2024

s41m0n May 6, 2024

ti-mo Mar 18, 2024

s41m0n May 6, 2024

ti-mo Mar 18, 2024

lmb Mar 21, 2024

s41m0n May 6, 2024

ti-mo Mar 18, 2024 •

edited

ti-mo Mar 18, 2024

s41m0n May 6, 2024

lmb left a comment

lmb Mar 21, 2024

s41m0n commented May 6, 2024

lmb commented May 14, 2024

s41m0n commented May 15, 2024

dylandreimerink commented May 16, 2024

lmb commented May 17, 2024

dylandreimerink commented May 17, 2024

MapIterator support for Queues/Stacks + further testing #1349

Are you sure you want to change the base?

MapIterator support for Queues/Stacks + further testing #1349

Conversation

s41m0n commented Feb 19, 2024

MapIterator support for Queue/Stack maps

Extended tests for Queue/Stack maps

Example of using a Queue map

Further Proposal

dylandreimerink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s41m0n Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s41m0n Feb 28, 2024 • edited

Choose a reason for hiding this comment

s41m0n commented Feb 29, 2024

ti-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-mo Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s41m0n commented May 6, 2024

lmb commented May 14, 2024

s41m0n commented May 15, 2024

dylandreimerink commented May 16, 2024

lmb commented May 17, 2024

dylandreimerink commented May 17, 2024

s41m0n Feb 28, 2024 •

edited

s41m0n Feb 28, 2024 •

edited

ti-mo Mar 18, 2024 •

edited