From: Krishna Kumar <redacted>
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).
Signed-off-by: Krishna Kumar <redacted>
---
include/net/sock.h | 21 +++++++++++++++++++++
net/core/sock.c | 7 ++++++-
2 files changed, 27 insertions(+), 1 deletion(-)
diff -ruNp org/include/net/sock.h new/include/net/sock.h
From: Krishna Kumar <redacted>
For connected sockets, the first run of dev_pick_tx saves the
calculated txq in sk_tx_queue_mapping. This is not saved if
either skb rx was recorded, or if the device has a queue select
handler. Next iterations of dev_pick_tx uses the cached value of
sk_tx_queue_mapping.
Signed-off-by: Krishna Kumar <redacted>
---
net/core/dev.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff -ruNp org/net/core/dev.c new/net/core/dev.c
From: Krishna Kumar <redacted>
dst_negative_advice() should check for changed dst and reset
sk_tx_queue_mapping accordingly. Pass sock to the callers of
dst_negative_advice.
(sk_reset_txq is defined just for use by dst_negative_advice. The
only way I could find to get around this is to move dst_negative_()
from dst.h to dst.c, include sock.h in dst.c, etc)
Signed-off-by: Krishna Kumar <redacted>
---
include/net/dst.h | 12 ++++++++++--
net/core/sock.c | 6 ++++++
net/dccp/timer.c | 4 ++--
net/decnet/af_decnet.c | 2 +-
net/ipv4/tcp_timer.c | 4 ++--
5 files changed, 21 insertions(+), 7 deletions(-)
diff -ruNp org/include/net/dst.h new/include/net/dst.h
From: Eric Dumazet <hidden> Date: 2009-10-16 09:49:47
Krishna Kumar a écrit :
From: Krishna Kumar <redacted>
For connected sockets, the first run of dev_pick_tx saves the
calculated txq in sk_tx_queue_mapping. This is not saved if
either skb rx was recorded, or if the device has a queue select
handler. Next iterations of dev_pick_tx uses the cached value of
sk_tx_queue_mapping.
Are we sure that for selection done by skb_tx_hash(dev, skb),
rx packets will use the same queue/cpu ?
Probably not, since it uses sk->sk_hash (tcp/udp port) :
u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)
{
u32 hash;
if (skb_rx_queue_recorded(skb)) {
hash = skb_get_rx_queue(skb);
while (unlikely(hash >= dev->real_num_tx_queues))
hash -= dev->real_num_tx_queues;
return hash;
}
if (skb->sk && skb->sk->sk_hash)
hash = skb->sk->sk_hash;
else
hash = skb->protocol;
hash = jhash_1word(hash, skb_tx_hashrnd);
return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
}
If NIC has some proprietary hash, and selects rx queue 3 for feeding us
packets, it would be nice to also use tx queue 3 for transmit.
We would have to record in sk the rx queue chosen by the device
when processing SYN / SYN-ACK packet for example for tcp flows.
From: Eric Dumazet <hidden> Date: 2009-10-16 09:57:55
Krishna Kumar a écrit :
From: Krishna Kumar <redacted>
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);
}
Eric Dumazet [off-list ref] wrote on 10/16/2009 03:18:56 PM:
Are we sure that for selection done by skb_tx_hash(dev, skb),
rx packets will use the same queue/cpu ?
Probably not, since it uses sk->sk_hash (tcp/udp port) :
Yes, I expect the same.
If NIC has some proprietary hash, and selects rx queue 3 for feeding us
packets, it would be nice to also use tx queue 3 for transmit.
I had written: "This is not saved if either skb rx was recorded, or
...". It is somewhat misleading, and I will try to correct it. The
idea is that if the same incoming skb is used for response after the
connection is setup, which I think is not possible in this case, then
txq will be same as rxq. See more below...
We would have to record in sk the rx queue chosen by the device
when processing SYN / SYN-ACK packet for example for tcp flows.
I wait for tcp_v4_syn_recv_sock to finish and create a connection.
sk_setup_caps/__sk_dst_set are called for the new socket resulting
in sk_dst_cache getting set (I cannot set the cache till the
connection is finished and dst is set). I guess that means that I
will wait for another skb to come and the response will use skb_tx_hash
instead of using the rx recorded.
Will this suffice, but if not, is it something I should handle or an
add-on patch should do instead?
Thanks,
- KK
Eric Dumazet [off-list ref] wrote on 10/16/2009 03:27:09 PM:
quoted
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.
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
From: Eric Dumazet <hidden> Date: 2009-10-16 14:08:17
Krishna Kumar2 a écrit :
I wait for tcp_v4_syn_recv_sock to finish and create a connection.
sk_setup_caps/__sk_dst_set are called for the new socket resulting
in sk_dst_cache getting set (I cannot set the cache till the
connection is finished and dst is set). I guess that means that I
will wait for another skb to come and the response will use skb_tx_hash
instead of using the rx recorded.
Well, I see no relation between rx mapping and tx mapping currently.
Will this suffice, but if not, is it something I should handle or an
add-on patch should do instead?
Your patch is fine, I was only giving ideas for future patches :)
Hi Eric,
Eric Dumazet [off-list ref] wrote on 10/16/2009 07:37:30 PM:
quoted
I wait for tcp_v4_syn_recv_sock to finish and create a connection.
sk_setup_caps/__sk_dst_set are called for the new socket resulting
in sk_dst_cache getting set (I cannot set the cache till the
connection is finished and dst is set). I guess that means that I
will wait for another skb to come and the response will use skb_tx_hash
instead of using the rx recorded.
Well, I see no relation between rx mapping and tx mapping currently.
You are suggesting an enhancement to assign the txq# at ack processing
using the skb->rx#. As you said it will help cards that are hashing the
rx# internally. I will try that separately after this patch.
quoted
Will this suffice, but if not, is it something I should handle or an
add-on patch should do instead?
Your patch is fine, I was only giving ideas for future patches :)
Thanks for the suggestions, I just wanted to make sure.
- KK