| Message ID | 20091016072120.24384.54449.sendpatchset@localhost.localdomain |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Krishna Kumar a écrit : > From: Krishna Kumar <krkumar2@in.ibm.com> > > Introduce sk_tx_queue_mapping; and functions that set, test and get > this value. Reset sk_tx_queue_mapping to -1 whenever the dst cache > is set/reset, and in socket alloc & free (free probably doesn't need > it). > Could you please use an u16 instead, and take the convention of 0 being the 'unitialized value' ? And define sk_tx_queue_clear(sk) instead of sk_record_tx_queue(sk, -1); I also suggest using following names : static inline void sk_tx_queue_set(struct sock *sk, u16 tx_queue) { sk->sk_tx_queue_mapping = tx_queue + 1; } static inline u16 sk_tx_queue_get(const struct sock *sk) { return sk->sk_tx_queue_mapping - 1; } static inline u16 sk_tx_queue_clear(struct sock *sk) // or _reset { sk->sk_tx_queue_mapping = 0; } static inline bool sk_tx_queue_recorded(const struct sock *sk) { return (sk && sk->sk_tx_queue_mapping > 0); } > @@ -1016,6 +1019,8 @@ static void sk_prot_free(struct proto *p > struct kmem_cache *slab; > struct module *owner; > > + sk_record_tx_queue(sk, -1); > + > owner = prot->owner; > slab = prot->slab; > This is not necessary, we are going to kfree(sk) anyway ! -- 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> wrote on 10/16/2009 03:27:09 PM: > > > Introduce sk_tx_queue_mapping; and functions that set, test and get > > this value. Reset sk_tx_queue_mapping to -1 whenever the dst cache > > is set/reset, and in socket alloc & free (free probably doesn't need > > it). > > > > Could you please use an u16 instead, and take the convention of 0 > being the 'unitialized value' ? I wanted to use a signed number to avoid doing an unnecessary sub on every iteration. I feel u16 is good for incoming path since skb->queue_mapping is set to 0 by default, and only those drivers doing rx recording needs to set this value. So queue_mapping reqd the logic of using !0 to mean rx is set and the subsequent sub on all retrieves. But on tx path where sk->txq# is saved permanently, I feel it is not required. If that sounds reasonable, I can change to a signed short. > And define sk_tx_queue_clear(sk) instead of sk_record_tx_queue(sk, -1); > > I also suggest using following names : > > static inline void sk_tx_queue_set(struct sock *sk, u16 tx_queue) > { > sk->sk_tx_queue_mapping = tx_queue + 1; > } > > static inline u16 sk_tx_queue_get(const struct sock *sk) > { > return sk->sk_tx_queue_mapping - 1; > } > > static inline u16 sk_tx_queue_clear(struct sock *sk) // or _reset > { > sk->sk_tx_queue_mapping = 0; > } > > static inline bool sk_tx_queue_recorded(const struct sock *sk) > { > return (sk && sk->sk_tx_queue_mapping > 0); > } > > > > @@ -1016,6 +1019,8 @@ static void sk_prot_free(struct proto *p > > struct kmem_cache *slab; > > struct module *owner; > > > > + sk_record_tx_queue(sk, -1); > > + > > owner = prot->owner; > > slab = prot->slab; > > > > This is not necessary, we are going to kfree(sk) anyway ! Yes, will make these changes, but please let me know your opinion on "return sk->sk_tx_queue_mapping - 1" vs "return sk->sk_tx_queue_mapping". 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 10:36:52.000000000 +0530 +++ new/include/net/sock.h 2009-10-16 12:09:54.000000000 +0530 @@ -107,6 +107,7 @@ struct net; * @skc_node: main hash linkage for various protocol lookup tables * @skc_nulls_node: main hash linkage for UDP/UDP-Lite protocol * @skc_refcnt: reference count + * @skc_tx_queue_mapping: tx queue number for this connection * @skc_hash: hash value used with various protocol lookup tables * @skc_family: network address family * @skc_state: Connection state @@ -128,6 +129,7 @@ struct sock_common { struct hlist_nulls_node skc_nulls_node; }; atomic_t skc_refcnt; + int skc_tx_queue_mapping; unsigned int skc_hash; unsigned short skc_family; @@ -215,6 +217,7 @@ struct sock { #define sk_node __sk_common.skc_node #define sk_nulls_node __sk_common.skc_nulls_node #define sk_refcnt __sk_common.skc_refcnt +#define sk_tx_queue_mapping __sk_common.skc_tx_queue_mapping #define sk_copy_start __sk_common.skc_hash #define sk_hash __sk_common.skc_hash @@ -1094,8 +1097,24 @@ static inline void sock_put(struct sock extern int sk_receive_skb(struct sock *sk, struct sk_buff *skb, const int nested); +static inline void sk_record_tx_queue(struct sock *sk, int tx_queue) +{ + sk->sk_tx_queue_mapping = tx_queue; +} + +static inline int sk_get_tx_queue(const struct sock *sk) +{ + return sk->sk_tx_queue_mapping; +} + +static inline bool sk_tx_queue_recorded(const struct sock *sk) +{ + return (sk && sk->sk_tx_queue_mapping >= 0); +} + static inline void sk_set_socket(struct sock *sk, struct socket *sock) { + sk_record_tx_queue(sk, -1); sk->sk_socket = sock; } @@ -1152,6 +1171,7 @@ __sk_dst_set(struct sock *sk, struct dst { struct dst_entry *old_dst; + sk_record_tx_queue(sk, -1); old_dst = sk->sk_dst_cache; sk->sk_dst_cache = dst; dst_release(old_dst); @@ -1170,6 +1190,7 @@ __sk_dst_reset(struct sock *sk) { 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); diff -ruNp org/net/core/sock.c new/net/core/sock.c --- org/net/core/sock.c 2009-10-16 11:53:40.000000000 +0530 +++ new/net/core/sock.c 2009-10-16 12:07:00.000000000 +0530 @@ -357,6 +357,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); return NULL; @@ -953,7 +954,8 @@ static void sock_copy(struct sock *nsk, void *sptr = nsk->sk_security; #endif BUILD_BUG_ON(offsetof(struct sock, sk_copy_start) != - sizeof(osk->sk_node) + sizeof(osk->sk_refcnt)); + sizeof(osk->sk_node) + sizeof(osk->sk_refcnt) + + sizeof(osk->sk_tx_queue_mapping)); memcpy(&nsk->sk_copy_start, &osk->sk_copy_start, osk->sk_prot->obj_size - offsetof(struct sock, sk_copy_start)); #ifdef CONFIG_SECURITY_NETWORK @@ -997,6 +999,7 @@ static struct sock *sk_prot_alloc(struct if (!try_module_get(prot->owner)) goto out_free_sec; + sk_record_tx_queue(sk, -1); } return sk; @@ -1016,6 +1019,8 @@ static void sk_prot_free(struct proto *p struct kmem_cache *slab; struct module *owner; + sk_record_tx_queue(sk, -1); + owner = prot->owner; slab = prot->slab;