Skip to contentSkip to conversation

staging: net: ipv4: tcp_westwood: fixed warnings and checks

PATCH In review Suraj Singh 3 participants 7 messages Nov 5, 2018 – Nov 9, 2018

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Suraj Singh

author
#0 lore raw

PATCH 1/1 staging: net: ipv4: tcp_westwood: fixed warnings and checks

Fixed warnings and checks for TCP Westwood

Signed-off-by Suraj Singh <[email protected]>
---
 net/ipv4/tcp_westwood.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)
net/ipv4/tcp_westwood.c +16 -21
index bec9caf..0b4c67a9 100644
@@ -43,11 +43,10 @@ struct westwood {
 };
 
 /* TCP Westwood functions and constants */
-#define TCP_WESTWOOD_RTT_MIN   (HZ/20)	/* 50ms */
-#define TCP_WESTWOOD_INIT_RTT  (20*HZ)	/* maybe too conservative?! */
+#define TCP_WESTWOOD_RTT_MIN   (HZ / 20)	/* 50ms */
+#define TCP_WESTWOOD_INIT_RTT  (20 * HZ)	/* maybe too conservative?! */
 
-/*
- * @tcp_westwood_create
+/* @tcp_westwood_create
  * This function initializes fields used in TCP Westwood+,
  * it is called after the initial SYN, so the sequence numbers
  * are correct but new passive connections we have no

@@ -67,14 +66,14 @@ static void tcp_westwood_init(struct sock *sk)
 	w->accounted = 0;
 	w->cumul_ack = 0;
 	w->reset_rtt_min = 1;
-	w->rtt_min = w->rtt = TCP_WESTWOOD_INIT_RTT;
+	w->rtt_min = TCP_WESTWOOD_INIT_RTT;
+	w->rtt = TCP_WESTWOOD_INIT_RTT;
 	w->rtt_win_sx = tcp_jiffies32;
 	w->snd_una = tcp_sk(sk)->snd_una;
 	w->first_ack = 1;
 }
 
-/*
- * @westwood_do_filter
+/* @westwood_do_filter
  * Low-pass filter. Implemented using constant coefficients.
  */
 static inline u32 westwood_do_filter(u32 a, u32 b)

@@ -94,8 +93,7 @@ static void westwood_filter(struct westwood *w, u32 delta)
 	}
 }
 
-/*
- * @westwood_pkts_acked
+/* @westwood_pkts_acked
  * Called after processing group of packets.
  * but all westwood needs is the last sample of srtt.
  */

@@ -108,8 +106,7 @@ static void tcp_westwood_pkts_acked(struct sock *sk,
 		w->rtt = usecs_to_jiffies(sample->rtt_us);
 }
 
-/*
- * @westwood_update_window
+/* @westwood_update_window
  * It updates RTT evaluation window if it is the right moment to do
  * it. If so it calls filter for evaluating bandwidth.
  */

@@ -127,8 +124,7 @@ static void westwood_update_window(struct sock *sk)
 		w->first_ack = 0;
 	}
 
-	/*
-	 * See if a RTT-window has passed.
+	/* See if a RTT-window has passed.
 	 * Be careful since if RTT is less than
 	 * 50ms we don't filter but we continue 'building the sample'.
 	 * This minimum limit was chosen since an estimation on small

@@ -149,12 +145,12 @@ static inline void update_rtt_min(struct westwood *w)
 	if (w->reset_rtt_min) {
 		w->rtt_min = w->rtt;
 		w->reset_rtt_min = 0;
-	} else
+	} else {
 		w->rtt_min = min(w->rtt, w->rtt_min);
+	}
 }
 
-/*
- * @westwood_fast_bw
+/* @westwood_fast_bw
  * It is called when we are in fast path. In particular it is called when
  * header prediction is successful. In such case in fact update is
  * straight forward and doesn't need any particular care.

@@ -171,8 +167,7 @@ static inline void westwood_fast_bw(struct sock *sk)
 	update_rtt_min(w);
 }
 
-/*
- * @westwood_acked_count
+/* @westwood_acked_count
  * This function evaluates cumul_ack for evaluating bk in case of
  * delayed or partial acks.
  */

@@ -207,8 +202,7 @@ static inline u32 westwood_acked_count(struct sock *sk)
 	return w->cumul_ack;
 }
 
-/*
- * TCP Westwood
+/* TCP Westwood
  * Here limit is evaluated as Bw estimation*RTTmin (for obtaining it
  * in packets we use mss_cache). Rttmin is guaranteed to be >= 2
  * so avoids ever returning 0.

@@ -243,7 +237,8 @@ static void tcp_westwood_event(struct sock *sk, enum tcp_ca_event event)
 
 	switch (event) {
 	case CA_EVENT_COMPLETE_CWR:
-		tp->snd_cwnd = tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
+		tp->snd_cwnd = tcp_westwood_bw_rttmin(sk);
+		tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
 		break;
 	case CA_EVENT_LOSS:
 		tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
Signature · 1 line
2.7.4
maintainer
#2 lore raw
In reply to Suraj Singh

From: Suraj Singh <[email protected]>
Date: Mon, 5 Nov 2018 19:23:05 +0530

Quoted 3 lines

Fixed warnings and checks for TCP Westwood

Signed-off-by Suraj Singh <[email protected]>

I asked you yesterday why "staging: " appears in your subject line and you have failed to respond and explain.

There are also functional issues with your patch:

net/ipv4/tcp_westwood.c static void tcp_westwood_event(struct sock *sk, enum tcp_ca_event event) · line 241

Quoted 3 lines
net/ipv4/tcp_westwood.c +2 -1
-		tp->snd_cwnd = tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
+		tp->snd_cwnd = tcp_westwood_bw_rttmin(sk);
+		tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);

This is bogus, now tcp_westwood_bw_rttmin(sk) will potentially be called two times instead of once.

The existing code is fine, please do not modify it.

author
#3 lore raw

Re: net: ipv4: tcp_westwood: fixed warnings and checks

In reply to Suraj Singh

From: Suraj Singh <[email protected]>

Regrding why I used "staging: " in the commmit message, I was following Greg Kroah-Hartman's video on YouTube on how to submit your first patch, and in his sample commit, he'd started his commit message with "staging: ", and so I thought it was convention to do so. I'll remove that immediately. I made this same mistake in another patch that I just sent for TCP Vegas, I'll make the change there as well.

I didn't consider the complexities of calling the same function twice. I was looking more towards satisying the scriptpatch.pl's requirements.

- tp->snd_cwnd = tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
+ tp->snd_cwnd = tcp_westwood_bw_rttmin(sk);
+ tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);

I've made the same mistake here. I'll make these changes and resubmit. Is there anything else that's wrong?

author
#4 lore raw

PATCH 1/1 staging: net: ipv4: tcp_westwood: fixed warnings and checks

In reply to Suraj Singh

Fixed warnings and checks for TCP Westwood

Signed-off-by Suraj Singh <[email protected]>
---
 net/ipv4/tcp_westwood.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)
net/ipv4/tcp_westwood.c +12 -19
index bec9caf..8879152 100644
@@ -43,11 +43,10 @@ struct westwood {
 };
 
 /* TCP Westwood functions and constants */
-#define TCP_WESTWOOD_RTT_MIN   (HZ/20)	/* 50ms */
-#define TCP_WESTWOOD_INIT_RTT  (20*HZ)	/* maybe too conservative?! */
+#define TCP_WESTWOOD_RTT_MIN   (HZ / 20)	/* 50ms */
+#define TCP_WESTWOOD_INIT_RTT  (20 * HZ)	/* maybe too conservative?! */
 
