Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge max-next into next #998

Merged
merged 28 commits into from
Aug 29, 2016
Merged

Merge max-next into next #998

merged 28 commits into from
Aug 29, 2016

Conversation

eugeneia
Copy link
Member

This includes: #966 #985 #984 #986 #990 #991 #988

eugeneia and others added 25 commits May 18, 2016 16:50
# Conflicts:
#	src/lib/ipsec/esp.lua
@lukego
Copy link
Member

lukego commented Aug 29, 2016

@kbara @eugeneia There are some more implications to consider with packet.clone_to_memory() (#999). Specifically I do not think that this implementation satisfies the stated goal:

The point is to have a fully valid struct packet at the end

The thing is that packet.allocate() is able to allocate memory "under the hood" in such a way that it satisfies specific invariants that other code can then depend on. For example, packet.allocate() ensures that packet memory is suitable for DMA by always allocating physically contiguous memory (HugeTLB) that is pinned with mlock() (can't be swapped out / relocated / lazy-allocated). We also apply a tag scheme where the virtual address of the packet data is a tagged pointer to the physical address.

If we want to support a packet.clone_to_memory() that creates a genuine struct packet in user-supplied memory then we need to account for these invariants in some way.

Some options:

  1. Document all of the invariants and require the memory supplied to clone_to_memory() to satisfy them.
  2. Define multiple classes of packets e.g. those that are known to satisfy invariants (DMA-safe, etc) vs those that are not.
  3. Eliminate the invariants e.g. use the IOMMU to make all memory suitable for packets.

Or the default option would be stick with the status quo and say that the only way to get a struct packet is to call packet.allocate() and that when you copy the payload somewhere else that is okay but it is not a struct packet.

Somehow resolving this issue is a blocker to merging packet.clone_to_memory().

@kbara
Copy link
Contributor

kbara commented Aug 29, 2016

Interesting points. As currently implemented, the code this is being used for does not satisfy those invariants. (The use case is hardened reassembly, where a large, predetermined number of packets are getting reassembled in a ctable at the same time, as fragments come in.) The underlying ctable is large enough (around half a gigabyte with the default parameters I've set - but the same applies down to surprisingly small sizes) that hugetlb mmap fails and we fall back to using non-hugetlb mmap.

Specifically, the value of the ctable includes an embedded struct packet, which is used for in-place reassembly, then cloned to a normal packet when that is complete. (The logic of how clone_to_memory came to be is documented in Igalia#393 - but basically, Igalia is using a more complicated struct packet, that needs to have book-keeping done, the right place to do the book-keeping is in packet.lua, and upstreaming the APi makes the fragmentation reassembly code closer to neutral with regards to the struct packet implementation being used).

What would be your preferred approach to dealing with hundreds of thousands of packets at a time, since in the case of fragmentation their lifecycle can be quite long? Would you simply dispense with using a 'struct packet' at all during reassembly, and use an identical struct without the special semantics?

@lukego
Copy link
Member

lukego commented Aug 29, 2016

I think the simplest solution would be to have two separate data structures:

  • Short lived struct packet objects used for data during the breath that it is transmitted or received.
  • Long lived memory in a custom format for data that is awaiting reassembly.

The custom format need not be the same as struct packet e.g. you could use scatter-gather lists if that were more suitable, etc.

(Generally speaking it is okay for struct packet objects to be long-lived too but since each one consumes 10KB of HugeTLB memory I can understand why that would not fit this particular use case.)

@kbara
Copy link
Contributor

kbara commented Aug 29, 2016

Agreed. In that case, let's forget this API ever existed. :-)

@lukego lukego merged commit 2a6fea9 into next Aug 29, 2016
lukego added a commit that referenced this pull request Aug 29, 2016
lukego added a commit that referenced this pull request Aug 29, 2016
This reverts commit bb9b3c2.

See discussion at #998 (comment)
@lukego
Copy link
Member

lukego commented Aug 29, 2016

Groovy! Merged now with that API reverted in commit 54ac86d. Thanks everybody!

@kbara
Copy link
Contributor

kbara commented Aug 30, 2016

Excellent. I've gotten rid of it downstream, too. :-)

wingo pushed a commit that referenced this pull request Nov 28, 2017
Make "snabbvmx top" print out only counters that are defined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants