Packet socket: mmapped IO: PACKET_TX_RING

There's no need to keep the index (packet_index). Just store the

Also, I don't think setting TP_STATUS_COPY is necessary since the user can't really do anything with that information. Simply leave it at TP_STATUS_USER & TP_STATUS_KERNEL.

The atomic_inc of pending_skb should happen after the skb was allocated - the atomic_dec in out_status can also be removed. out_status can be removed completely if you get rid of TP_STATUS_COPY. If you leave it, you still need the barriers as above after changing tp_status, or the user may not see the change.

Also, you've got a potential threading issue - you're not protecting the frame index behind the spinlock.

Also, when you set the status back to TP_STATUS_KERNEL in the

to add the following barriers:

__packet_set_status(po, ph, TP_STATUS_KERNEL);
smp_mb(); // make sure the TP_STATUS_KERNEL was actually written to
memory before this - couldn't this actually be just a smp_wmb? flush_dcache_page(virt_to_page(ph)); // on non-x86 architectures like ARM that have a moronic cache (i.e cache by virtual rather than physical address). on x86 this is a noop.

Also, I think that I looked at your code while working on my version and there may have been some logical problems with the way you're building packets. I'll compare it within the next few days as I start cleaning up my code.

As a benchmark on a 10G card (and not performing any optimizations like using syspart & dedicating a cpu for tx), I was able to hit 8.6 GBits/s using a dedicated kernel thread for the transmission. With the dedicated CPU, I'm confident the line-rate will go up significantly

.I'll try to test your changes within the next few days. TCPdump maxes out at around 1.5 GBits/s

As for CPU usage, there's a noticeable advantage to traditional send. Using tcpreplay - there's about 80% CPU utilization when sending. Using the tx ring, there's maybe 10-15%.

I'm going to have latency numbers soon as well (i.e. how much jitter is introduced by the kernel).

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: October-31-08 3:58 AM
To: Lovich, Vitali
Cc: David Miller; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali,

Quoted 1 line

There's no need to keep the index (packet_index). Just store the

pointer directly (change it to void *) - saves an extra lookup.

Indeed, it will be faster. I'll do the change.

Quoted 2 lines

Also, I don't think setting TP_STATUS_COPY is necessary since the

user

Quoted 2 lines

can't really do anything with that information. Simply leave it at TP_STATUS_USER & TP_STATUS_KERNEL.

This information is not useful for user. It is to prevent kernel from sending a packet twice or more. Inside the tx ring lock, queued packets must be tagged as "Kernel has already handled this packet" to not send it again at next turn of tx ring.
(That case can happen if device/queue is very slow or if you have only few frames)

Quoted 5 lines

The atomic_inc of pending_skb should happen after the skb was allocated - the atomic_dec in out_status can also be removed. out_status can be removed completely if you get rid of TP_STATUS_COPY. If you leave it, you still need the barriers as

above

Quoted 1 line

after changing tp_status, or the user may not see the change.

I don't understand why "atomic_inc of pending_skb should happen after the skb was allocated". This counter is used to monitor the number of TX
packets queued. So as requirement, we have to increment it before dev_queue_xmit().

atomic_dec() will be needed anyway if tpacket_fill_skb() or dev_queue_xmit() are failing (If performed after skb alloc).

Quoted 2 lines

Also, you've got a potential threading issue - you're not protecting the frame index behind the spinlock.

You are right, I think I will spin-lock outside the do_while loop.

Quoted 2 lines

Also, when you set the status back to TP_STATUS_KERNEL in the

destructor, you need

Quoted 6 lines

to add the following barriers:

__packet_set_status(po, ph, TP_STATUS_KERNEL);
smp_mb(); // make sure the TP_STATUS_KERNEL was actually written to
memory before this - couldn't this actually be just a smp_wmb? flush_dcache_page(virt_to_page(ph)); // on non-x86 architectures

like

Quoted 3 lines

ARM that have a moronic cache (i.e cache by virtual rather than physical address). on x86 this is a noop.

So, If my understanding of those memory barriers is correct, we should have a smp_rmb() before status reading and smp_wmb() after status writing in skb destructor and send procedure.

-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of Lovich, Vitali
Sent: October-31-08 10:08 AM
To: Johann Baudy
Cc: [email protected]
Subject: RE: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Quoted 9 lines

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: October-31-08 3:58 AM
To: Lovich, Vitali
Cc: David Miller; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali,

Quoted 4 lines

Quoted 1 line

There's no need to keep the index (packet_index). Just store the

pointer directly (change it to void *) - saves an extra lookup.

Indeed, it will be faster. I'll do the change.

Actually, I just realized that we don't need to modify the sock_buff structure. We're already filling in the fragments of the skb - just take the first fragment and reverse the process to determine the starting address of the frame.

Here's the code I came up with to determine the offset - anyone see any problems?

union {
   tpacket2_hdr *t2;
   u8 *raw;
}

struct page *first_frag;
u32 page_offset;

first_frag = skb_shinfo(skb)->frags[0].page; page_offset = skb_shinfo(skb)->frags[0].page_offset;

page_offset -= (skb_headlen(skb) + sizeof(struct tpacket2_hdr));

if (unlikely(page_offset < 0)) {
	first_frag--;
	page_offset += PAGE_SIZE;
}

frame.raw = page_address(first_frag) + page_offset;

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-04-08 2:46 PM
To: Lovich, Vitali
Cc: [email protected]; Evgeniy Polyakov
Subject: RE: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

This won't work if your network card doesn't support Scatter/Gather IO. Indeed, when SG is not supported, fragmented packets are linearized after dev_queue_xmit() call. Thus addresses of pages are then lost.

Best regards,
Johann

In either case, the skb given to the destructor will still have the correct values for fragments we specified. Of course, this is based on 2 assumptions:

1. Nothing further down the line won't add fragments, thereby overwriting frags[0]
2. No-one writes to frags[0].page & frags[0].page_offset

1 is reasonable because since the only reason we would be linearizing in the first place is if the device doesn't support scatter/gather, so it would be strange for something down the line to add more fragments that would have to be linearized anyways.

2 is reasonable since it would only make sense if something down the line used this as a temporary variable storage, which again should be unlikely.

Another approach may be to store it in the cb as we had done so originally, except with skb_clone to ensure no other layers overwrite it, although I'm not 100% sure of the implications of skb_clone has for things like the cb & users fields and kfree_skb.

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-06-08 12:03 AM
To: Lovich, Vitali
Cc: Johann Baudy; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali.

Quoted 1 line · Vitali

In either case, the skb given to the destructor will still have the

correct values for fragments we specified. Of course, this is based on 2 assumptions:

Quoted 2 lines

1. Nothing further down the line won't add fragments, thereby

overwriting frags[0]

Quoted 3 lines

2. No-one writes to frags[0].page & frags[0].page_offset

1 is reasonable because since the only reason we would be linearizing

in the first place is if the device doesn't support scatter/gather, so it would be strange for something down the line to add more fragments that would have to be linearized anyways.

Quoted 1 line

Quoted 1 line

2 is reasonable since it would only make sense if something down the

line used this as a temporary variable storage, which again should be unlikely.

What if skb was queued in hardware or qdisk and userspace rewrites mapped page placed in fraglist?

Quoted 2 lines

Another approach may be to store it in the cb as we had done so

originally, except with skb_clone to ensure no other layers overwrite it, although I'm not 100% sure of the implications of skb_clone has for things like the cb & users fields and kfree_skb.

It is not allowed to store something in cb block which is intended to live while skb is processed on different layers. In this particular case
qdisk engine will overwrite skb's cb.

Quoted 2 lines

What if skb was queued in hardware or qdisk and userspace rewrites mapped page placed in fraglist?

Can you please clarify what you mean? Are you talking about the user changing the status field after the skb was queued? Or are you saying that the user may ask the kernel to resize the ring buffer while the skb is queued?

In the first case, I don't think we don't really care - the user is breaking their side of the contract, but we'll still have a valid address since the page pointer should still be valid.

In the second case, the behaviour is undefined because the hardware would use those page pointers for its scatter-gather which would, best case, cause the skb to fail (kernel oops is more likely I think). We can add this to the documentation, but in the end there's nothing we can really do outside of copying data from the ring buffer into the skb, which defeats the point of doing this.

Quoted 5 lines

It is not allowed to store something in cb block which is intended to live while skb is processed on different layers. In this particular case
qdisk engine will overwrite skb's cb.

That's what I figured - however, I was wondering if we even need to go through the qdisc engine. Couldn't we just bypass it and send out packets directly through the card (i.e. dev_hard_start_xmit) similar to how pktgen does this? I haven't looked at the documentation (I'm not actually sure with which to start), but are there any caveats that need to be considered (e.g. must be called outside of an interrupt, can't be interruptible, etc.)

The reason I'm thinking we'd want to bypass the qdisc engine is that the idea is that you would use this method only if you want the most performance possible, so why is packet scheduling even needed? The approach I'm thinking is that if the tx-ring buffer is implemented, we can even get rid of the in-kernel pktgen and port it over to user-space as an example of how to utilize the tx-ring buffer.

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-06-08 11:41 AM
To: Lovich, Vitali
Cc: Johann Baudy; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali.

Quoted 3 lines · Vitali

Quoted 2 lines

What if skb was queued in hardware or qdisk and userspace rewrites mapped page placed in fraglist?

Can you please clarify what you mean? Are you talking about the user

changing the status field after the skb was queued? Or are you saying that the user may ask the kernel to resize the ring buffer while the skb is queued?

Quoted 2 lines

In the first case, I don't think we don't really care - the user is

breaking their side of the contract, but we'll still have a valid address since the page pointer should still be valid.

Quoted 2 lines

In the second case, the behaviour is undefined because the hardware

would use those page pointers for its scatter-gather which would, best case, cause the skb to fail (kernel oops is more likely I think). We can add this to the documentation, but in the end there's nothing we can really do outside of copying data from the ring buffer into the skb, which defeats the point of doing this.

I'm not sure that it is allowed to fail even for root. System could sleep and not permit the resize until all skbs are freed though.

Quoted 2 lines

Quoted 2 lines

It is not allowed to store something in cb block which is intended

to

Quoted 5 lines

Quoted 4 lines

live while skb is processed on different layers. In this particular case
qdisk engine will overwrite skb's cb.

That's what I figured - however, I was wondering if we even need to

go through the qdisc engine. Couldn't we just bypass it and send out packets directly through the card (i.e. dev_hard_start_xmit) similar to how pktgen does this? I haven't looked at the documentation (I'm not actually sure with which to start), but are there any caveats that need to be considered (e.g. must be called outside of an interrupt, can't be interruptible, etc.)

Quoted 2 lines

The reason I'm thinking we'd want to bypass the qdisc engine is that

the idea is that you would use this method only if you want the most performance possible, so why is packet scheduling even needed? The approach I'm thinking is that if the tx-ring buffer is implemented, we can even get rid of the in-kernel pktgen and port it over to user-space as an example of how to utilize the tx-ring buffer.

I can not say if it is good or bad idea, it is not my project :) But existing packet socket does not bypass qdisc, so users may expect the same from the new interface. Overall idea of storing something in cb
and expect it to live there between layers is very subtle: what if tomorrow we will gen new layer between driver and qdisc? Or if we will start updating cb in the driver itself when it owns skb?

Which brings up a possible bug in the proposed patch - the mapped atomic int field should be part of the struct containing a ring buffer - each ring buffer has a different number of mappings. Otherwise, there will be issues trying to create both a tx & rx ring.

Are there any consequences to bypassing qdisc (other than not participating in any load balancing)? Also, since any users using the tx ring approach would have to write new code, it would be part of the documentation that the send behaviour is optimized for the least latency and highest throughput, thus no guarantees and assumptions can be made about the path the data takes through the stack.

I agree with you about not using the cb - I was just looking at all possible alternatives of how this can be solved. I'm liking my approach of using the frag list because it's the least intrusive (doesn't require modification of the skb_buff structure) and the seems to be the most resilient (trying to hide data in some unused skb field and hope no one overwrites it)

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-07-08 8:36 AM
To: Lovich, Vitali
Cc: Evgeniy Polyakov; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Quoted 1 line

Which brings up a possible bug in the proposed patch - the mapped

atomic int field should be part of the struct containing a ring buffer
- each ring buffer has a different number of mappings. Otherwise, there will be issues trying to create both a tx & rx ring.

Could you please clarify?
tx_pending_skb is only used for TX process.

Quoted 2 lines

Are there any consequences to bypassing qdisc (other than not

participating in any load balancing)? Also, since any users using the tx ring approach would have to write new code, it would be part of the documentation that the send behaviour is optimized for the least latency and highest throughput, thus no guarantees and assumptions can be made about the path the data takes through the stack.

Quoted 2 lines

I agree with you about not using the cb - I was just looking at all

possible alternatives of how this can be solved. I'm liking my approach of using the frag list because it's the least intrusive (doesn't require modification of the skb_buff structure) and the seems to be the most resilient (trying to hide data in some unused skb field and hope no one overwrites it)

Quoted 1 line

My feeling is that all socket families have to use the same interface. We must not bypass it, otherwise we're gonna make a mess.

I think we must not care about skb frags/device management to get tpacket_hdr pointer back from skb. If it is changing tomorrow, we will be in trouble.

A generic solution could be to forward a void* argument as the skb destructor is forwarded through layers. Kind of:
skb->destructor(skb->desctructor_arg) when executing the callback. A backup copy of destructor/destruct_arg pair should be performed if someone needs to replace it.
This way all entities/layers could use this field to give an argument to their destructor without adding a new field in sk_buff struct.

Also, there's no need to support both header versions in the tx path - since v2 should be used anyways and there's no legacy (v1 headers are deprecated no?), we can use v2 only in the tx path.

Quoted 2 lines

You don't reset the tp_status flag when you reject a message for being too short. You incorrectly calculate to_write in the situation of sock->type == SOCK_DGRAM.

Quoted 2 lines

You don't check for the MAX_SKB_FRAGS condition.

Quoted 2 lines

Not your code, but in packet_set_ring, it's redundant to clear the status for all the frames to TP_STATUS_KERNEL (0) since the pages are allocated with kzalloc (pages are already zeroed). I surrounded this with a CONFIG_DEBUG_KERNEL block.

Quoted 2 lines

Earlier, I think I had said that the mapped variable should be placed in packet_ring_buffer struct - I was wrong. It's correct where it is. I had forgotten that rx & tx are mapped contiguously and simultaneously. There needs to be a note in the doc that the rings should be set up before mapping (i.e. cannot set up the rx or tx ring buffer first, mmap, and then try to change it).

Quoted 1 line

We do need still need a check for tx_pending_skb when sizing the ring buffer (in the situation where we unfreed skbs created by a call to send from Thread B, while Thread A unmaps the ring buffer and trys a resize). Also, in tpacket_snd, we need to increment mapped before allocating the skb and test if the original value was 0 (we decrement after incrementing tx_pending_skb). If it was, then that indicates that the user had unmapped the ring buffer, which means we should stop sending. No need for additional locking because of the atomic relationship this sets up between tx_pending_skb and mapped.

Ok, so we can skip below block according to this assumption:

		for (i = 0; i < req->tp_block_nr; i++) {
			void *ptr = pg_vec[i];
			int k;

			for (k = 0; k < rb->frames_per_block; k++) {
				__packet_set_status(po, ptr, TP_STATUS_KERNEL);
				ptr += req->tp_frame_size;
			}
		}

Quoted 2 lines

Earlier, I think I had said that the mapped variable should be placed

in packet_ring_buffer struct - I was wrong. It's correct where it is. I had forgotten that rx & tx are mapped contiguously and simultaneously. There needs to be a note in the doc that the rings should be set up before mapping (i.e. cannot set up the rx or tx ring buffer first, mmap, and then try to change it).

Yes, we can also add a check for po->mapped in PACKET_TX_RING/PACKET_RX_RING cases of packet_setsockopt() switch.

Quoted 3 lines

We do need still need a check for tx_pending_skb when sizing the ring

buffer (in the situation where we unfreed skbs created by a call to send from Thread B, while Thread A unmaps the ring buffer and trys a resize). Also, in tpacket_snd, we need to increment mapped before allocating the skb and test if the original value was 0 (we decrement after incrementing tx_pending_skb). If it was, then that indicates that the user had unmapped the ring buffer, which means we should stop sending. No need for additional locking because of the atomic relationship this sets up between tx_pending_skb and mapped.

I'm not sure to understand:

Let me, sum up :
* when setting or resizing the ring: we have to return an error if:
- someone has mapped the socket (1.a)
- sending is ongoing - tx_pending_skb != 0 (1.b)
- a send() procedure is ongoing (1.c)

* when closing the socket, we have to return an error if:
- sending is ongoing - tx_pending_skb != 0 (2.a)
- a send() procedure is ongoing (2.b)

* during do-while loop of send(),:
- protect ring against simultaneous send() (3.a)
- ensure that ring is not resized (3.b) (linked with 1.c) Our main issue is when: tx_pending_skb = 0, Thread A starts to send a packet while Thread B unmaps and resizes the ring.
Thread B can potentially resizes the ring until increment of
tx_pending_skb in Thread A.
- loop can continue even if ring is not mapped (3.c), this has no sense but it is not critical

* when executing the destructor callback:
- we must ensure that ring structure has not changed since send() (4.a)

(1.a) can be done with po->mmaped check in packet_setsockopt() (1.b) and (2.a) can be done as you said : tx_pending_skb != 0 check when sizing buffer in packet_set_ring()
(3.a) by a mutex M outside of the do-while loop
(1.c), (2.b) and (3.b) can be performed thanks to the same mutex M and mutex_trylock() in packet_set_ring(). An error is returned if we can't take the mutex . The mutex is released at the end of packet_set_ring() call.
(4.a) is validated with tx_pending_skb increment, (3.b),(1.b) and (2.a)

Do I miss something?

Found another issue. I'm not sure how your code managed to work, but in my patch I had to have skb_reset_network_header(skb) after a skb_reserve - otherwise, it would be rejected when I passed it on to the device.

Also, there's no need to support both header versions in the tx path - since v2 should be used anyways and there's no legacy (v1 headers are deprecated no?), we can use v2 only in the tx path.

Additionally, if the packet size is too long, the kernel has a responsibility to clear that flag (otherwise that slot will never free up).

Quoted 1 line

Additionally, if the packet size is too long, the kernel has a

responsibility to clear that flag (otherwise that slot will never free up).
I'm not sure to understand, could you please clarify ?

If the user places a 1 KB packet into the buffer, but then when we try to send it, it turns out the mtu is too small (or something like that), if we don't clear the flag back to TP_STATUS_KERNEL and just return an error, the user will never know.

     --------------------------------------
0    | 1 KB frame - status = TP_STATUS_USER |
     --------------------------------------
1    | 1 KB frame - status = TP_STATUS_USER |
     --------------------------------------

Etc

So when the user fills up the ring buffer and goes back to frame 0, he won't know that that packet was actually fatally rejected (i.e. the kernel moved on) and will just block waiting for the status to clear (which it never will). Similarly in the case where the packet gets rejected for being too small.

In other words, when the kernel doesn't send a packet but moves on and doesn't keep trying (as would be the case for the packet size being too small or too big because that is considered an irrecoverable error), it has to notify the user by clearing that status flag for that frame.

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-11-08 10:08 AM
To: Lovich, Vitali
Cc: Evgeniy Polyakov; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

If there is an error in packet_skb_fill, it returns a negative value as error code. If a negative value is returned from packet_skb_fill we exit send() procedure and set status of packet to TP_STATUS_USER. (this is how error is handled in my last code (not sure committed one)).
Do you want to introduce a new packet status that marks packet as ERROR (with TP_STATUS_LOOSING for example?). This way user will be able to determine easily the one which is failing?

PS: I need to test my last code at office and I'll send it.

I'm saying that that frame's status should just be set to TP_STATUS_KERNEL, not user. The kernel wants nothing to do with this frame any more and the user needs to know that. I was going to add statistics tracking to keep track of how many packets were sent vs rejected. This was more of an issue with my approach where I had a dedicated kernel thread doing all of this.

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-11-08 10:59 AM
To: Lovich, Vitali
Cc: Evgeniy Polyakov; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali,

Quoted 1 line

I'm saying that that frame's status should just be set to

TP_STATUS_KERNEL, not user. The kernel wants nothing to do with this frame any more and the user needs to know that. I was going to add statistics tracking to keep track of how many packets were sent vs rejected. This was more of an issue with my approach where I had a dedicated kernel thread doing all of this.

In all cases of error, Kernel is blocking on the same frame until update, indeed packet_increment_head() is only called on success. I've done this in order to not skip a packet in case of skb_alloc_send_skb error().

I believe, we have 3 options on all failure, if we set packet to:
- TP_STATUS_KERNEL: User can consider that buffer is free and it will fill it again.
If we choose TP_STATUS_KERNEL by default, that's means content is lost even for sock_alloc_send_skb() error. Such behavior is not acceptable, User is not able to show the difference between packet success and packet failure on a specific frame (same status).
- TP_STATUS_USER: Kernel will try to send it again and again, especially if send() is called in a loop (even it is impossible). This is current way of working, It looks like a manner to block on issue.
- TP_STATUS_LOOSING(or other): as TP_STATUS_USER with more info for user.
Whereas TP_STATUS_USER, this one provides user with the buffer where error has occurred.

Thanks,
Johann
--
Johann Baudy
[email protected]

So the way I had done it is that non-recoverable errors are those caused by malformed data coming from the user (i.e. packet size is invalid). In this case, we free the frame and return. In all other cases, it assumed that the cause was some kind of bad kernel state (i.e. no memory, device busy, etc) that will eventually be recovered from, so we keep retrying or return to the user without changing the status code.

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-12-08 4:10 AM
To: Lovich, Vitali
Cc: Evgeniy Polyakov; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

I still believe that we should stop on non-recoverable errors. User must be informed that the packet has not been sent.
If you skip the frame and set status to TP_STATUS_KERNEL. Frame will be lost and user will think that malformed frame has been transmitted.

I'm using TP_STATUS_LOSING in case of non-recoverable error. We can maybe introduce a socket option to chose the behavior for non-recoverable error.

Quoted 4 lines

I think we must not care about skb frags/device management to get tpacket_hdr pointer back from skb. If it is changing tomorrow, we will be in trouble.

I'm a bit confused about the point you are trying to make here. Could you please clarify?

This would require a massive change in the kernel to add a feature that up till now no one has needed. Certainly, I don't think it's wise to introduce such a wide-spread change when we could just as well accomplish it through other means. Here are my problems with this approach:

1. There's no need to actually pass the argument through the destructor - it could just be part of the skb (after all, since the callback only happens in one place, it's not like we could customize that argument - it would always be skb->destructor_arg). That way whoever needs to use would use it, but there's no need to change all existing destructor code.

2. Backing up this field isn't really possible since where would you place that backup?

3. You're increasing the size of the structure for everyone, even if they have disabled PACKET_MMAP (and since no-one else uses this, it doesn't really make sense). So if it is decided that the correct approach is to store the pointer in the skb, then we should continue doing what other protocols do in the situation where they need custom data in the skb and just add our own custom pointer surrounded by an #ifdef/#endif for PACKET_MMAP.

Quoted 1 line

2. Backing up this field isn't really possible since where would you

place that backup?

Ex: Destructor arg points to a struct that contains previous destructor function and destructor argument.
Those are restored and executed during new destructor. Layer that replaces it, is in charge of the backup

Quoted 2 lines

3. You're increasing the size of the structure for everyone, even if

they have disabled PACKET_MMAP (and since no-one else uses this, it doesn't really make sense). So if it is decided that the correct approach is to store the pointer in the skb, then we should continue doing what other protocols do in the situation where they need custom data in the skb and just add our own custom pointer surrounded by an #ifdef/#endif for PACKET_MMAP.

That's could be included into another "feature flag" which will be enabled only when a feature as PACKET_MMAP needs it.

I'm suggesting a variable that sub-layers will guarantee as skb->destructor. A variable that will be at least restored before destructor callback call.

This can be used also for sock_w_free() destructor. Current code is assuming that the socket destructor is always sock_w_free(), if tomorrow, this callback is changed or removed, we will have to change packet_mmap code (called in packet_skb_destruct()). Such mechanism should make replacement of destructor/destructor arg more independent toward layers.

Again, I'm not convinced this is 100% feasible. I understand, I think, what you're getting at. If you want to use the field, you defined a structure like struct my_arg { void *old, ... my data ... }. However, this isn't like C++ where the destructors for all layers are called. There is only 1 destructor for the layer that created the skb (AFAIK) - all the layers simply increment their reference to it (which is decremented by a free). Thus there's really no way to do what you want with minimal impact on existing code - the ideal behaviour would be to define a common struct (i.e. { void * olddata, void *thisdata }, that is filled with whatever data that layer needs when it increments the refcount and then have kfree_skb unwind the pointer as needed. This would require a massive chang

Quoted 7 lines

This can be used also for sock_w_free() destructor. Current code is assuming that the socket destructor is always sock_w_free(), if tomorrow, this callback is changed or removed, we will have to change packet_mmap code (called in packet_skb_destruct()). Such mechanism should make replacement of destructor/destructor arg more independent toward layers.

We're talking about skb destructors, not socket destructors. sock_wfree is not a destructor - it releases the resources allocated to the skb back to the socket that the resources are from.

Also, from what I can tell, this is how the Linux kernel works - if someone's change breaks the code, they're responsible for fixing it. Changing how sock_wfree is fairly significant enough that we don't need to worry about making it easier - it'll probably be so significant that there will be a lot of changes anyways. It's also highly unlikely to happen I think.

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-11-08 9:50 AM
To: Lovich, Vitali
Cc: Evgeniy Polyakov; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali,

You say "There is only 1 destructor for the layer that created the skb (AFAIK)", why not a "There is only 1 destructor argument for the layer that created the skb (AFAIK)"?
Where is the impact? only a void * destructor_arg that only the creator of skb is allowed to use. Moreover it can be put under feature flag.

Quoted 5 lines

Quoted 5 lines

This can be used also for sock_w_free() destructor. Current code is assuming that the socket destructor is always sock_w_free(), if tomorrow, this callback is changed or removed, we will have to

change

Quoted 2 lines

Quoted 2 lines

packet_mmap code (called in packet_skb_destruct()). Such mechanism should make replacement of destructor/destructor arg more

independent

Quoted 2 lines

Quoted 1 line

toward layers.

We're talking about skb destructors, not socket destructors.

sock_wfree is not a destructor - it releases the resources allocated to the skb back to the socket that the resources are from.
I agree, I'm just talking about destructor/destructor_arg pair.

Quoted 2 lines

Also, from what I can tell, this is how the Linux kernel works - if

someone's change breaks the code, they're responsible for fixing it. Changing how sock_wfree is fairly significant enough that we don't need to worry about making it easier - it'll probably be so significant that there will be a lot of changes anyways. It's also highly unlikely to happen I think.

I don't want to break this code. Just add a variable in sk_buff that can be used as the destructor is ! kind of CB[] that is not overwritten by sub layer ! or at least backuped.

Right, we're saying the exact same thing I think. I had wanted just a void * that is specific to PACKET_MMAP, so that no one else pays the price if they don't need to. You instead want to make it a feature flag - that's fine I think, but we need to make a note of that in the Kconfig file (that enabling PACKET_MMAP increases all skbs by the size of a long). I'd still rather prefer using the fragments instead though, because it seems like a reasonable solution that has 0 impact on any other code.

I think you should be almost 200% sure that skb is not allowed to grow up :)

Hi.

Quoted 1 line · Vitali

Right, we're saying the exact same thing I think. I had wanted just a void * that is specific to PACKET_MMAP, so that no one else pays the price if they don't need to. You instead want to make it a feature flag - that's fine I think, but we need to make a note of that in the Kconfig file (that enabling PACKET_MMAP increases all skbs by the size of a long). I'd still rather prefer using the fragments instead though, because it seems like a reasonable solution that has 0 impact on any other code.

I think you should be almost 200% sure that skb is not allowed to grow up :)

--
Evgeniy Polyakov

Quoted 1 line

I think you should be almost 200% sure that skb is not allowed to grow up :)

Well! I'm lost :)

How can we forward a pointer from skb allocation to skb destructor without adding a kind of destructor argument in sk_buff struct?

There are several possibilities:
* update destructor with your own data and put old one behind the data in skb
* update destructor, but put your own data into some storage which can be accessed from your callback, storage entry may contain whatever data you want and of course old destructor

* put your data into socket pointer (there still may be issues with destructor)
* if you fully own the skb, reuse whatever members there you like (except those which may be used by lower layer)
* find why do you need to have callback at destruction time and eliminate this problem.
Btw, netchannels do this way, since they are very similar to packet socket, but compared to your approach they have a copy.

At the first place I do not understand, why do you want to change the skb, since you can mmap whatever area you need and provide those pages into skb, which at free time will drop the reference counter. Skb lives in socket queue, so until it is empty you can sleep not allowing mapped buffer to shrink.

Thanks a lot for this list :)
So for example, is using of skb->mark to store a buffer index a good solution ? Or do you prefer another solution?

Quoted 5 lines

At the first place I do not understand, why do you want to change the skb, since you can mmap whatever area you need and provide those pages into skb, which at free time will drop the reference counter. Skb lives in socket queue, so until it is empty you can sleep not allowing mapped buffer to shrink.

We need to get the tpacket_hdr associated to data in skb in order to inform user that buffer is now available (This is done through status field, which is not sent over the network). What do you mean with "provide those pages", through frags? If yes, is it ok when frags are linearized?

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-12-08 5:58 AM
To: Johann Baudy
Cc: Lovich, Vitali; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi.
Hmm, for example splice() is supposed to do the same what you have, but because of udp_send*() limitations it can't. Splice does not care if data was overwritten during transfer, i.e. when ->sendpage() returns all
code assumes that data has been successfully transmitted, so userspace can overwrite mapped area or file content, and this new data will actually be transmitted. I believe you can use the same model and do not
care about userspace notifications after execution returned to the userspace from the 'sending call'.

I think you misunderstand how the API works. Yes the user could call send from the thread he's filling in data from. But the idea is that he would ideally do this from another thread so that it's possible to control latency between packet sends. Additionally, even without trying to control latency, there would still have to be complicated logic in userspace to determine when enough packets have been placed in the buffer to overcome the cost of a system call.

This is why we need to know which frame in the ring buffer the skb is associated with.

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-12-08 9:41 AM
To: Lovich, Vitali
Cc: Johann Baudy; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Vitali.

Quoted 1 line · Vitali

I think you misunderstand how the API works. Yes the user could call

send from the thread he's filling in data from. But the idea is that he would ideally do this from another thread so that it's possible to control latency between packet sends. Additionally, even without trying to control latency, there would still have to be complicated logic in userspace to determine when enough packets have been placed in the buffer to overcome the cost of a system call.

Quoted 2 lines

This is why we need to know which frame in the ring buffer the skb is

associated with.

What's the problem of invoking send machinery from different thread? You can wait in appropriate syscall until multiple packets have been sent and then return, or update some shared flag in the mapped area which says that something is being sent. When dev_queue_xmit() for selected set of packets completes, you can update that variable, and based on its value userspace can overwrite the area used by already sent packets.

I see the only reason to have a notification about skb completion is an absolute need to send sync data, i.e. packet data can not be overwritten
until packet reaches the media. But getting that existing Linux splice/sendfile and any other ->sendpage() users are racy in this regard
for centuries, this does not look as a strong demand for me.

They aren't racy - not in the sense that your suggestion would make it. With current splice & sendfile, when it returns from the syscall, the user knows that it has been transmitted and thus can continue using the file descriptors & memory (in the case of vmsplice). However, with your suggestion, the user could actually never know when it's safe to write into memory (even in a single-threaded situation). Thus it is racy in a single-threaded situation (even on a UP it's potentially racy) which is a pretty amazing feat. Consider:

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-12-08 10:12 AM
To: Lovich, Vitali
Cc: Johann Baudy; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Quoted 8 lines · Vitali

They aren't racy - not in the sense that your suggestion would make it. With current splice & sendfile, when it returns from the syscall, the user knows that it has been transmitted and thus can continue using the file descriptors & memory (in the case of vmsplice). However, with your suggestion, the user could actually never know when it's safe to write into memory (even in a single-threaded situation). Thus it is racy in a single-threaded situation (even on a UP it's potentially racy) which is a pretty amazing feat. Consider:

Problem is, that when sendfile() returns nothing has been transferred. And in some NICs it will take a while to actually make a transfer. And what you wrote below is exactly the same problem as exists with splice/sendfile in particular and ->sendpage() in general.

Please also update your mailer to wrap strings into 80-or-so lines, it is hard to answer into the middle of the paragraph.

I still don't see it. This can only be a problem for vmsplice, since I believe sendpage & splice copy the data from the source pipe if necessary. vmsplice solves this through the SPLICE_F_GIFT flag (if not specified, I'm assuming it copies the data into a temporary buffer). So I don't believe that these are actually racy functions if used properly.

However, your suggestion makes non-racy usage of the tx ring impossible unless you know ahead of time how many frames you will need (in which case, resetting the status flag is pointless). But for proper ring buffer behaviour, it needs to clear the flag in the skb destructor, once we know the data will no longer be used by the driver.

Quoted 2 lines

Please also update your mailer to wrap strings into 80-or-so lines, it is hard to answer into the middle of the paragraph.

Sorry - I hate using Outlook because it doesn't seem to honour my settings. I'll split up the lines manually instead of trusting Outlook.

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-12-08 11:14 AM
To: Lovich, Vitali
Cc: Johann Baudy; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Here is the main point: why do you ever care about data that was or was not transmitted and want to update something at destruction time and not
when dev_qeueue_xmit() returns. As pointed above, destruction time does not guarantee that skb was sent as long as return from dev_qeueue_xmit().

So you can update whatever flags you have to after return of the dev_qeueue_xmit() and will get the same behaviour as sendfile:
immediate write into the same memory area results in sending new content
(on some NICs).

I don't care whether or not the data was sent - I care whether or not the driver might still use the data in the frame the skb is referring to. In the destructor, clearly the driver can't since it gave up its reference. After dev_queue_xmit, we don't know because the driver (or the skb queue layer) may have decided to delay packet transmission.

Potentially the user might even have written half the payload of a packet when the device decides to send out the skb for that frame and thus send out half the payload from one
packet and half the payload from another.

Quoted 5 lines

So you can update whatever flags you have to after return of the dev_qeueue_xmit() and will get the same behaviour as sendfile:
immediate write into the same memory area results in sending new content
(on some NICs).

But using your approach, how can a user ever know whether or not he actually sent a packet?

Am I missing something fundamental in my understanding? I don't see any way, outside of using the skb destructor, to notify the user when he can safely write to a frame without interfering with any pending skbs.

-----Original Message-----
From: Evgeniy Polyakov [mailto:[email protected]]
Sent: November-12-08 1:46 PM
To: Lovich, Vitali
Cc: Johann Baudy; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Quoted 1 line · Vitali

I don't care whether or not the data was sent - I care whether or not

the driver

Quoted 1 line

might still use the data in the frame the skb is referring to. In

the destructor, clearly the

Quoted 1 line

driver can't since it gave up its reference. After dev_queue_xmit,

we don't know because

Quoted 1 line

the driver (or the skb queue layer) may have decided to delay packet

transmission.

Quoted 1 line

Quoted 1 line

Potentially the user might even have written half the payload of a

packet when the device decides to

Quoted 1 line

send out the skb for that frame and thus send out half the payload

from one

Quoted 1 line

packet and half the payload from another.

And what's the point in waiting for data to be unused?

You want to implement a system, which will behave more consistent than existing zero-copy approach, but yet not 100% correctly...

Quoted 6 lines

Quoted 5 lines

So you can update whatever flags you have to after return of the dev_qeueue_xmit() and will get the same behaviour as sendfile:
immediate write into the same memory area results in sending new content
(on some NICs).

But using your approach, how can a user ever know whether or not he

actually sent

Quoted 1 line

a packet?

There is no way to know that. At all. skb can be dropped by zillions of reasons and after it was submitted to the qdisk layer, there is no way to know how its life will continue. Well, in some cases it is possible to know (when qdisk just frees skb), but it is far from 100% of the cases.

Quoted 2 lines

Am I missing something fundamental in my understanding? I don't see

any way, outside

Quoted 1 line

of using the skb destructor, to notify the user when he can safely

write to a frame

Quoted 1 line

without interfering with any pending skbs.

Having a callback at destruction time does mean that noone uses skb, but
are you sure this is needed? With existing zero-copy (splice/sendfile) this is not true, but you want to extend this approach...

If you _do_ want to make it that way, you can remove destructor at all and implement own packet-socket-only allocation policy and thus have own
private destructor without extending skb.

If you _do_ want to make it that way, you can remove destructor at all and implement own packet-socket-only allocation policy and thus have own private destructor without extending skb.

Currently, we are executing sock_alloc_send_skb() to allocate a new skb from socket.
Then, we replace destructor sock_wfree() with our destructor packet_skb_destruct() which executes sock_wfree() once status of packet frame (associated to skb data) has been given back to user (status changed).

Is this way ok ?
Or shall we implement our own sock_alloc_send_skb()?

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-18-08 10:49 AM
To: Evgeniy Polyakov
Cc: Lovich, Vitali; [email protected]
Subject: Re: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Hi Evgeniy,

Quoted 1 line

If you _do_ want to make it that way, you can remove destructor at

all

Quoted 1 line

and implement own packet-socket-only allocation policy and thus have

own

Quoted 1 line

private destructor without extending skb.

Currently, we are executing sock_alloc_send_skb() to allocate a new skb from socket.
Then, we replace destructor sock_wfree() with our destructor packet_skb_destruct() which executes sock_wfree() once status of packet frame (associated to skb data) has been given back to user (status changed).

Are there any consequences to bypassing qdisc (other than not participating in any load balancing)? Also, since any users using the tx ring approach would have to write new code, it would be part of the documentation that the send behaviour is optimized for the least latency and highest throughput, thus no guarantees and assumptions can be made about the path the data takes through the stack.

Quoted 1 line

Are there any consequences to bypassing qdisc (other than not participating in any load balancing)? Also, since any users using the tx ring approach would have to write new code, it would be part of the documentation that the send behaviour is optimized for the least latency and highest throughput, thus no guarantees and assumptions can be made about the path the data takes through the stack.

qdisc is a base for multiqueue NICs (and all others too now), you can not bypass it there.

Quoted 4 lines

Quoted 2 lines

Also, you've got a potential threading issue - you're not protecting the frame index behind the spinlock.

You are right, I think I will spin-lock outside the do_while loop.

Can't do that I think - dev_queue_xmit sleeps AFAIK and you can't hold a spinlock when you sleep.

Can you explain why you need a spinlock though? I think you're trying to make it thread-safe if multiple threads make calls to send, correct? In this case, just change it to a semaphore, and protect the entire send call (well - at least in a safe enough way to make sure you're properly unlocking upon exit). Also, a better question is - is there any particular reason we need to support multiple threads calling send? Just stipulate that the behaviour is undefined when there are calls to send from different threads.

