* [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames
@ 2009-12-09 8:26 Krishna Kumar
2009-12-09 8:26 ` [PATCH 2/3] tcp: Remove unrequired operations in tcp_push() Krishna Kumar
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Krishna Kumar @ 2009-12-09 8:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Krishna Kumar
From: Krishna Kumar <krkumar2@in.ibm.com>
tcp_push checks tcp_send_head and calls __tcp_push_pending_frames,
which again checks tcp_send_head, and this unnecessary check is
done for every other caller of __tcp_push_pending_frames.
Remove tcp_send_head check in __tcp_push_pending_frames and add
the check to tcp_push_pending_frames. Other functions call
__tcp_push_pending_frames only when tcp_send_head would evaluate
to true.
Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
include/net/tcp.h | 16 +++++++++-------
net/ipv4/tcp_output.c | 5 -----
2 files changed, 9 insertions(+), 12 deletions(-)
diff -ruNp org/include/net/tcp.h new/include/net/tcp.h
--- org/include/net/tcp.h 2009-12-04 18:15:26.000000000 +0530
+++ new/include/net/tcp.h 2009-12-09 09:57:39.000000000 +0530
@@ -857,13 +857,6 @@ static inline void tcp_check_probe_timer
icsk->icsk_rto, TCP_RTO_MAX);
}
-static inline void tcp_push_pending_frames(struct sock *sk)
-{
- struct tcp_sock *tp = tcp_sk(sk);
-
- __tcp_push_pending_frames(sk, tcp_current_mss(sk), tp->nonagle);
-}
-
static inline void tcp_init_wl(struct tcp_sock *tp, u32 seq)
{
tp->snd_wl1 = seq;
@@ -1366,6 +1359,15 @@ static inline int tcp_write_queue_empty(
return skb_queue_empty(&sk->sk_write_queue);
}
+static inline void tcp_push_pending_frames(struct sock *sk)
+{
+ if (tcp_send_head(sk)) {
+ struct tcp_sock *tp = tcp_sk(sk);
+
+ __tcp_push_pending_frames(sk, tcp_current_mss(sk), tp->nonagle);
+ }
+}
+
/* Start sequence of the highest skb with SACKed bit, valid only if
* sacked > 0 or when the caller has ensured validity by itself.
*/
diff -ruNp org/net/ipv4/tcp_output.c new/net/ipv4/tcp_output.c
--- org/net/ipv4/tcp_output.c 2009-12-04 18:16:00.000000000 +0530
+++ new/net/ipv4/tcp_output.c 2009-12-04 18:16:24.000000000 +0530
@@ -1799,11 +1799,6 @@ static int tcp_write_xmit(struct sock *s
void __tcp_push_pending_frames(struct sock *sk, unsigned int cur_mss,
int nonagle)
{
- struct sk_buff *skb = tcp_send_head(sk);
-
- if (!skb)
- return;
-
/* If we are closed, the bytes will have to remain here.
* In time closedown will finish, we empty the write queue and
* all will be happy.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
2009-12-09 8:26 [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Krishna Kumar
@ 2009-12-09 8:26 ` Krishna Kumar
2009-12-10 8:10 ` Ilpo Järvinen
2009-12-09 8:26 ` [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg Krishna Kumar
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Krishna Kumar @ 2009-12-09 8:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Krishna Kumar
From: Krishna Kumar <krkumar2@in.ibm.com>
Remove unrequired operations in tcp_push()
Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
net/ipv4/tcp.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff -ruNp org/net/ipv4/tcp.c new/net/ipv4/tcp.c
--- org/net/ipv4/tcp.c 2009-12-04 18:16:10.000000000 +0530
+++ new/net/ipv4/tcp.c 2009-12-09 10:01:35.000000000 +0530
@@ -536,8 +536,7 @@ static inline void skb_entail(struct soc
tp->nonagle &= ~TCP_NAGLE_PUSH;
}
-static inline void tcp_mark_urg(struct tcp_sock *tp, int flags,
- struct sk_buff *skb)
+static inline void tcp_mark_urg(struct tcp_sock *tp, int flags)
{
if (flags & MSG_OOB)
tp->snd_up = tp->write_seq;
@@ -546,13 +545,15 @@ static inline void tcp_mark_urg(struct t
static inline void tcp_push(struct sock *sk, int flags, int mss_now,
int nonagle)
{
- struct tcp_sock *tp = tcp_sk(sk);
-
if (tcp_send_head(sk)) {
- struct sk_buff *skb = tcp_write_queue_tail(sk);
- if (!(flags & MSG_MORE) || forced_push(tp))
+ struct tcp_sock *tp = tcp_sk(sk);
+
+ if (!(flags & MSG_MORE) || forced_push(tp)) {
+ struct sk_buff *skb = tcp_write_queue_tail(sk);
+
tcp_mark_push(tp, skb);
- tcp_mark_urg(tp, flags, skb);
+ }
+ tcp_mark_urg(tp, flags);
__tcp_push_pending_frames(sk, mss_now,
(flags & MSG_MORE) ? TCP_NAGLE_CORK : nonagle);
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg
2009-12-09 8:26 [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Krishna Kumar
2009-12-09 8:26 ` [PATCH 2/3] tcp: Remove unrequired operations in tcp_push() Krishna Kumar
@ 2009-12-09 8:26 ` Krishna Kumar
2009-12-10 8:17 ` Ilpo Järvinen
2009-12-10 8:05 ` [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Ilpo Järvinen
2009-12-23 22:15 ` David Miller
3 siblings, 1 reply; 13+ messages in thread
From: Krishna Kumar @ 2009-12-09 8:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Krishna Kumar
From: Krishna Kumar <krkumar2@in.ibm.com>
Slightly optimize tcp_sendmsg since NETIF_F_SG is used many
times iteratively in the loop. The only other modification is
to change:
} else if (i == MAX_SKB_FRAGS ||
(!i &&
!(sk->sk_route_caps & NETIF_F_SG))) {
to:
} else if (i == MAX_SKB_FRAGS || !sg) {
Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
net/ipv4/tcp.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff -ruNp org/net/ipv4/tcp.c new/net/ipv4/tcp.c
--- org/net/ipv4/tcp.c 2009-12-09 10:01:35.000000000 +0530
+++ new/net/ipv4/tcp.c 2009-12-09 10:26:42.000000000 +0530
@@ -878,12 +878,12 @@ ssize_t tcp_sendpage(struct socket *sock
#define TCP_PAGE(sk) (sk->sk_sndmsg_page)
#define TCP_OFF(sk) (sk->sk_sndmsg_off)
-static inline int select_size(struct sock *sk)
+static inline int select_size(struct sock *sk, int sg)
{
struct tcp_sock *tp = tcp_sk(sk);
int tmp = tp->mss_cache;
- if (sk->sk_route_caps & NETIF_F_SG) {
+ if (sg) {
if (sk_can_gso(sk))
tmp = 0;
else {
@@ -907,7 +907,7 @@ int tcp_sendmsg(struct kiocb *iocb, stru
struct sk_buff *skb;
int iovlen, flags;
int mss_now, size_goal;
- int err, copied;
+ int sg, err, copied;
long timeo;
lock_sock(sk);
@@ -935,6 +935,8 @@ int tcp_sendmsg(struct kiocb *iocb, stru
if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
goto out_err;
+ sg = sk->sk_route_caps & NETIF_F_SG;
+
while (--iovlen >= 0) {
int seglen = iov->iov_len;
unsigned char __user *from = iov->iov_base;
@@ -960,8 +962,9 @@ new_segment:
if (!sk_stream_memory_free(sk))
goto wait_for_sndbuf;
- skb = sk_stream_alloc_skb(sk, select_size(sk),
- sk->sk_allocation);
+ skb = sk_stream_alloc_skb(sk,
+ select_size(sk, sg),
+ sk->sk_allocation);
if (!skb)
goto wait_for_memory;
@@ -998,9 +1001,7 @@ new_segment:
/* We can extend the last page
* fragment. */
merge = 1;
- } else if (i == MAX_SKB_FRAGS ||
- (!i &&
- !(sk->sk_route_caps & NETIF_F_SG))) {
+ } else if (i == MAX_SKB_FRAGS || !sg) {
/* Need to add new fragment and cannot
* do this because interface is non-SG,
* or because all the page slots are
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames
2009-12-09 8:26 [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Krishna Kumar
2009-12-09 8:26 ` [PATCH 2/3] tcp: Remove unrequired operations in tcp_push() Krishna Kumar
2009-12-09 8:26 ` [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg Krishna Kumar
@ 2009-12-10 8:05 ` Ilpo Järvinen
2009-12-23 22:15 ` David Miller
3 siblings, 0 replies; 13+ messages in thread
From: Ilpo Järvinen @ 2009-12-10 8:05 UTC (permalink / raw)
To: Krishna Kumar; +Cc: davem, netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2437 bytes --]
On Wed, 9 Dec 2009, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
>
> tcp_push checks tcp_send_head and calls __tcp_push_pending_frames,
> which again checks tcp_send_head, and this unnecessary check is
> done for every other caller of __tcp_push_pending_frames.
>
> Remove tcp_send_head check in __tcp_push_pending_frames and add
> the check to tcp_push_pending_frames. Other functions call
> __tcp_push_pending_frames only when tcp_send_head would evaluate
> to true.
>
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
> include/net/tcp.h | 16 +++++++++-------
> net/ipv4/tcp_output.c | 5 -----
> 2 files changed, 9 insertions(+), 12 deletions(-)
>
> diff -ruNp org/include/net/tcp.h new/include/net/tcp.h
> --- org/include/net/tcp.h 2009-12-04 18:15:26.000000000 +0530
> +++ new/include/net/tcp.h 2009-12-09 09:57:39.000000000 +0530
> @@ -857,13 +857,6 @@ static inline void tcp_check_probe_timer
> icsk->icsk_rto, TCP_RTO_MAX);
> }
>
> -static inline void tcp_push_pending_frames(struct sock *sk)
> -{
> - struct tcp_sock *tp = tcp_sk(sk);
> -
> - __tcp_push_pending_frames(sk, tcp_current_mss(sk), tp->nonagle);
> -}
> -
> static inline void tcp_init_wl(struct tcp_sock *tp, u32 seq)
> {
> tp->snd_wl1 = seq;
> @@ -1366,6 +1359,15 @@ static inline int tcp_write_queue_empty(
> return skb_queue_empty(&sk->sk_write_queue);
> }
>
> +static inline void tcp_push_pending_frames(struct sock *sk)
> +{
> + if (tcp_send_head(sk)) {
> + struct tcp_sock *tp = tcp_sk(sk);
> +
> + __tcp_push_pending_frames(sk, tcp_current_mss(sk), tp->nonagle);
> + }
> +}
> +
> /* Start sequence of the highest skb with SACKed bit, valid only if
> * sacked > 0 or when the caller has ensured validity by itself.
> */
> diff -ruNp org/net/ipv4/tcp_output.c new/net/ipv4/tcp_output.c
> --- org/net/ipv4/tcp_output.c 2009-12-04 18:16:00.000000000 +0530
> +++ new/net/ipv4/tcp_output.c 2009-12-04 18:16:24.000000000 +0530
> @@ -1799,11 +1799,6 @@ static int tcp_write_xmit(struct sock *s
> void __tcp_push_pending_frames(struct sock *sk, unsigned int cur_mss,
> int nonagle)
> {
> - struct sk_buff *skb = tcp_send_head(sk);
> -
> - if (!skb)
> - return;
> -
> /* If we are closed, the bytes will have to remain here.
> * In time closedown will finish, we empty the write queue and
> * all will be happy.
Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
2009-12-09 8:26 ` [PATCH 2/3] tcp: Remove unrequired operations in tcp_push() Krishna Kumar
@ 2009-12-10 8:10 ` Ilpo Järvinen
2009-12-10 9:06 ` Krishna Kumar2
0 siblings, 1 reply; 13+ messages in thread
From: Ilpo Järvinen @ 2009-12-10 8:10 UTC (permalink / raw)
To: Krishna Kumar; +Cc: davem, netdev
On Wed, 9 Dec 2009, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
>
> Remove unrequired operations in tcp_push()
>
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
> net/ipv4/tcp.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff -ruNp org/net/ipv4/tcp.c new/net/ipv4/tcp.c
> --- org/net/ipv4/tcp.c 2009-12-04 18:16:10.000000000 +0530
> +++ new/net/ipv4/tcp.c 2009-12-09 10:01:35.000000000 +0530
> @@ -536,8 +536,7 @@ static inline void skb_entail(struct soc
> tp->nonagle &= ~TCP_NAGLE_PUSH;
> }
>
> -static inline void tcp_mark_urg(struct tcp_sock *tp, int flags,
> - struct sk_buff *skb)
> +static inline void tcp_mark_urg(struct tcp_sock *tp, int flags)
> {
> if (flags & MSG_OOB)
> tp->snd_up = tp->write_seq;
> @@ -546,13 +545,15 @@ static inline void tcp_mark_urg(struct t
> static inline void tcp_push(struct sock *sk, int flags, int mss_now,
> int nonagle)
> {
> - struct tcp_sock *tp = tcp_sk(sk);
> -
> if (tcp_send_head(sk)) {
> - struct sk_buff *skb = tcp_write_queue_tail(sk);
> - if (!(flags & MSG_MORE) || forced_push(tp))
> + struct tcp_sock *tp = tcp_sk(sk);
> +
> + if (!(flags & MSG_MORE) || forced_push(tp)) {
> + struct sk_buff *skb = tcp_write_queue_tail(sk);
> +
> tcp_mark_push(tp, skb);
I suppose one could kill the temporary variable completely then?
> - tcp_mark_urg(tp, flags, skb);
> + }
> + tcp_mark_urg(tp, flags);
> __tcp_push_pending_frames(sk, mss_now,
> (flags & MSG_MORE) ? TCP_NAGLE_CORK : nonagle);
> }
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg
2009-12-09 8:26 ` [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg Krishna Kumar
@ 2009-12-10 8:17 ` Ilpo Järvinen
2009-12-10 10:29 ` Krishna Kumar2
0 siblings, 1 reply; 13+ messages in thread
From: Ilpo Järvinen @ 2009-12-10 8:17 UTC (permalink / raw)
To: Krishna Kumar; +Cc: David Miller, Netdev
On Wed, 9 Dec 2009, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
>
> Slightly optimize tcp_sendmsg since NETIF_F_SG is used many
> times iteratively in the loop. The only other modification is
> to change:
> } else if (i == MAX_SKB_FRAGS ||
> (!i &&
> !(sk->sk_route_caps & NETIF_F_SG))) {
> to:
> } else if (i == MAX_SKB_FRAGS || !sg) {
I can see that you make this change from the patch itself but could you
give a justification on why dropping the !i is possible? ...I couldn't
see what would allow that.
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
> net/ipv4/tcp.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff -ruNp org/net/ipv4/tcp.c new/net/ipv4/tcp.c
> --- org/net/ipv4/tcp.c 2009-12-09 10:01:35.000000000 +0530
> +++ new/net/ipv4/tcp.c 2009-12-09 10:26:42.000000000 +0530
> @@ -878,12 +878,12 @@ ssize_t tcp_sendpage(struct socket *sock
> #define TCP_PAGE(sk) (sk->sk_sndmsg_page)
> #define TCP_OFF(sk) (sk->sk_sndmsg_off)
>
> -static inline int select_size(struct sock *sk)
> +static inline int select_size(struct sock *sk, int sg)
> {
> struct tcp_sock *tp = tcp_sk(sk);
> int tmp = tp->mss_cache;
>
> - if (sk->sk_route_caps & NETIF_F_SG) {
> + if (sg) {
> if (sk_can_gso(sk))
> tmp = 0;
> else {
> @@ -907,7 +907,7 @@ int tcp_sendmsg(struct kiocb *iocb, stru
> struct sk_buff *skb;
> int iovlen, flags;
> int mss_now, size_goal;
> - int err, copied;
> + int sg, err, copied;
> long timeo;
>
> lock_sock(sk);
> @@ -935,6 +935,8 @@ int tcp_sendmsg(struct kiocb *iocb, stru
> if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
> goto out_err;
>
> + sg = sk->sk_route_caps & NETIF_F_SG;
> +
> while (--iovlen >= 0) {
> int seglen = iov->iov_len;
> unsigned char __user *from = iov->iov_base;
> @@ -960,8 +962,9 @@ new_segment:
> if (!sk_stream_memory_free(sk))
> goto wait_for_sndbuf;
>
> - skb = sk_stream_alloc_skb(sk, select_size(sk),
> - sk->sk_allocation);
> + skb = sk_stream_alloc_skb(sk,
> + select_size(sk, sg),
> + sk->sk_allocation);
> if (!skb)
> goto wait_for_memory;
>
> @@ -998,9 +1001,7 @@ new_segment:
> /* We can extend the last page
> * fragment. */
> merge = 1;
> - } else if (i == MAX_SKB_FRAGS ||
> - (!i &&
> - !(sk->sk_route_caps & NETIF_F_SG))) {
> + } else if (i == MAX_SKB_FRAGS || !sg) {
> /* Need to add new fragment and cannot
> * do this because interface is non-SG,
> * or because all the page slots are
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
2009-12-10 8:10 ` Ilpo Järvinen
@ 2009-12-10 9:06 ` Krishna Kumar2
2009-12-10 10:26 ` Ilpo Järvinen
0 siblings, 1 reply; 13+ messages in thread
From: Krishna Kumar2 @ 2009-12-10 9:06 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: davem, netdev
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009 01:40:51
PM:
> Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
>
>
> > static inline void tcp_push(struct sock *sk, int flags, int mss_now,
> > int nonagle)
> > {
> > - struct tcp_sock *tp = tcp_sk(sk);
> > -
> > if (tcp_send_head(sk)) {
> > - struct sk_buff *skb = tcp_write_queue_tail(sk);
> > - if (!(flags & MSG_MORE) || forced_push(tp))
> > + struct tcp_sock *tp = tcp_sk(sk);
> > +
> > + if (!(flags & MSG_MORE) || forced_push(tp)) {
> > + struct sk_buff *skb = tcp_write_queue_tail(sk);
> > +
> > tcp_mark_push(tp, skb);
>
> I suppose one could kill the temporary variable completely then?
I did consider that, but kept it this way for readability reasons.
Should I change it?
Thanks,
- KK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
2009-12-10 9:06 ` Krishna Kumar2
@ 2009-12-10 10:26 ` Ilpo Järvinen
2009-12-10 11:59 ` Krishna Kumar2
0 siblings, 1 reply; 13+ messages in thread
From: Ilpo Järvinen @ 2009-12-10 10:26 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: David Miller, Netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1212 bytes --]
On Thu, 10 Dec 2009, Krishna Kumar2 wrote:
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009 01:40:51
> PM:
>
> > Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
> >
> >
> > > static inline void tcp_push(struct sock *sk, int flags, int mss_now,
> > > int nonagle)
> > > {
> > > - struct tcp_sock *tp = tcp_sk(sk);
> > > -
> > > if (tcp_send_head(sk)) {
> > > - struct sk_buff *skb = tcp_write_queue_tail(sk);
> > > - if (!(flags & MSG_MORE) || forced_push(tp))
> > > + struct tcp_sock *tp = tcp_sk(sk);
> > > +
> > > + if (!(flags & MSG_MORE) || forced_push(tp)) {
> > > + struct sk_buff *skb = tcp_write_queue_tail(sk);
> > > +
> > > tcp_mark_push(tp, skb);
> >
> > I suppose one could kill the temporary variable completely then?
>
> I did consider that, but kept it this way for readability reasons.
> Should I change it?
Honestly that doesn't look that fuzzy code even if you'd stick it into the
tcp_mark_push line (nor should be even close to 80 limit). ...I was even
thinking of getting totally rid of that skb arg of tcp_mark_push as I
think it's always tcp_write_queue_tail(sk) that is put there.
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg
2009-12-10 8:17 ` Ilpo Järvinen
@ 2009-12-10 10:29 ` Krishna Kumar2
2009-12-10 10:42 ` Ilpo Järvinen
0 siblings, 1 reply; 13+ messages in thread
From: Krishna Kumar2 @ 2009-12-10 10:29 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: David Miller, Netdev
Hi Ilpo,
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009 01:47:30
PM:
> Subject: Re: [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg>
>
> > Slightly optimize tcp_sendmsg since NETIF_F_SG is used many
> > times iteratively in the loop. The only other modification is
> > to change:
> > } else if (i == MAX_SKB_FRAGS ||
> > (!i &&
> > !(sk->sk_route_caps & NETIF_F_SG))) {
> > to:
> > } else if (i == MAX_SKB_FRAGS || !sg) {
>
> I can see that you make this change from the patch itself but could you
> give a justification on why dropping the !i is possible? ...I couldn't
> see what would allow that.
From what *I understood*, this code (other than the MAX_SKB_FRAGS case)
executes only due to the else part of "if (skb_tailroom(skb) > 0) {" -
there was no space in the skb to put the data inline. Hence SG is false
is a sufficient condition and there is no way to add a fragment (i == 0
since skb_fill_page_desc cannot be called when !SG).
thanks,
- KK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg
2009-12-10 10:29 ` Krishna Kumar2
@ 2009-12-10 10:42 ` Ilpo Järvinen
0 siblings, 0 replies; 13+ messages in thread
From: Ilpo Järvinen @ 2009-12-10 10:42 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: David Miller, Netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1409 bytes --]
On Thu, 10 Dec 2009, Krishna Kumar2 wrote:
> Hi Ilpo,
>
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009 01:47:30
> PM:
>
> > Subject: Re: [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg>
> >
> > > Slightly optimize tcp_sendmsg since NETIF_F_SG is used many
> > > times iteratively in the loop. The only other modification is
> > > to change:
> > > } else if (i == MAX_SKB_FRAGS ||
> > > (!i &&
> > > !(sk->sk_route_caps & NETIF_F_SG))) {
> > > to:
> > > } else if (i == MAX_SKB_FRAGS || !sg) {
> >
> > I can see that you make this change from the patch itself but could you
> > give a justification on why dropping the !i is possible? ...I couldn't
> > see what would allow that.
>
> From what *I understood*, this code (other than the MAX_SKB_FRAGS case)
> executes only due to the else part of "if (skb_tailroom(skb) > 0) {" -
> there was no space in the skb to put the data inline. Hence SG is false
> is a sufficient condition and there is no way to add a fragment (i == 0
> since skb_fill_page_desc cannot be called when !SG).
Please put this description into the patch description then :-). ...After
some more thinking, reading and verification it seems right, however, it
certainly wassn't immediately obvious from the code.
...You can add my Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> for
the resubmission.
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
2009-12-10 10:26 ` Ilpo Järvinen
@ 2009-12-10 11:59 ` Krishna Kumar2
2009-12-10 12:03 ` Ilpo Järvinen
0 siblings, 1 reply; 13+ messages in thread
From: Krishna Kumar2 @ 2009-12-10 11:59 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: David Miller, Netdev
"Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009 03:56:59
PM:
> Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
>
> On Thu, 10 Dec 2009, Krishna Kumar2 wrote:
>
> > "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009
01:40:51
> > PM:
> >
> > > Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
> > >
> > >
> > > > static inline void tcp_push(struct sock *sk, int flags, int
mss_now,
> > > > int nonagle)
> > > > {
> > > > - struct tcp_sock *tp = tcp_sk(sk);
> > > > -
> > > > if (tcp_send_head(sk)) {
> > > > - struct sk_buff *skb = tcp_write_queue_tail(sk);
> > > > - if (!(flags & MSG_MORE) || forced_push(tp))
> > > > + struct tcp_sock *tp = tcp_sk(sk);
> > > > +
> > > > + if (!(flags & MSG_MORE) || forced_push(tp)) {
> > > > + struct sk_buff *skb = tcp_write_queue_tail(sk);
> > > > +
> > > > tcp_mark_push(tp, skb);
> > >
> > > I suppose one could kill the temporary variable completely then?
> >
> > I did consider that, but kept it this way for readability reasons.
> > Should I change it?
>
> Honestly that doesn't look that fuzzy code even if you'd stick it into
the
> tcp_mark_push line (nor should be even close to 80 limit). ...I was even
> thinking of getting totally rid of that skb arg of tcp_mark_push as I
> think it's always tcp_write_queue_tail(sk) that is put there.
Yes, the skb is always the one at the tail. But in 4/5 cases, the
skb pointer is already available, so may be OK to pass the skb. I
will resubmit with the change you suggested (remove the temporary
variable).
Also, for the NETIF_F_SG patch, I will try to put a meaningful
explanation and resubmit.
Thanks for your review,
- KK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
2009-12-10 11:59 ` Krishna Kumar2
@ 2009-12-10 12:03 ` Ilpo Järvinen
0 siblings, 0 replies; 13+ messages in thread
From: Ilpo Järvinen @ 2009-12-10 12:03 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: David Miller, Netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1842 bytes --]
On Thu, 10 Dec 2009, Krishna Kumar2 wrote:
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009 03:56:59
> PM:
>
> > Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
> >
> > On Thu, 10 Dec 2009, Krishna Kumar2 wrote:
> >
> > > "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote on 12/10/2009
> 01:40:51
> > > PM:
> > >
> > > > Re: [PATCH 2/3] tcp: Remove unrequired operations in tcp_push()
> > > >
> > > >
> > > > > static inline void tcp_push(struct sock *sk, int flags, int
> mss_now,
> > > > > int nonagle)
> > > > > {
> > > > > - struct tcp_sock *tp = tcp_sk(sk);
> > > > > -
> > > > > if (tcp_send_head(sk)) {
> > > > > - struct sk_buff *skb = tcp_write_queue_tail(sk);
> > > > > - if (!(flags & MSG_MORE) || forced_push(tp))
> > > > > + struct tcp_sock *tp = tcp_sk(sk);
> > > > > +
> > > > > + if (!(flags & MSG_MORE) || forced_push(tp)) {
> > > > > + struct sk_buff *skb = tcp_write_queue_tail(sk);
> > > > > +
> > > > > tcp_mark_push(tp, skb);
> > > >
> > > > I suppose one could kill the temporary variable completely then?
> > >
> > > I did consider that, but kept it this way for readability reasons.
> > > Should I change it?
> >
> > Honestly that doesn't look that fuzzy code even if you'd stick it into
> the
> > tcp_mark_push line (nor should be even close to 80 limit). ...I was even
> > thinking of getting totally rid of that skb arg of tcp_mark_push as I
> > think it's always tcp_write_queue_tail(sk) that is put there.
>
> Yes, the skb is always the one at the tail. But in 4/5 cases, the
> skb pointer is already available, so may be OK to pass the skb.
...which was exactly why I didn't immediately suggest it :-).
> Also, for the NETIF_F_SG patch, I will try to put a meaningful
> explanation and resubmit.
Thanks.
--
i.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames
2009-12-09 8:26 [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Krishna Kumar
` (2 preceding siblings ...)
2009-12-10 8:05 ` [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Ilpo Järvinen
@ 2009-12-23 22:15 ` David Miller
3 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2009-12-23 22:15 UTC (permalink / raw)
To: krkumar2; +Cc: netdev
From: Krishna Kumar <krkumar2@in.ibm.com>
Date: Wed, 09 Dec 2009 13:56:13 +0530
> From: Krishna Kumar <krkumar2@in.ibm.com>
>
> tcp_push checks tcp_send_head and calls __tcp_push_pending_frames,
> which again checks tcp_send_head, and this unnecessary check is
> done for every other caller of __tcp_push_pending_frames.
>
> Remove tcp_send_head check in __tcp_push_pending_frames and add
> the check to tcp_push_pending_frames. Other functions call
> __tcp_push_pending_frames only when tcp_send_head would evaluate
> to true.
>
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
Applied.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2009-12-23 22:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-12-09 8:26 [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Krishna Kumar
2009-12-09 8:26 ` [PATCH 2/3] tcp: Remove unrequired operations in tcp_push() Krishna Kumar
2009-12-10 8:10 ` Ilpo Järvinen
2009-12-10 9:06 ` Krishna Kumar2
2009-12-10 10:26 ` Ilpo Järvinen
2009-12-10 11:59 ` Krishna Kumar2
2009-12-10 12:03 ` Ilpo Järvinen
2009-12-09 8:26 ` [PATCH 3/3] tcp: Slightly optimize tcp_sendmsg Krishna Kumar
2009-12-10 8:17 ` Ilpo Järvinen
2009-12-10 10:29 ` Krishna Kumar2
2009-12-10 10:42 ` Ilpo Järvinen
2009-12-10 8:05 ` [PATCH 1/3] tcp: Remove check in __tcp_push_pending_frames Ilpo Järvinen
2009-12-23 22:15 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.