Message ID | 20091015055758.30145.77495.sendpatchset@localhost.localdomain |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Krishna Kumar a écrit : > From: Krishna Kumar <krkumar2@in.ibm.com> > > Reuse code by adding ___sk_dst_reset() which is called by > __sk_dst_reset() and __sk_dst_check(). This new API is also > called from IPv6 to hide the internals of __sk_dst_reset > and the setting of sk_tx_queue_mapping. > > Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com> > --- > include/net/sock.h | 13 +++++++++---- > net/core/sock.c | 4 +--- > net/ipv6/inet6_connection_sock.c | 4 +--- > 3 files changed, 11 insertions(+), 10 deletions(-) > > diff -ruNp org/include/net/sock.h new/include/net/sock.h > --- org/include/net/sock.h 2009-10-14 18:00:17.000000000 +0530 > +++ new/include/net/sock.h 2009-10-14 18:00:30.000000000 +0530 > @@ -1186,17 +1186,22 @@ sk_dst_set(struct sock *sk, struct dst_e > } > > static inline void > -__sk_dst_reset(struct sock *sk) > +___sk_dst_reset(struct sock *sk, struct dst_entry *old_dst) > { > - struct dst_entry *old_dst; > - > sk_record_tx_queue(sk, -1); > - old_dst = sk->sk_dst_cache; > sk->sk_dst_cache = NULL; > dst_release(old_dst); > } > > static inline void > +__sk_dst_reset(struct sock *sk) > +{ > + struct dst_entry *old_dst = sk->sk_dst_cache; > + > + ___sk_dst_reset(sk, old_dst); > +} > + > +static inline void > sk_dst_reset(struct sock *sk) > { > write_lock(&sk->sk_dst_lock); > diff -ruNp org/net/core/sock.c new/net/core/sock.c > --- org/net/core/sock.c 2009-10-14 18:00:22.000000000 +0530 > +++ new/net/core/sock.c 2009-10-14 18:00:30.000000000 +0530 > @@ -364,9 +364,7 @@ struct dst_entry *__sk_dst_check(struct > struct dst_entry *dst = sk->sk_dst_cache; > > if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) { > - sk_record_tx_queue(sk, -1); > - sk->sk_dst_cache = NULL; > - dst_release(dst); > + ___sk_dst_reset(sk, dst); > return NULL; > } > > diff -ruNp org/net/ipv6/inet6_connection_sock.c new/net/ipv6/inet6_connection_sock.c > --- org/net/ipv6/inet6_connection_sock.c 2009-10-14 18:00:17.000000000 +0530 > +++ new/net/ipv6/inet6_connection_sock.c 2009-10-14 18:00:30.000000000 +0530 > @@ -168,9 +168,7 @@ struct dst_entry *__inet6_csk_dst_check( > if (dst) { > struct rt6_info *rt = (struct rt6_info *)dst; > if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) { > - sk_record_tx_queue(sk, -1); > - sk->sk_dst_cache = NULL; > - dst_release(dst); > + ___sk_dst_reset(sk, dst); > dst = NULL; > } > } > > Encapsulation seems un-necessary to me, since only use cases are ___sk_dst_reset(sk, sk->sk_dst_cache) static inline void __sk_dst_reset(struct sock *sk) { struct dst_entry *old_dst = sk->sk_dst_cache; sk_record_tx_queue(sk, -1); sk->sk_dst_cache = NULL; dst_release(old_dst); } -- 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
Hi Eric, Eric Dumazet <eric.dumazet@gmail.com> wrote on 10/15/2009 03:23:30 PM: I am responding in one post for your convenience. > Hmm, why not cache ops->ndo_select_queue(dev, skb) choice too ? > > if (ops->ndo_select_queue) > queue_index = ops->ndo_select_queue(dev, skb); > else { > queue_index = 0; > if (dev->real_num_tx_queues > 1) > queue_index = skb_tx_hash(dev, skb); > } > if (sk && sk->sk_dst_cache) > sk_record_tx_queue(sk, queue_index); > > Or should ndo_select_queue() method take care of calling sk_record_tx_queue() itself ? I initially cached for ndo_select_queue too, but felt it should not do that and leave to the driver for each skb. But your idea of the driver caching is good - I think ixgbe_select_queue & mlx4_en_select_queue (but not iwm_select_queue) can internally cache it if they are calling skb_tx_hash. > > diff -ruNp org/net/ipv6/inet6_connection_sock.c new/net/ipv6/inet6_connection_sock.c > > --- org/net/ipv6/inet6_connection_sock.c 2009-10-14 18:00:17.000000000 +0530 > > +++ new/net/ipv6/inet6_connection_sock.c 2009-10-14 18:00:30.000000000 +0530 > > @@ -168,9 +168,7 @@ struct dst_entry *__inet6_csk_dst_check( > > if (dst) { > > struct rt6_info *rt = (struct rt6_info *)dst; > > if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) { > > - sk_record_tx_queue(sk, -1); > > - sk->sk_dst_cache = NULL; > > - dst_release(dst); > > + ___sk_dst_reset(sk, dst); > > dst = NULL; > > } > > } > > Encapsulation seems un-necessary to me, since only use cases are > ___sk_dst_reset(sk, sk->sk_dst_cache) > > > static inline void __sk_dst_reset(struct sock *sk) > { > struct dst_entry *old_dst = sk->sk_dst_cache; > > sk_record_tx_queue(sk, -1); > sk->sk_dst_cache = NULL; > dst_release(old_dst); > } That's right. For the IPv6 case, I can simply call __sk_dst_reset(sk). I will make this change and remove [patch 5/5]. > Hmm, two remarks : > > 1) It adds a 32bits hole on 64bit arches > 2) sk_tx_queue_mapping is only read in tx path, but sits close to > skc_refcnt, which is now only read/written in rx path (by socket lookups) > > But since sock_common is small, 56 bytes on x86_64,(under a cache line), > there is nothing we can do at this moment. > > My plan is to move skc_refcnt at the end of sock_common and I'll need to add > new generic fields into sock_common to make offsetof(skc_refcnt) = 64. > > Next to sock_common, will be placed fields used in rx path. So that will fix this problem too as tx mapping will then be part of read-cache line, I guess. > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Thanks for your feedback and approval. I will wait for any more comments (on the ndo_select_queue also), and resubmit v2 tomorrow. Thanks, - KK -- 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 -ruNp org/include/net/sock.h new/include/net/sock.h --- org/include/net/sock.h 2009-10-14 18:00:17.000000000 +0530 +++ new/include/net/sock.h 2009-10-14 18:00:30.000000000 +0530 @@ -1186,17 +1186,22 @@ sk_dst_set(struct sock *sk, struct dst_e } static inline void -__sk_dst_reset(struct sock *sk) +___sk_dst_reset(struct sock *sk, struct dst_entry *old_dst) { - struct dst_entry *old_dst; - sk_record_tx_queue(sk, -1); - old_dst = sk->sk_dst_cache; sk->sk_dst_cache = NULL; dst_release(old_dst); } static inline void +__sk_dst_reset(struct sock *sk) +{ + struct dst_entry *old_dst = sk->sk_dst_cache; + + ___sk_dst_reset(sk, old_dst); +} + +static inline void sk_dst_reset(struct sock *sk) { write_lock(&sk->sk_dst_lock); diff -ruNp org/net/core/sock.c new/net/core/sock.c --- org/net/core/sock.c 2009-10-14 18:00:22.000000000 +0530 +++ new/net/core/sock.c 2009-10-14 18:00:30.000000000 +0530 @@ -364,9 +364,7 @@ struct dst_entry *__sk_dst_check(struct struct dst_entry *dst = sk->sk_dst_cache; if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) { - sk_record_tx_queue(sk, -1); - sk->sk_dst_cache = NULL; - dst_release(dst); + ___sk_dst_reset(sk, dst); return NULL; } diff -ruNp org/net/ipv6/inet6_connection_sock.c new/net/ipv6/inet6_connection_sock.c --- org/net/ipv6/inet6_connection_sock.c 2009-10-14 18:00:17.000000000 +0530 +++ new/net/ipv6/inet6_connection_sock.c 2009-10-14 18:00:30.000000000 +0530 @@ -168,9 +168,7 @@ struct dst_entry *__inet6_csk_dst_check( if (dst) { struct rt6_info *rt = (struct rt6_info *)dst; if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) { - sk_record_tx_queue(sk, -1); - sk->sk_dst_cache = NULL; - dst_release(dst); + ___sk_dst_reset(sk, dst); dst = NULL; } }