Skip to contentSkip to conversation

: Fix Ack Vector MPS Minimum Value

PATCH In review Gerrit Renker 2 participants 5 messages Feb 25, 2008 – Feb 26, 2008 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): patch_v2_ack_vector_MPS_contribution

#1 lore raw

v2 cover : Fix Ack Vector MPS Minimum Value

This is a resubmission to fix a problem with accounting for Ack Vector length in the MPS.

The present solution did not work well: the MPS did not account for Ack Vectors, so that applications which relied on the MPS value via getopt were disadvantaged by having all their Ack Vectors put onto Syncs -- sorely degrading performance.

Found out by testing with gstreamer DCCP plugin.

Hence the revision of these two patches implements a new strategy -- it now
* subtracts an estimated minimum from the MPS -
currently set to 16 bytes (found via experimentation) and
* schedules a Sync only if the actual length is greater than this minimum _and_ there is no space left on the skb.

Patch #1: Is the revised version of the account-for-option-sizes-in-MPS patch. Patch #2: Is the revised "exception handler" for overly large Ack Vectors.

Both patches have been uploaded to the test tree on

	git://eden-feed.erg.abnd.ac.uk/dccp_exp		[dccp]
#2 lore raw

PATCH v2 1/2 : Leave headroom for options when calculating the MPS

The Maximum Packet Size (MPS) is of interest for applications which want to transfer data, so it is only relevant to the data transfer phase of a connection (unless one wants to send data on the DCCP-Request, but that is not considered here).

The strategy chosen to deal with this requirement is to leave room for only such options that may appear on data packets.

A special consideration applies to Ack Vectors: this is purely guesswork, since these can have any length between 3 and 1020 bytes. The strategy chosen here is to subtract a configurable minimum, the value of 16 bytes (2 bytes for type/length plus 14 Ack Vector cells) has been found by experimentatation. If people experience this as too much or too little, this could later be turned into a Kconfig option.

There are currently no CCID-specific header options which may appear on data packets, hence it is not necessary to define a corresponding CCID field.

---
 net/dccp/ackvec.h |    3 +++
 net/dccp/output.c |   22 ++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)
net/dccp/ackvec.h +3 -0
@@ -21,6 +21,9 @@
 /* We can spread an ack vector across multiple options */
 #define DCCP_MAX_ACKVEC_LEN (DCCP_SINGLE_OPT_MAXLEN * 2)
 
+/* Estimated minimum average Ack Vector length - used for updating MPS */
+#define DCCPAV_MIN_OPTLEN	16
+
 #define DCCP_ACKVEC_STATE_RECEIVED	0
 #define DCCP_ACKVEC_STATE_ECN_MARKED	(1 << 6)
 #define DCCP_ACKVEC_STATE_NOT_RECEIVED	(3 << 6)
net/dccp/output.c +14 -8
@@ -153,21 +153,27 @@ unsigned int dccp_sync_mss(struct sock *
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct dccp_sock *dp = dccp_sk(sk);
 	u32 ccmps = dccp_determine_ccmps(dp);
-	int cur_mps = ccmps ? min(pmtu, ccmps) : pmtu;
+	u32 cur_mps = ccmps ? min(pmtu, ccmps) : pmtu;
 
 	/* Account for header lengths and IPv4/v6 option overhead */
 	cur_mps -= (icsk->icsk_af_ops->net_header_len + icsk->icsk_ext_hdr_len +
 		    sizeof(struct dccp_hdr) + sizeof(struct dccp_hdr_ext));
 
 	/*
-	 * FIXME: this should come from the CCID infrastructure, where, say,
-	 * TFRC will say it wants TIMESTAMPS, ELAPSED time, etc, for now lets
-	 * put a rough estimate for NDP + TIMESTAMP + TIMESTAMP_ECHO + ELAPSED
-	 * TIME + TFRC_OPT_LOSS_EVENT_RATE + TFRC_OPT_RECEIVE_RATE + padding to
-	 * make it a multiple of 4
+	 * Leave enough headroom for common DCCP header options.
+	 * This only considers options which may appear on DCCP-Data packets, as
+	 * per table 3 in RFC 4340, 5.8. When running out of space for other
+	 * options (eg. Ack Vector which can take up to 255 bytes), it is better
+	 * to schedule a separate Ack. Thus we leave headroom for the following:
+	 *  - 1 byte for Slow Receiver (11.6)
+	 *  - 6 bytes for Timestamp (13.1)
+	 *  - 10 bytes for Timestamp Echo (13.3)
+	 *  - 8 bytes for NDP count (7.7, when activated)
+	 *  - 6 bytes for Data Checksum (9.3)
+	 *  - %DCCPAV_MIN_OPTLEN bytes for Ack Vector size (11.4, when enabled)
 	 */
-
-	cur_mps -= ((5 + 6 + 10 + 6 + 6 + 6 + 3) / 4) * 4;
+	cur_mps -= roundup(1 + 6 + 10 + dp->dccps_send_ndp_count * 8 + 6 +
+			   (dp->dccps_hc_rx_ackvec ? DCCPAV_MIN_OPTLEN : 0), 4);
 
 	/* And store cached results */
 	icsk->icsk_pmtu_cookie = pmtu;
#3 lore raw

PATCH v2 2/2 : Schedule Sync as out-of-band mechanism

The problem with Ack Vectors is that

i) their length is variable and can in principle grow quite large,
ii) it is hard to predict exactly how large they will be.

Due to the second point it seems not a good idea to reduce the MPS; in particular when on average there is enough room for the Ack Vector and an increase in length is momentarily due to some burst loss, after which the Ack Vector returns to its normal/average length.

The solution taken by this patch is to subtract a minimum-expected Ack Vector length from the MPS (previous patch), and to defer any larger Ack Vectors onto a separate Sync - but only if indeed there is no space left on the skb.

This patch provides the infrastructure to schedule Sync-packets for transporting (urgent) out-of-band data. Its signalling is quicker than scheduling an Ack, since it does not need to wait for new application data.

It can thus serve other parts of the DCCP code as well.

---
 include/linux/dccp.h |    2 ++
 net/dccp/options.c   |   24 ++++++++++++++++++++----
 net/dccp/output.c    |    8 ++++++++
 3 files changed, 30 insertions(+), 4 deletions(-)
include/linux/dccp.h +2 -0
@@ -475,6 +475,7 @@ struct dccp_ackvec;
  * @dccps_hc_rx_insert_options - receiver wants to add options when acking
  * @dccps_hc_tx_insert_options - sender wants to add options when sending
  * @dccps_server_timewait - server holds timewait state on close (RFC 4340, 8.3)
+ * @dccps_sync_scheduled - flag which signals "send out-of-band message soon"
  * @dccps_xmit_timer - timer for when CCID is not ready to send
  * @dccps_syn_rtt - RTT sample from Request/Response exchange (in usecs)
  */
@@ -515,6 +516,7 @@ struct dccp_sock {
 	__u8				dccps_hc_rx_insert_options:1;
 	__u8				dccps_hc_tx_insert_options:1;
 	__u8				dccps_server_timewait:1;
+	__u8				dccps_sync_scheduled:1;
 	struct timer_list		dccps_xmit_timer;
 };
 
