net_packet.c (legacy and sptps)

[splitice]

Any objections to moving all legacy protocol communication to a file i.e protocol/legacy.c and moving sptps to protocol/sptps*.c?

That would leave net_packet.c for common functions (currently not common) interacting with the lower level send/sendto/recv/recvfrom and vpn_packet_t

protocol.c might also be a good idea to provide dispatcher functions.

So the call chain would look something like

receive_udppacket (protocol.c) -> legacy_receive_udppacket (protocol/legacy.c) -> net_receive_packet (net_packet.c)

This would reduce the size of net_packet.c considerably making it easier to implement some capabilities needed for packet buffering and vhost-net.

[splitice]

While I'm getting oppinions it would be extremely benificial to vhost-net and packet buffering if the lifetime of the vpn_packet_t (better name would be net_packet_t soon) could be taken out of the stack.

Ideally I would like to:

• rename it to net_packet_t and use it for sptps data
• reference count net_packet_t with an allocation pool
• perform protocol detection only once for continuous valid packets. protocol_t set in node_t for consecutive valid packets to use. Invalid packet clears this detected protocol, perhaps by setting the protocol_t to one defined protocol/detect.c (also the default)?
• use net_packet_t as the primative for all transmissions, pkt->data+pkt->offset would always be the start of transmission after being passed to net_transmit_packet regardless of protocol (eliminating the SEQNO macro calls for tx offset). Protocol will set the offset prior to sending to net_packet tx functions.

Goals not limited to just performance, but also in improving the abstraction between legacy and sptps.

[gsliepen]

Any objections to moving all legacy protocol communication to a file i.e protocol/legacy.c and moving sptps to protocol/sptps*.c?

That sounds like a good idea.

While I'm getting oppinions it would be extremely benificial to vhost-net and packet buffering if the lifetime of the vpn_packet_t (better name would be net_packet_t soon) could be taken out of the stack.

Sure.

reference count net_packet_t with an allocation pool

If we can do it without reference counting, that would be even better. If we just make sure there's a single owner of each packet?

perform protocol detection only once for continuous valid packets. protocol_t set in node_t for consecutive valid packets to use.

Just so you know, if a target node is not directly reachable, packets will be forwarded via intermediate nodes which might use a different version of the protocol.

use net_packet_t as the primative for all transmissions, pkt->data+pkt->offset would always be the start of transmission after being passed to net_transmit_packet regardless of protocol (eliminating the SEQNO macro calls for tx offset). Protocol will set the offset prior to sending to net_packet tx functions.

That sounds good as well.

[splitice]

If we can do it without reference counting, that would be even better. If we just make sure there's a single owner of each packet?

That might be possible, the simple select,rcvfrom,write(tap) path (no vhost, no packet buffer) might not be as efficient but a simple pool allocator so at-least 2 packet buffered that can be swapped should eliminate most of the cost.

Currently are a few possible places currently where packets are reformed (memcpy, decompression, reforming the packet i.e relay) and its quite unclear who owns a packet in the end. By reference counting we can make it all very simple. I expect the peak refcnt will be 2. I guess we can assert that sending a packet transfers ownership (is there anywhere that needs to use a packet after transmission?) instead of refcounting.

With vhost-net I need to be able to put the packets into a circular queue that is consumed out of sync of the main loop, so ownership needs to be transferred to the vhost-net layer on send (decap egress). Packet buffers (for enap egress) should transmit at the end of the loop on the otherhand are simpler and can be assumed to be transmitted at the end of loop (or at-least before any other subsequent loop if buffer full)

vhost-net rx (decap ingress) needs a circular buffer to feed from as well. So a pool of packets would be great. recvmmsg (encap ingress) could also benefit from such a pool (with unused buffers immediately returned).

Just so you know, if a target node is not directly reachable, packets will be forwarded via intermediate nodes which might use a different version of the protocol.

I havent thought through how to handle that yet. Have you considered that that might not be desirable (protocol downgrade vulnerability)?

It shouldnt really matter as long as the receiving protocol allocates enough headspace for any transmitting protocol to fill (DEFAULT_PACKET_OFFSET currently).

From my undestanding when handle_incoming_vpn_packet receives a legacy packet it looks to see if we are the targetted node if not, tries to find the target. If that target is 1.1 it will get sent via send_sptps_data which will add a to & from node id to the packet. Currently this would require a memcpy.

Is there more reforming of the packet than what I've found for these cross protocol interactions? A relay.c would be really nice to move all the relay logic out of the transmit and receive logic.

Perhaps the received packet should be a struct with a union, say:

typedef struct net_packet_t
union {
struct {
uint32_t unused1;
node_id_t src;
node_id_t dst;
} sptps;
struct {
node_id_t src;
node_id_t dst;
uint32_t seqno;
} sptps_legacy;
struct {
uint32_t unused1;
uint32_t unused2;
uint32_t unused3;
uint32_t seqno
} legacy;
} hdr;
uint8_t payload[MAX_PAYLOAD]
} net_packet_t;

typedef struct net_buffer_t {
length_t len;
// priority etc;
net_packet_t packet;
} net_buffer_t;

No memcpy for the payload then. I'm not sure if there is a nicer way to right align structs (thats what the unused* members are).

[splitice]

Also perhaps instead of an offset we simply include a protocol field, the offset can be inferred from this (and there is value in knowing the contents of the packet).

[gsliepen]

Just so you know, if a target node is not directly reachable, packets will be forwarded via intermediate nodes which might use a different version of the protocol.

I havent thought through how to handle that yet. Have you considered that that might not be desirable (protocol downgrade vulnerability)?

This is not a protocol downgrade vulnerability; 1.1 packets routed via 1.0 nodes still have end-to-end security. It just piggy-backs SPTPS-encrypted packets over the legacy 1.0 protocol.

Is there more reforming of the packet than what I've found for these cross protocol interactions? A relay.c would be really nice to move all the relay logic out of the transmit and receive logic

I would not worry about inefficiencies in mixed 1.0 and 1.1 networks. The goal is just to provide a reasonable upgrade path. So we should only worry about making communication between 1.1 nodes efficient and using as little memcpy()s as necessary.

[splitice]

@gsliepen Maskes sense.

I think the tasks for this should involve. Agree?

1. Merge epoll & AES (and any other big commits that won't merge easily after)
2. Small PR to move/split legacy protocol and sptps
3. Bigger PR for moving packets out of stack

2 is the step that will really make merging difficult for any in progress PR.

[gsliepen]

Sorry for the late response. But yes, do the improvements in step 1 first and get it released perhaps, then do cleanups for the next release.