| Message ID | 1448488250.24696.40.camel@edumazet-glaptop2.roam.corp.google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet <eric.dumazet@gmail.com> writes: > From: Eric Dumazet <edumazet@google.com> > > If tcp_send_ack() can not allocate skb, we properly handle this > and setup a timer to try later. > > Use __GFP_NOWARN to avoid polluting syslog in the case host is > under memory pressure, so that pertinent messages are not lost under > a flood of useless information. > > sk_gfp_atomic() can use its gfp_mask argument (all callers currently > were using GFP_ATOMIC before this patch) > > Note that when tcp_transmit_skb() is called with clone_it set to false, > we do not attempt memory allocations, so can pass a 0 gfp_mask, which > most compilers can emit faster than a non zero value. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > include/net/sock.h | 2 +- > net/ipv4/tcp_output.c | 12 +++++++----- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/include/net/sock.h b/include/net/sock.h > index 7f89e4ba18d1..ead514332ae8 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void) > > static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) > { > - return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC); > + return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); > } > Sorry if I'm missing something obvious here, but with a name like sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well? Otherwise, what is the _atomic is saying? Thanks, Aaron -- 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
On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote: > > diff --git a/include/net/sock.h b/include/net/sock.h > > index 7f89e4ba18d1..ead514332ae8 100644 > > --- a/include/net/sock.h > > +++ b/include/net/sock.h > > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void) > > > > static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) > > { > > - return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC); > > + return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); > > } > > > > Sorry if I'm missing something obvious here, but with a name like > sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well? > Otherwise, what is the _atomic is saying? Not sure what you suggest. Are you suggesting I remove GFP_ATOMIC from all callers ? I am fine with this, but looks more invasive, and who knows, maybe one caller might want to not use GFP_ATOMIC one day (like : do not attempt to use reserves) This sk_gfp_atomic() helper has a misleading name, since all it wanted was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one) with __GFP_MEMALLOC for some special sockets. Should have been sk_gfp_or_memalloc() or something... -- 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
On Wed, 2015-11-25 at 14:32 -0800, Eric Dumazet wrote: > On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote: > > > > diff --git a/include/net/sock.h b/include/net/sock.h > > > index 7f89e4ba18d1..ead514332ae8 100644 > > > --- a/include/net/sock.h > > > +++ b/include/net/sock.h > > > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void) > > > > > > static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) > > > { > > > - return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC); > > > + return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); > > > } > > > > > > > Sorry if I'm missing something obvious here, but with a name like > > sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well? > > Otherwise, what is the _atomic is saying? > > Not sure what you suggest. > > Are you suggesting I remove GFP_ATOMIC from all callers ? > > I am fine with this, but looks more invasive, and who knows, maybe one > caller might want to not use GFP_ATOMIC one day (like : do not attempt > to use reserves) > > This sk_gfp_atomic() helper has a misleading name, since all it wanted > was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one) > with __GFP_MEMALLOC for some special sockets. > > Should have been sk_gfp_or_memalloc() or something... > BTW original commit changelog was clear and matches my expectations : commit 99a1dec70d5acbd8c6b3928cdebb4a2d1da676c8 Author: Mel Gorman <mgorman@suse.de> Date: Tue Jul 31 16:44:14 2012 -0700 net: introduce sk_gfp_atomic() to allow addition of GFP flags depending on the individual socket Introduce sk_gfp_atomic(), this function allows to inject sock specific flags to each sock related allocation. It is only used on allocation paths that may be required for writing pages back to network storage. -- 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
Eric Dumazet <eric.dumazet@gmail.com> writes: > On Wed, 2015-11-25 at 14:32 -0800, Eric Dumazet wrote: >> On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote: >> >> > > diff --git a/include/net/sock.h b/include/net/sock.h >> > > index 7f89e4ba18d1..ead514332ae8 100644 >> > > --- a/include/net/sock.h >> > > +++ b/include/net/sock.h >> > > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void) >> > > >> > > static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) >> > > { >> > > - return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC); >> > > + return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); >> > > } >> > > >> > >> > Sorry if I'm missing something obvious here, but with a name like >> > sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well? >> > Otherwise, what is the _atomic is saying? >> >> Not sure what you suggest. >> >> Are you suggesting I remove GFP_ATOMIC from all callers ? That's an option, and one that looks pretty clean. >> I am fine with this, but looks more invasive, and who knows, maybe one >> caller might want to not use GFP_ATOMIC one day (like : do not attempt >> to use reserves) Probably that would call for a different more primitive version of this API (sk_gfp_or_memalloc() as you suggest below). Then this could be written in terms of that static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask) { return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); } static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) { return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC); } Not sure if it's "too much API". >> This sk_gfp_atomic() helper has a misleading name, since all it wanted >> was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one) >> with __GFP_MEMALLOC for some special sockets. >> >> Should have been sk_gfp_or_memalloc() or something... >> > > BTW original commit changelog was clear and matches my expectations : > > commit 99a1dec70d5acbd8c6b3928cdebb4a2d1da676c8 > Author: Mel Gorman <mgorman@suse.de> > Date: Tue Jul 31 16:44:14 2012 -0700 > > net: introduce sk_gfp_atomic() to allow addition of GFP flags > depending on the individual socket > > Introduce sk_gfp_atomic(), this function allows to inject sock specific > flags to each sock related allocation. It is only used on allocation > paths that may be required for writing pages back to network storage. Cool. If you think my suggestion is too much for this set, that's fine. I understand not wanting to be too intrusive. Thanks, Aaron -- 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
On Wed, 2015-11-25 at 22:17 -0500, Aaron Conole wrote: > Probably that would call for a different more primitive version of this > API (sk_gfp_or_memalloc() as you suggest below). Then this could be > written in terms of that > > static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask) > { > return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); > } > > static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) > { > return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC); > } > > Not sure if it's "too much API". Well, this looks like it, not sure how this is going to make code clearer. The only thing we bring from sk is the __GFP_MEMALLOC thing, so a single function seems enough ? I honestly do not care that much about function names, I mostly look at actual implementation. And current implementation ignores the gfp_t gfp_mask argument, for no real good reason. -- 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
Eric Dumazet <eric.dumazet@gmail.com> writes: > On Wed, 2015-11-25 at 22:17 -0500, Aaron Conole wrote: > >> Probably that would call for a different more primitive version of this >> API (sk_gfp_or_memalloc() as you suggest below). Then this could be >> written in terms of that >> >> static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask) >> { >> return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); >> } >> >> static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) >> { >> return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC); >> } >> >> Not sure if it's "too much API". > > Well, this looks like it, not sure how this is going to make code > clearer. > > The only thing we bring from sk is the __GFP_MEMALLOC thing, so a single > function seems enough ? Okay. Just thought that a 'gfp' function ending in _atomic that doesn't actually set GFP_ATOMIC might now confuse, but if you think it's no big deal, hey no skin off my back :) > I honestly do not care that much about function names, I mostly look at > actual implementation. And current implementation ignores the gfp_t > gfp_mask argument, for no real good reason. I agree with this. -- 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 25 Nov 2015 13:50:50 -0800 > diff --git a/include/net/sock.h b/include/net/sock.h > index 7f89e4ba18d1..ead514332ae8 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void) > > static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) > { > - return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC); > + return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); > } > > static inline void sk_acceptq_removed(struct sock *sk) Eric, please rename this to "sk_gfp_mask()" or "sk_gfp_flags()" or something like that since it doesn't unconditionally use GFP_ATOMIC any more. Otherwise I'm %100 fine with this change. Thank you. -- 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
diff --git a/include/net/sock.h b/include/net/sock.h index 7f89e4ba18d1..ead514332ae8 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void) static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask) { - return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC); + return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC); } static inline void sk_acceptq_removed(struct sock *sk) diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index cb7ca569052c..0a1d4f6ab52f 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3352,8 +3352,9 @@ void tcp_send_ack(struct sock *sk) * tcp_transmit_skb() will set the ownership to this * sock. */ - buff = alloc_skb(MAX_TCP_HEADER, sk_gfp_atomic(sk, GFP_ATOMIC)); - if (!buff) { + buff = alloc_skb(MAX_TCP_HEADER, + sk_gfp_atomic(sk, GFP_ATOMIC | __GFP_NOWARN)); + if (unlikely(!buff)) { inet_csk_schedule_ack(sk); inet_csk(sk)->icsk_ack.ato = TCP_ATO_MIN; inet_csk_reset_xmit_timer(sk, ICSK_TIME_DACK, @@ -3375,7 +3376,7 @@ void tcp_send_ack(struct sock *sk) /* Send it off, this clears delayed acks for us. */ skb_mstamp_get(&buff->skb_mstamp); - tcp_transmit_skb(sk, buff, 0, sk_gfp_atomic(sk, GFP_ATOMIC)); + tcp_transmit_skb(sk, buff, 0, (__force gfp_t)0); } EXPORT_SYMBOL_GPL(tcp_send_ack); @@ -3396,7 +3397,8 @@ static int tcp_xmit_probe_skb(struct sock *sk, int urgent, int mib) struct sk_buff *skb; /* We don't queue it, tcp_transmit_skb() sets ownership. */ - skb = alloc_skb(MAX_TCP_HEADER, sk_gfp_atomic(sk, GFP_ATOMIC)); + skb = alloc_skb(MAX_TCP_HEADER, + sk_gfp_atomic(sk, GFP_ATOMIC | __GFP_NOWARN)); if (!skb) return -1; @@ -3409,7 +3411,7 @@ static int tcp_xmit_probe_skb(struct sock *sk, int urgent, int mib) tcp_init_nondata_skb(skb, tp->snd_una - !urgent, TCPHDR_ACK); skb_mstamp_get(&skb->skb_mstamp); NET_INC_STATS(sock_net(sk), mib); - return tcp_transmit_skb(sk, skb, 0, GFP_ATOMIC); + return tcp_transmit_skb(sk, skb, 0, (__force gfp_t)0); } void tcp_send_window_probe(struct sock *sk)