-/*
- * @tcp_westwood_create
+/* @tcp_westwood_create
  * This function initializes fields used in TCP Westwood+,
  * it is called after the initial SYN, so the sequence numbers
  * are correct but new passive connections we have no

@@ -73,8 +72,7 @@ static void tcp_westwood_init(struct sock *sk)
 	w->first_ack = 1;
 }
 
-/*
- * @westwood_do_filter
+/* @westwood_do_filter
  * Low-pass filter. Implemented using constant coefficients.
  */
 static inline u32 westwood_do_filter(u32 a, u32 b)

@@ -94,8 +92,7 @@ static void westwood_filter(struct westwood *w, u32 delta)
 	}
 }
 
-/*
- * @westwood_pkts_acked
+/* @westwood_pkts_acked
  * Called after processing group of packets.
  * but all westwood needs is the last sample of srtt.
  */

@@ -108,8 +105,7 @@ static void tcp_westwood_pkts_acked(struct sock *sk,
 		w->rtt = usecs_to_jiffies(sample->rtt_us);
 }
 
-/*
- * @westwood_update_window
+/* @westwood_update_window
  * It updates RTT evaluation window if it is the right moment to do
  * it. If so it calls filter for evaluating bandwidth.
  */

@@ -127,8 +123,7 @@ static void westwood_update_window(struct sock *sk)
 		w->first_ack = 0;
 	}
 
-	/*
-	 * See if a RTT-window has passed.
+	/* See if a RTT-window has passed.
 	 * Be careful since if RTT is less than
 	 * 50ms we don't filter but we continue 'building the sample'.
 	 * This minimum limit was chosen since an estimation on small

@@ -149,12 +144,12 @@ static inline void update_rtt_min(struct westwood *w)
 	if (w->reset_rtt_min) {
 		w->rtt_min = w->rtt;
 		w->reset_rtt_min = 0;
-	} else
+	} else {
 		w->rtt_min = min(w->rtt, w->rtt_min);
+	}
 }
 
-/*
- * @westwood_fast_bw
+/* @westwood_fast_bw
  * It is called when we are in fast path. In particular it is called when
  * header prediction is successful. In such case in fact update is
  * straight forward and doesn't need any particular care.

@@ -171,8 +166,7 @@ static inline void westwood_fast_bw(struct sock *sk)
 	update_rtt_min(w);
 }
 
-/*
- * @westwood_acked_count
+/* @westwood_acked_count
  * This function evaluates cumul_ack for evaluating bk in case of
  * delayed or partial acks.
  */

@@ -207,8 +201,7 @@ static inline u32 westwood_acked_count(struct sock *sk)
 	return w->cumul_ack;
 }
 
-/*
- * TCP Westwood
+/* TCP Westwood
  * Here limit is evaluated as Bw estimation*RTTmin (for obtaining it
  * in packets we use mss_cache). Rttmin is guaranteed to be >= 2
  * so avoids ever returning 0.
Signature · 1 line
2.7.4
#5 lore raw

Hi,

Quoted 10 lines

Gesendet: Donnerstag, 08. November 2018 um 10:46 Uhr
Von: "Suraj Singh" <[email protected]>
An: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Betreff: [PATCH] staging: net: ipv4: tcp_westwood: fixed warnings and checks

Fixed warnings and checks for TCP Westwood

Signed-off-by Suraj Singh <[email protected]>

---

You use the prefix "staging" again in you subject line, which still is wrong. "staging" means that you fix something from the staging area (see drivers/staging/ in the kernel sources. BTW: the staging area is a much better starting point for first patches than the core parts of the network subsystem).
Also if you send a subsequent version of a patch you have to write the version number (e.g. v2) in the subject line. Have a look at other patch submissions on this mailing list for examples.

Regards,
Lino

author
#6 lore raw

Re: net: ipv4: tcp_westwood

In reply to Suraj Singh

Hi Lino,

Thank you for clarifying the doubts I had and for the suggestion on where to start contributing. Previously, and in this patch I hadn't configured mutt and was manually using git send-mail to respond to each thread using the corresponding mail-id. That's why I wasn't sure why even in this patch, "staging: " had appeared even after editing the commit message. Now that I've got the email client setup, I'll fix that "staging: " issue and send a [v3] patch.