net/dccp/options.c +20 -4
@@ -428,6 +428,7 @@ int dccp_insert_option_ackvec(struct soc
 {
 	struct dccp_sock *dp = dccp_sk(sk);
 	struct dccp_ackvec *av = dp->dccps_hc_rx_ackvec;
+	struct dccp_skb_cb *dcb = DCCP_SKB_CB(skb);
 	const u16 buflen = dccp_ackvec_buflen(av);
 	/* Figure out how many options do we need to represent the ackvec */
 	const u16 nr_opts = DIV_ROUND_UP(buflen, DCCP_SINGLE_OPT_MAXLEN);
@@ -436,10 +437,25 @@ int dccp_insert_option_ackvec(struct soc
 	const unsigned char *tail, *from;
 	unsigned char *to;
 
-	if (DCCP_SKB_CB(skb)->dccpd_opt_len + len > DCCP_MAX_OPT_LEN)
+	if (dcb->dccpd_opt_len + len > DCCP_MAX_OPT_LEN) {
+		DCCP_WARN("Lacking space for %u bytes on %s packet\n", len,
+			  dccp_packet_name(dcb->dccpd_type));
 		return -1;
-
-	DCCP_SKB_CB(skb)->dccpd_opt_len += len;
+	}
+	/*
+	 * Since Ack Vectors are variable-length, we can not always predict
+	 * their size. To catch exception cases where the space is running out
+	 * on the skb, a separate Sync is scheduled to carry the Ack Vector.
+	 */
+	if (len > DCCPAV_MIN_OPTLEN &&
+	    len + dcb->dccpd_opt_len + skb->len > dp->dccps_mss_cache) {
+		DCCP_WARN("No space left for Ack Vector (%u) on skb (%u+%u), "
+			  "MPS=%u => reduce payload size?\n", len, skb->len,
+			  dcb->dccpd_opt_len, dp->dccps_mss_cache);
+		dp->dccps_sync_scheduled = 1;
+		return 0;
+	}
+	dcb->dccpd_opt_len += len;
 
 	to   = skb_push(skb, len);
 	len  = buflen;
@@ -480,7 +496,7 @@ int dccp_insert_option_ackvec(struct soc
 	/*
 	 * Each sent Ack Vector is recorded in the list, as per A.2 of RFC 4340.
 	 */
-	if (dccp_ackvec_update_records(av, DCCP_SKB_CB(skb)->dccpd_seq, nonce))
+	if (dccp_ackvec_update_records(av, dcb->dccpd_seq, nonce))
 		return -ENOBUFS;
 	return 0;
 }
net/dccp/output.c +8 -0
@@ -292,6 +292,8 @@ void dccp_write_xmit(struct sock *sk, in
 			if (err)
 				DCCP_BUG("err=%d after ccid_hc_tx_packet_sent",
 					 err);
+			if (dp->dccps_sync_scheduled)
+				dccp_send_sync(sk, dp->dccps_gsr, DCCP_PKT_SYNC);
 		} else {
 			dccp_pr_debug("packet discarded due to err=%d\n", err);
 			kfree_skb(skb);
@@ -564,6 +566,12 @@ void dccp_send_sync(struct sock *sk, con
 	DCCP_SKB_CB(skb)->dccpd_type = pkt_type;
 	DCCP_SKB_CB(skb)->dccpd_ack_seq = ackno;
 
+	/*
+	 * Clear the flag in case the Sync was scheduled for out-of-band data,
+	 * such as carrying a long Ack Vector.
+	 */
+	dccp_sk(sk)->dccps_sync_scheduled = 0;
+
 	dccp_transmit_skb(sk, skb);
 }
 
#4 lore raw

Re: [PATCH 2/2] [ACKVEC]: Schedule Sync as out-of-band mechanism

2008/2/26, Gerrit Renker <[email protected]>:

Quoted 4 lines

Hi Leandro,

thank you for paying attention and looking closely. Btw - this was not copied to the list?

I just forgot to copy to the list and I was thinking that I just git-pull the latest dccp branch in a wrong way. I'm playing attention to all your patches since I have (since a month ago) to backport the patches to kernel 2.6.21, which I use in Nokia N800/N810 devices that run linux. I'm using this devices to experiment DCCP and provide an up-to-date DCCP implementation for them. Before you ask: I have to use 2.6.21 because there are some issues regarding to some proprietary code like the wifi driver that just work in 2.6.21.

Quoted 43 lines

 | >  --- a/net/dccp/output.c
 | >  +++ b/net/dccp/output.c
 | >  @@ -292,6 +292,8 @@ void dccp_write_xmit(struct sock *sk, in
 | >                         if (err)
 | >                                 DCCP_BUG("err=%d after ccid_hc_tx_packet_sent",
 | >                                          err);
 | >  +                       if (dp->dccps_sync_scheduled)
 | >  +                               dccp_send_sync(sk, dp->dccps_gsr, DCCP_PKT_SYNC);
 |
 | These two lines are not appearing in the latest version of the dccp
 | branch of you git tree. BTW, this is totally different. Did I do
 | something wrong while pulling the branch or since this is a
 | resubmission of your patch, you had to assume that code (nowadays
 | modified)? In my version, this function uses a switch statement:
 |
 | void dccp_write_xmit(struct sock *sk)
 | {
 |         struct dccp_sock *dp = dccp_sk(sk);
 |         struct sk_buff *skb;
 |
 |         while ((skb = skb_peek(&sk->sk_write_queue))) {
 |                 int rc = ccid_hc_tx_send_packet(dp->dccps_hc_tx_ccid, sk, skb);
 |
 |                 switch (ccid_packet_dequeue_eval(rc)) {
 |                 case CCID_PACKET_WILL_DEQUEUE_LATER:
 |                         return;
 |                 case CCID_PACKET_DELAY:
 |                         sk_reset_timer(sk, &dp->dccps_xmit_timer, (jiffies +
 |                                                         msecs_to_jiffies(rc)));
 |                         return;
 |                 case CCID_PACKET_SEND_AT_ONCE:
 |                         dccp_xmit_packet(sk);
 |                         break;
 |                 case CCID_PACKET_ERR:
 |                         skb_dequeue(&sk->sk_write_queue);
 |                         kfree_skb(skb);
 |                         dccp_pr_debug("packet discarded due to err=%d\n", rc);
 |                 }
 |         }
 | }
Quoted 2 lines

And this is correct -- the `sync_scheduled' is now executed within
dccp_xmit_packet(), a few lines above this function in net/dccp/output.c:

Yes, I just check this function and certified these changes.

Quoted 25 lines
        static void dccp_xmit_packet(struct sock *sk)
        {
                // <snip>
                // ...

               /*
                * If the CCID needs to transfer additional header options out-of-band
                * (e.g. Ack Vectors or feature-negotiation options), it activates the
                * flag to schedule a Sync. The Sync will automatically incorporate all
                * currently valid header options so that this backlog is now cleared.
                */

               if (dp->dccps_sync_scheduled)
                       dccp_send_sync(sk, dp->dccps_gsr, DCCP_PKT_SYNC);
        }

The seeming contradiction comes from the order among the patches: the
out-of-band mechanism is introduced earlier, as part of the Ack Vector patches. After the Ack Vector patches there is an update for the dequeueing interface, which stops CCID-2 from polling. If you remove the three following patches from the top of your git tree, you will get the state as described above:

Quoted 6 lines

[CCID2]: Stop polling
[CCID]: Refine the wait-for-ccid mechanism [DCCP]: Extend CCID packet dequeueing interface

The last one introduces the new interface and shifts the sync_scheduled.

Yes, I also backport these patches last week for 2.6.21 linux omap kernel.

Quoted 3 lines

Gerrit

By the way, I had some issues to solve and I have just finish the modifications in the dccp plugin for gstreamer today. I will just now write the release notes, submit the new .tar.gz2 to the garage.maemo.org (as the previous version) and I will let you know the link to download it. Thank you for your feedback.

Leandro.

#5 lore raw

Re: [PATCH 2/2] [ACKVEC]: Schedule Sync as out-of-band mechanism

In reply to Gerrit Renker

| code like the wifi driver that just work in 2.6.21. |
If it is of any help, I still keep the per-release patch sets on
http://www.erg.abdn.ac.uk/users/gerrit/dccp/testing_dccp/test-tree/

Will probably not help much with back-porting, but can be used (with patch-utils / interdiff) to get a quick overview of what is happening.

I normally also send an inventory of all patches to Arnaldo, maybe should post this also to the list.

| By the way, I had some issues to solve and I have just finish the
| modifications in the dccp plugin for gstreamer today. I will just now
| write the release notes, submit the new .tar.gz2 to the
| garage.maemo.org (as the previous version) and I will let you know the
| link to download it.

Looking forward to testing the new version. Will also finish the two mpg123 patches and a little "piper" application. This worked together with the gstreamer plugin.

Haven't tested gstreamer with streaming from/to VLC yet.

I suppose the link will again be?:-

	https://garage.maemo.org/frs/?group_id)7