Actually, it represents the number of skbs allocated, which represents the number of pending skbs. Consider the following scenario:

atomic_inc(pending_skb) (pending_skb = 1)
skb is allocated
for some reason the filling in of the skb fails after the destructor is set (i.e. tp_len is too big) we go to out_free & free the skb
our destructor gets called
atomic_dec(pending_skb) (pending_skb = 0)
atomic_dec(pending_skb) (pending_skb = -1)

That is why it needs to be set after the skb is allocated (so that you don't need that atomic_dec).

Quoted 4 lines

atomic_dec() will be needed anyway if tpacket_fill_skb() or dev_queue_xmit() are failing (If performed after skb alloc).

No, because when you call kfree_skb it'll call the destructor (which tpacket_fill_skb should fill as the first thing, before any error conditions can happen).

Quoted 4 lines

Quoted 2 lines

Also, you've got a potential threading issue - you're not protecting the frame index behind the spinlock.

You are right, I think I will spin-lock outside the do_while loop.

Can't do that I think - dev_queue_xmit sleeps AFAIK and you can't hold a spinlock when you sleep.
Can you explain why you need a spinlock though? I think you're trying to make it thread-safe if multiple threads make calls to send, correct?
In this case, just change it to a semaphore, and protect the entire send call (well - at least in a safe enough way to make sure you're properly unlocking upon exit). Also, a better question is - is there any particular reason we need to support multiple threads calling send? Just stipulate that the behaviour is undefined when there are calls to send from different threads.

Quoted 3 lines

So, If my understanding of those memory barriers is correct, we should have a smp_rmb() before status reading and smp_wmb() after status writing in skb destructor and send procedure.

I'm not 100% sure about this - for this I was just referring to how the RX ring buffer does it. However, I don't think so. My understanding of the smp_wmb is that it's there so that the data cache is flushed properly. In other words, when you change the tp_status field, you need to make sure that it's actually been written before you flush. However, for reading, this is not a problem I think. I also think that since TP_STATUS_COPY is only used only within the kernel, when you set TP_STATUS_COPY, you don't need to do the smp_wmb or the data cache flush.

Perhaps someone who's more comfortable with memory barriers could respond on this issue?

-----Original Message-----
From: Johann Baudy [mailto:[email protected]]
Sent: November-04-08 2:34 PM
To: Lovich, Vitali
Cc: [email protected]; Evgeniy Polyakov
Subject: RE: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

Quoted 1 line

Quoted 1 line

So, If my understanding of those memory barriers is correct, we

should

Quoted 3 lines

Quoted 2 lines

have a smp_rmb() before status reading and smp_wmb() after status writing in skb destructor and send procedure.

I'm not 100% sure about this - for this I was just referring to how

the RX ring buffer does it. However, I don't think so. My understanding of the smp_wmb is that it's there so that the data cache is flushed properly. In other words, when you change the tp_status field, you need to make sure that it's actually been written before you flush. However, for reading, this is not a problem I think. I also think that since TP_STATUS_COPY is only used only within the kernel, when you set TP_STATUS_COPY, you don't need to do the smp_wmb or the data cache flush.

Quoted 2 lines

Perhaps someone who's more comfortable with memory barriers could

respond on this issue?

According to memory-barrier documentation, "A write barrier should always be paired with a data dependency barrier or read barrier" (cf. below).

SMP BARRIER PAIRING
-------------------

When dealing with CPU-CPU interactions, certain types of memory barrier should
always be paired. A lack of appropriate pairing is almost certainly an error.

A write barrier should always be paired with a data dependency barrier or read
barrier, though a general barrier would also be viable. Similarly a read
barrier or a data dependency barrier should always be paired with at least an
write barrier, though, again, a general barrier is viable:

	CPU 1		CPU 2
	===============	===============
	a = 1;
	<write barrier>
	b = 2;		x = b;
			<read barrier>
			y = a;

Best regards,
Johann