* [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
@ 2010-12-22 18:39 Jiri Kosina
2010-12-23 8:24 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2010-12-22 18:39 UTC (permalink / raw)
To: David Miller, netdev, linux-kernel; +Cc: Vojtech Pavlik, Ilpo Järvinen
Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case when
congestion window initialization has been mistakenly omitted by
introducing cwnd label and putting backwards jump from the end of the
function.
This makes the code unnecessarily tricky to read and understand on a first
sight.
Shuffle the code around a little bit to make it more obvious.
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
net/ipv4/tcp_input.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 6d8ab1c..dddff6d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -915,11 +915,7 @@ static void tcp_init_metrics(struct sock *sk)
if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
goto reset;
-cwnd:
- tp->snd_cwnd = tcp_init_cwnd(tp, dst);
- tp->snd_cwnd_stamp = tcp_time_stamp;
- return;
-
+ goto out;
reset:
/* Play conservative. If timestamps are not
* supported, TCP will fail to recalculate correct
@@ -930,7 +926,9 @@ reset:
tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
inet_csk(sk)->icsk_rto = TCP_TIMEOUT_INIT;
}
- goto cwnd;
+out:
+ tp->snd_cwnd = tcp_init_cwnd(tp, dst);
+ tp->snd_cwnd_stamp = tcp_time_stamp;
}
static void tcp_update_reordering(struct sock *sk, const int metric,
--
1.7.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
2010-12-22 18:39 [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics() Jiri Kosina
@ 2010-12-23 8:24 ` Eric Dumazet
2010-12-23 9:03 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2010-12-23 8:24 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Miller, netdev, linux-kernel, Vojtech Pavlik,
Ilpo Järvinen
Le mercredi 22 décembre 2010 à 19:39 +0100, Jiri Kosina a écrit :
> Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case when
> congestion window initialization has been mistakenly omitted by
> introducing cwnd label and putting backwards jump from the end of the
> function.
>
> This makes the code unnecessarily tricky to read and understand on a first
> sight.
>
> Shuffle the code around a little bit to make it more obvious.
Well in fine you have
if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
goto reset;
goto out;
reset:
Is that really more obvious ? ;)
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
> net/ipv4/tcp_input.c | 10 ++++------
> 1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 6d8ab1c..dddff6d 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -915,11 +915,7 @@ static void tcp_init_metrics(struct sock *sk)
> if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
> goto reset;
>
> -cwnd:
> - tp->snd_cwnd = tcp_init_cwnd(tp, dst);
> - tp->snd_cwnd_stamp = tcp_time_stamp;
> - return;
> -
> + goto out;
> reset:
> /* Play conservative. If timestamps are not
> * supported, TCP will fail to recalculate correct
> @@ -930,7 +926,9 @@ reset:
> tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
> inet_csk(sk)->icsk_rto = TCP_TIMEOUT_INIT;
> }
> - goto cwnd;
> +out:
> + tp->snd_cwnd = tcp_init_cwnd(tp, dst);
> + tp->snd_cwnd_stamp = tcp_time_stamp;
> }
>
> static void tcp_update_reordering(struct sock *sk, const int metric,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
2010-12-23 8:24 ` Eric Dumazet
@ 2010-12-23 9:03 ` Jiri Kosina
2010-12-23 9:14 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2010-12-23 9:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, linux-kernel, Vojtech Pavlik,
Ilpo Järvinen
On Thu, 23 Dec 2010, Eric Dumazet wrote:
> Le mercredi 22 décembre 2010 à 19:39 +0100, Jiri Kosina a écrit :
> > Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case when
> > congestion window initialization has been mistakenly omitted by
> > introducing cwnd label and putting backwards jump from the end of the
> > function.
> >
> > This makes the code unnecessarily tricky to read and understand on a first
> > sight.
> >
> > Shuffle the code around a little bit to make it more obvious.
>
> Well in fine you have
>
> if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
> goto reset;
> goto out;
> reset:
>
> Is that really more obvious ? ;)
To me it seems much more obvious than goto from the very end of the
function somewhere into the middle and returning from there, but
definitely a matter of personal taste.
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
2010-12-23 9:03 ` Jiri Kosina
@ 2010-12-23 9:14 ` Eric Dumazet
2010-12-23 9:23 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2010-12-23 9:14 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Miller, netdev, linux-kernel, Vojtech Pavlik,
Ilpo Järvinen
Le jeudi 23 décembre 2010 à 10:03 +0100, Jiri Kosina a écrit :
> On Thu, 23 Dec 2010, Eric Dumazet wrote:
>
> > Le mercredi 22 décembre 2010 à 19:39 +0100, Jiri Kosina a écrit :
> > > Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case when
> > > congestion window initialization has been mistakenly omitted by
> > > introducing cwnd label and putting backwards jump from the end of the
> > > function.
> > >
> > > This makes the code unnecessarily tricky to read and understand on a first
> > > sight.
> > >
> > > Shuffle the code around a little bit to make it more obvious.
> >
> > Well in fine you have
> >
> > if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
> > goto reset;
> > goto out;
> > reset:
> >
> > Is that really more obvious ? ;)
>
> To me it seems much more obvious than goto from the very end of the
> function somewhere into the middle and returning from there, but
> definitely a matter of personal taste.
>
You dont understand what I said. Please read again.
To me I prefer you _finish_ the cleanup so that we have :
if (some condition) {
reset:
}
out:
You remove two "goto" in the process.
Is that clear now ?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
2010-12-23 9:14 ` Eric Dumazet
@ 2010-12-23 9:23 ` Jiri Kosina
2010-12-23 17:56 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2010-12-23 9:23 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, linux-kernel, Vojtech Pavlik,
Ilpo Järvinen
On Thu, 23 Dec 2010, Eric Dumazet wrote:
> > > > Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case when
> > > > congestion window initialization has been mistakenly omitted by
> > > > introducing cwnd label and putting backwards jump from the end of the
> > > > function.
> > > >
> > > > This makes the code unnecessarily tricky to read and understand on a first
> > > > sight.
> > > >
> > > > Shuffle the code around a little bit to make it more obvious.
> > >
> > > Well in fine you have
> > >
> > > if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
> > > goto reset;
> > > goto out;
> > > reset:
> > >
> > > Is that really more obvious ? ;)
> >
> > To me it seems much more obvious than goto from the very end of the
> > function somewhere into the middle and returning from there, but
> > definitely a matter of personal taste.
> >
>
> You dont understand what I said. Please read again.
>
> To me I prefer you _finish_ the cleanup so that we have :
>
> if (some condition) {
> reset:
> }
>
> out:
>
> You remove two "goto" in the process.
>
> Is that clear now ?
Right, that's even better. Updated patch below.
From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case
when congestion window initialization has been mistakenly omitted
by introducing cwnd label and putting backwards goto from the
end of the function.
This makes the code unnecessarily tricky to read and understand
on a first sight.
Shuffle the code around a little bit to make it more obvious.
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
net/ipv4/tcp_input.c | 29 ++++++++++++-----------------
1 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 6d8ab1c..3d1e015 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -912,25 +912,20 @@ static void tcp_init_metrics(struct sock *sk)
tp->mdev_max = tp->rttvar = max(tp->mdev, tcp_rto_min(sk));
}
tcp_set_rto(sk);
- if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp)
- goto reset;
-
-cwnd:
- tp->snd_cwnd = tcp_init_cwnd(tp, dst);
- tp->snd_cwnd_stamp = tcp_time_stamp;
- return;
-
+ if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp) {
reset:
- /* Play conservative. If timestamps are not
- * supported, TCP will fail to recalculate correct
- * rtt, if initial rto is too small. FORGET ALL AND RESET!
- */
- if (!tp->rx_opt.saw_tstamp && tp->srtt) {
- tp->srtt = 0;
- tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
- inet_csk(sk)->icsk_rto = TCP_TIMEOUT_INIT;
+ /* Play conservative. If timestamps are not
+ * supported, TCP will fail to recalculate correct
+ * rtt, if initial rto is too small. FORGET ALL AND RESET!
+ */
+ if (!tp->rx_opt.saw_tstamp && tp->srtt) {
+ tp->srtt = 0;
+ tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
+ inet_csk(sk)->icsk_rto = TCP_TIMEOUT_INIT;
+ }
}
- goto cwnd;
+ tp->snd_cwnd = tcp_init_cwnd(tp, dst);
+ tp->snd_cwnd_stamp = tcp_time_stamp;
}
static void tcp_update_reordering(struct sock *sk, const int metric,
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
2010-12-23 9:23 ` Jiri Kosina
@ 2010-12-23 17:56 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2010-12-23 17:56 UTC (permalink / raw)
To: jkosina; +Cc: eric.dumazet, netdev, linux-kernel, vojtech, ilpo.jarvinen
From: Jiri Kosina <jkosina@suse.cz>
Date: Thu, 23 Dec 2010 10:23:38 +0100 (CET)
> Right, that's even better. Updated patch below.
>
>
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics()
>
> Commit 86bcebafc5e7f5 ("tcp: fix >2 iw selection") fixed a case
> when congestion window initialization has been mistakenly omitted
> by introducing cwnd label and putting backwards goto from the
> end of the function.
>
> This makes the code unnecessarily tricky to read and understand
> on a first sight.
>
> Shuffle the code around a little bit to make it more obvious.
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Applied, thanks everyone.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-23 17:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-22 18:39 [PATCH] tcp: cleanup of cwnd initialization in tcp_init_metrics() Jiri Kosina
2010-12-23 8:24 ` Eric Dumazet
2010-12-23 9:03 ` Jiri Kosina
2010-12-23 9:14 ` Eric Dumazet
2010-12-23 9:23 ` Jiri Kosina
2010-12-23 17:56 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox