rewriting skb->truesize... good or bad idea

Hi Folks

I was looking at SCTP performance problem that is a result of receive buffer exhaustion and found the we severely overcharge the receive buffer when multiple data chunks are bundled together. This bundling usually happens at retransmit time which penalizes us even more.

Here is what happens. For every "data" chunk that SCTP stack receives, we clone skb of that data chunk, charge the receive buffer for the skb, and put the chunk on the the socket receive queue (this is skipping a few steps, but they don't matter for the sake of this discussion). We charge the receive buffer with the skb->truesize.

The problem shows up when multiple data chunks are "bundled" into the same skb. We end up with multiple clones, and for each clone we charge skb->truesize against the receive buffer. However, since skb_clone() preservers the original truesize in all clones, we end up overcharging.

One of the proposed solutions is change the skb->truesize of the clone to just be sizeof(struct sk_buff), if and only if this is not the first data chunk in the packet.

I've attached the patch, in case people want to look at the code.

However, we question if this is a good idea or if this is going to break things...

Thanks
-vlad

index 5b5ae79..9bb1dbd 100644
@@ -5349,7 +5349,7 @@ static int sctp_eat_data(const struct sc
 	if (SCTP_CMD_CHUNK_ULP == deliver)
 		sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_TSN, SCTP_U32(tsn));
 
-	chunk->data_accepted = 1;
+	chunk->data_accepted++;
 
 	/* Note: Some chunks may get overcounted (if we drop) or overcounted
 	 * if we renege and the chunk arrives again.

index ee23678..0e1f11d 100644
@@ -685,6 +685,17 @@ struct sctp_ulpevent *sctp_ulpevent_make
 	/* Initialize event with flags 0.  */
 	sctp_ulpevent_init(event, 0);
 
+	/* Check to see if we need to fixup the truesize of the clone.
+	 * We are about to charge the receive buffer for this chunk,
+	 * and we always use skb->truesize.  However, this doesn't work
+	 * for bundled data chunks since we'll drastically overcharge.
+	 * To get around that, keep the oiginal truesize on the clone
+	 * only for the first data chunk, and update truesize for the clone
+	 * on subsequent ones.
+	 */
+	if (chunk->data_accepted > 1)
+		skb->truesize = sizeof(struct skb);
+
 	sctp_ulpevent_receive_data(event, asoc);
 
 	event->stream = ntohs(chunk->subh.data_hdr->stream);

From: Vlad Yasevich <[email protected]>
Date: Fri, 29 Sep 2006 14:16:57 -0400

Modification of skb->truesize is very dangerous and is only legal in a very limited set of circumstances.

The core problem is that if any other reference exists to the skb you could corrupt existing socket accounting by changing skb->truesize.

Let's say that the packet went into a network tap like tcpdump and the packet has been charged to an AF_PACKET socket via skb->truesize. If you modify skb->truesize then when the AF_PACKET socket releases it's reference it will subtract the wrong amount of skb->truesize from the socket receive buffer.

If, on the other hand, you know you have exclusive access to the skb and there are no other references, setting skb->truesize can be OK. However setting it to sizeof(struct sk_buff) doesn't make sense since there is at least:

sizeof(struct sk_buff) + sizeof(struct skb_shared_info)

memory assosciated with any SKB whatsoever in the kernel. That is, even for a "zero length" skb->data buffer, we still always allocate the skb_shared_info area which must be accounted for.

BTW, I think the whole chunking mechanism in the SCTP code is the largest source of problems and maintainability issues in that stack. Any time I want to make major modifications to SKBs to make them smaller or whatever, the most difficult piece of code to adjust is this code.

Hi David

Thanks for the answer.

Yes, that's what I've found in the code.

Thankfully, this does not appear to be a problem in this particular case. The clones that would have their truesize changed, only exist in SCTP. The packet has already gone through all verifications and we just queue the clones to the socket. The original packet skb remains unchanged. SCTP calls kfree_skb on it once all the cloning is done.

Well, since we are dealing with clones of the original skb, I didn't think that we need to include skb_shared_info. I thought that was accounted for in the original skb?

The clones, that SCTP creates, point at a subset of data... something like this:

clone 1 ------+ clone 2 ------+

               |                        |
               |                        |
               v                        v
  +-------------------------------------------------------
  | proto hdrs | sctp data chunk 1      | data chunk 2
  +-------------------------------------------------------

^
|
|
+--- orig_skb->head

Right now, for every clone we use the generic socket accounting code that uses skb->truesize.

The alternative to changing truesize is to write an SCTP version of socket accounting. This is what we did back in 2.6.10 days and we tried get away from that.

If you have another idea of how we could do this, I'd appreciate it.

I think you've already removed all the dependencies between chunking and SKBs. All we have now are some pointers to skb. This work actually made the protocol a lot more stable.

Thanks
-vlad

From: Vlad Yasevich <[email protected]>
Date: Mon, 02 Oct 2006 10:46:42 -0400

We use the original packet's truesize, as-is, when making SKB clones. This is our global policy.

The reason for that behavior is the usual case is that the clone is being attached to a different socket than the original SKB will. And we very much should charge the memory fully to every socket that references it.

In your case it seems that the memory is already charged to the socket, and we would just be charging that memory again to the same socket. So in this very specific case, it should be OK not to include skb_shared_info in the truesize calculation.